Fix calculations locating the explicit bit in the bit-packed `AttributeContainer$Large`. If the number of attributes is a multiple of 8, the explicit bit for the last attribute is leaking into the array element for the first attribute's value and clobbering it. PiperOrigin-RevId: 510179709 Change-Id: Ifa04204fd645d3e8817eaf600de42a46b8434d28
diff --git a/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java b/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java index 01450dd..227316d 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java
@@ -321,25 +321,32 @@ // - stateIndex: an index into the state[] array. // - valueIndex: an index into the attributeValues array. + /** Calculates the number of bytes necessary to have an explicit bit for each attribute. */ private static int prefixSize(int attrCount) { // ceil(max attributes / 8) - return (attrCount + 7) >> 3; + return (attrCount + 7) / 8; } - /** Set the specified bit in the byte array. Assumes bitIndex is a valid index. */ - private static void setBit(byte[] bits, int bitIndex) { - int idx = (bitIndex + 1); - int explicitByte = bits[idx >> 3]; - byte mask = (byte) (1 << (idx & 0x07)); - bits[idx >> 3] = (byte) (explicitByte | mask); + /** + * Sets the explicit bit for {@code attrIndex} in the byte array. Assumes {@code attrIndex} is a + * valid index. + */ + private static void setExplicitBit(byte[] bytes, int attrIndex) { + int byteIndex = attrIndex / 8; + int bitIndex = attrIndex % 8; + byte byteValue = bytes[byteIndex]; + bytes[byteIndex] = (byte) (byteValue | (1 << bitIndex)); } - /** Get the specified bit in the byte array. Assumes bitIndex is a valid index. */ - private static boolean getBit(byte[] bits, int bitIndex) { - int idx = (bitIndex + 1); - int explicitByte = bits[idx >> 3]; - int mask = (byte) (1 << (idx & 0x07)); - return (explicitByte & mask) != 0; + /** + * Gets the explicit bit for {@code attrIndex} in the byte array. Assumes {@code attrIndex} is a + * valid index. + */ + private static boolean getExplicitBit(byte[] bytes, int attrIndex) { + int byteIndex = attrIndex / 8; + int bitIndex = attrIndex % 8; + byte byteValue = bytes[byteIndex]; + return (byteValue & (1 << bitIndex)) != 0; } /** @@ -367,7 +374,7 @@ continue; } if (explicitAttrs.get(attrIndex)) { - setBit(state, attrIndex); + setExplicitBit(state, attrIndex); } state[index + p] = (byte) attrIndex; values[index] = attrValue; @@ -381,7 +388,7 @@ */ @Override boolean isAttributeValueExplicitlySpecified(int attrIndex) { - return (attrIndex >= 0) && getBit(state, attrIndex); + return (attrIndex >= 0) && getExplicitBit(state, attrIndex); } @Nullable
diff --git a/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java b/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java index a74204e..ee21b9b 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java
@@ -196,8 +196,8 @@ @Test public void testGetRawAttributeValuesReturnsCopy() { AttributeContainer mutable = new Mutable(2); - mutable.setAttributeValue(0, "hi", /*explicit=*/ true); - mutable.setAttributeValue(1, null, /*explicit=*/ false); + mutable.setAttributeValue(0, "hi", /* explicit= */ true); + mutable.setAttributeValue(1, null, /* explicit= */ false); AttributeContainer container = mutable.freeze(); // Nulls don't make it into the frozen representation. @@ -206,4 +206,19 @@ container.getRawAttributeValues().set(0, "foo"); assertThat(container.getRawAttributeValues()).containsExactly("hi"); } + + /** Regression test for b/269593252. */ + @Test + public void boundaryOfLargeContainer() { + AttributeContainer mutable = new Mutable(128); + mutable.setAttributeValue(0, "0", /* explicit= */ true); + mutable.setAttributeValue(127, "127", /* explicit= */ true); + + AttributeContainer frozen = mutable.freeze(); + + assertThat(frozen.getAttributeValue(0)).isEqualTo("0"); + assertThat(frozen.isAttributeValueExplicitlySpecified(0)).isTrue(); + assertThat(frozen.getAttributeValue(127)).isEqualTo("127"); + assertThat(frozen.isAttributeValueExplicitlySpecified(127)).isTrue(); + } }