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

Author: justinhorvitz

src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java

@@ -321,25 +321,32 @@ static final class Large extends Frozen {
     //  - 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 @@ private Large(Object[] attrValues, BitSet explicitAttrs) {
           continue;
         }
         if (explicitAttrs.get(attrIndex)) {
-          setBit(state, attrIndex);
+          setExplicitBit(state, attrIndex);
         }
         state[index + p] = (byte) attrIndex;
         values[index] = attrValue;
@@ -381,7 +388,7 @@ private Large(Object[] attrValues, BitSet explicitAttrs) {
      */
     @Override
     boolean isAttributeValueExplicitlySpecified(int attrIndex) {
-      return (attrIndex >= 0) && getBit(state, attrIndex);
+      return (attrIndex >= 0) && getExplicitBit(state, attrIndex);
     }
 
     @Nullable

src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java

@@ -196,8 +196,8 @@ public void testMutableGetRawAttributeValuesReturnsNullSafeCopy() {
   @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 @@ public void testGetRawAttributeValuesReturnsCopy() {
     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();
+  }
 }