Use consistent storage type in bit structs.

Propagate the storage type from the enclosing union to each
individual field when it is defined by provided macros.

Test: m test-art-host-gtest
Bug: 148125232
Change-Id: I6976c4e7668d62f500ee1a413a30e02bc8b68cea

4 files changed, 74 insertions, 55 deletions

diff --git a/libartbase/base/bit_struct.h b/libartbase/base/bit_struct.h
index 277e4b52e3..eca8780568 100644
--- a/libartbase/base/bit_struct.h
+++ b/libartbase/base/bit_struct.h
@@ -32,9 +32,9 @@
 //
 //   // Definition for type 'Example'
 //   BITSTRUCT_DEFINE_START(Example, 10)
-//     BitStructUint<0, 2> u2;     // Every field must be a BitStruct[*].
-//     BitStructInt<2, 7>  i7;
-//     BitStructUint<9, 1> i1;
+//     BITSTRUCT_UINT(0, 2) u2;     // Every field must be a BitStruct[*] with the same StorageType,
+//     BITSTRUCT_INT(2, 7)  i7;     // preferably using BITSTRUCT_{FIELD,UINT,INT}
+//     BITSTRUCT_UINT(9, 1) i1;     // to fill in the StorageType parameter.
 //   BITSTRUCT_DEFINE_END(Example);
 //
 //  Would define a bit struct with this layout:
@@ -109,8 +109,8 @@ namespace art {
 // of T can be represented by kBitWidth.
 template <typename T,
           size_t kBitOffset,
-          size_t kBitWidth = BitStructSizeOf<T>(),
-          typename StorageType = typename detail::MinimumTypeUnsignedHelper<kBitOffset + kBitWidth>::type>
+          size_t kBitWidth,
+          typename StorageType>
 struct BitStructField {
   static_assert(std::is_standard_layout<T>::value, "T must be standard layout");
 
@@ -186,10 +186,8 @@ struct BitStructField {
 //
 // (Common usage should be BitStructInt, BitStructUint -- this
 // intermediate template allows a user-defined integer to be used.)
-template <typename T, size_t kBitOffset, size_t kBitWidth>
-struct BitStructNumber : public BitStructField<T, kBitOffset, kBitWidth, /*StorageType*/T> {
-  using StorageType = T;
-
+template <typename T, size_t kBitOffset, size_t kBitWidth, typename StorageType>
+struct BitStructNumber : public BitStructField<T, kBitOffset, kBitWidth, StorageType> {
   BitStructNumber& operator=(T value) {
     return BaseType::Assign(*this, value);
   }
@@ -221,7 +219,7 @@ struct BitStructNumber : public BitStructField<T, kBitOffset, kBitWidth, /*Stora
   }
 
  private:
-  using BaseType = BitStructField<T, kBitOffset, kBitWidth, /*StorageType*/T>;
+  using BaseType = BitStructField<T, kBitOffset, kBitWidth, StorageType>;
   using BaseType::Get;
 };
 
@@ -229,21 +227,23 @@ struct BitStructNumber : public BitStructField<T, kBitOffset, kBitWidth, /*Stora
 // in order to be large enough to fit (kBitOffset + kBitWidth).
 //
 // Values are sign-extended when they are read out.
-template <size_t kBitOffset, size_t kBitWidth>
+template <size_t kBitOffset, size_t kBitWidth, typename StorageType>
 using BitStructInt =
     BitStructNumber<typename detail::MinimumTypeHelper<int, kBitOffset + kBitWidth>::type,
                     kBitOffset,
-                    kBitWidth>;
+                    kBitWidth,
+                    StorageType>;
 
 // Create a BitStruct field which uses the smallest underlying uint storage type,
 // in order to be large enough to fit (kBitOffset + kBitWidth).
 //
 // Values are zero-extended when they are read out.
-template <size_t kBitOffset, size_t kBitWidth>
+template <size_t kBitOffset, size_t kBitWidth, typename StorageType>
 using BitStructUint =
     BitStructNumber<typename detail::MinimumTypeHelper<unsigned int, kBitOffset + kBitWidth>::type,
                     kBitOffset,
-                    kBitWidth>;
+                    kBitWidth,
+                    StorageType>;
 
 // Start a definition for a bitstruct.
 // A bitstruct is defined to be a union with a common initial subsequence
@@ -260,6 +260,8 @@ using BitStructUint =
 // standard-layout struct members.
 #define BITSTRUCT_DEFINE_START(name, bitwidth)                                        \
     union name {                                                         /* NOLINT */ \
+      using StorageType =                                                             \
+          typename detail::MinimumTypeUnsignedHelper<(bitwidth)>::type;               \
       art::detail::DefineBitStructSize<(bitwidth)> _;                                 \
       static constexpr size_t BitStructSizeOf() { return (bitwidth); }                \
       name& operator=(const name& other) { _ = other._; return *this; }  /* NOLINT */ \
@@ -267,6 +269,14 @@ using BitStructUint =
       name() = default;                                                               \
       ~name() = default;
 
+// Define a field. See top of file for usage example.
+#define BITSTRUCT_FIELD(type, bit_offset, bit_width)                           \
+    BitStructField<type, (bit_offset), (bit_width), StorageType>
+#define BITSTRUCT_INT(bit_offset, bit_width)                                   \
+    BitStructInt<(bit_offset), (bit_width), StorageType>
+#define BITSTRUCT_UINT(bit_offset, bit_width)                                  \
+    BitStructUint<(bit_offset), (bit_width), StorageType>
+
 // End the definition of a bitstruct, and insert a sanity check
 // to ensure that the bitstruct did not exceed the specified size.
 //
diff --git a/libartbase/base/bit_struct_test.cc b/libartbase/base/bit_struct_test.cc
index 1874a4d6d1..6b0d0f0b92 100644
--- a/libartbase/base/bit_struct_test.cc
+++ b/libartbase/base/bit_struct_test.cc
@@ -61,33 +61,33 @@ size_t AsUint(const T& value) {
 
 struct CustomBitStruct {
   CustomBitStruct() = default;
-  explicit CustomBitStruct(int8_t data) : data(data) {}
+  explicit CustomBitStruct(uint8_t data) : data(data) {}
 
   static constexpr size_t BitStructSizeOf() {
     return 4;
   }
 
-  int8_t data;
+  uint8_t data;
 };
 
 TEST(BitStructs, Custom) {
-  CustomBitStruct expected(0b1111);
+  CustomBitStruct expected(0b1111u);
 
-  BitStructField<CustomBitStruct, /*lsb=*/4, /*width=*/4> f{};
+  BitStructField<CustomBitStruct, /*lsb=*/4, /*width=*/4, uint8_t> f{};
 
   EXPECT_EQ(1u, sizeof(f));
 
-  f = CustomBitStruct(0b1111);
+  f = CustomBitStruct(0b1111u);
 
   CustomBitStruct read_out = f;
-  EXPECT_EQ(read_out.data, 0b1111);
+  EXPECT_EQ(read_out.data, 0b1111u);
 
   EXPECT_EQ(AsUint(f), 0b11110000u);
 }
 
 BITSTRUCT_DEFINE_START(TestTwoCustom, /* size= */ 8)
-  BitStructField<CustomBitStruct, /*lsb=*/0, /*width=*/4> f4_a;
-  BitStructField<CustomBitStruct, /*lsb=*/4, /*width=*/4> f4_b;
+  BITSTRUCT_FIELD(CustomBitStruct, /*lsb=*/0, /*width=*/4) f4_a;
+  BITSTRUCT_FIELD(CustomBitStruct, /*lsb=*/4, /*width=*/4) f4_b;
 BITSTRUCT_DEFINE_END(TestTwoCustom);
 
 TEST(BitStructs, TwoCustom) {
@@ -122,7 +122,7 @@ TEST(BitStructs, TwoCustom) {
 }
 
 TEST(BitStructs, Number) {
-  BitStructNumber<uint16_t, /*lsb=*/4, /*width=*/4> bsn{};
+  BitStructNumber<uint16_t, /*lsb=*/4, /*width=*/4, uint16_t> bsn{};
   EXPECT_EQ(2u, sizeof(bsn));
 
   bsn = 0b1111;
@@ -135,25 +135,28 @@ TEST(BitStructs, Number) {
   EXPECT_EQ(AsUint(bsn), 0b11110000u);
 }
 
+TEST(BitStructs, NumberNarrowStorage) {
+  BitStructNumber<uint16_t, /*lsb=*/4, /*width=*/4, uint8_t> bsn{};
+  EXPECT_EQ(1u, sizeof(bsn));
+
+  bsn = 0b1111;
+
+  uint32_t read_out = static_cast<uint32_t>(bsn);
+  uint32_t read_out_impl = bsn;
+
+  EXPECT_EQ(read_out, read_out_impl);
+  EXPECT_EQ(read_out, 0b1111u);
+  EXPECT_EQ(AsUint(bsn), 0b11110000u);
+}
+
 BITSTRUCT_DEFINE_START(TestBitStruct, /* size= */ 8)
-  BitStructInt</*lsb=*/0, /*width=*/3> i3;
-  BitStructUint</*lsb=*/3, /*width=*/4> u4;
+  BITSTRUCT_INT(/*lsb=*/0, /*width=*/3) i3;
+  BITSTRUCT_UINT(/*lsb=*/3, /*width=*/4) u4;
 
-  BitStructUint</*lsb=*/0, /*width=*/7> alias_all;
+  BITSTRUCT_UINT(/*lsb=*/0, /*width=*/7) alias_all;
 BITSTRUCT_DEFINE_END(TestBitStruct);
 
 TEST(BitStructs, Test1) {
-  {
-    // Check minimal size selection is correct.
-    BitStructInt</*lsb=*/0, /*width=*/3> i3;
-    BitStructUint</*lsb=*/3, /*width=*/4> u4;
-
-    BitStructUint</*lsb=*/0, /*width=*/7> alias_all;
-
-    EXPECT_EQ(1u, sizeof(i3));
-    EXPECT_EQ(1u, sizeof(u4));
-    EXPECT_EQ(1u, sizeof(alias_all));
-  }
   TestBitStruct tst{};
 
   // Check minimal size selection is correct.
@@ -217,11 +220,11 @@ TEST(BitStructs, Test1) {
 }
 
 BITSTRUCT_DEFINE_START(MixedSizeBitStruct, /* size= */ 32)
-  BitStructUint</*lsb=*/0, /*width=*/3> u3;
-  BitStructUint</*lsb=*/3, /*width=*/10> u10;
-  BitStructUint</*lsb=*/13, /*width=*/19> u19;
+  BITSTRUCT_UINT(/*lsb=*/0, /*width=*/3) u3;
+  BITSTRUCT_UINT(/*lsb=*/3, /*width=*/10) u10;
+  BITSTRUCT_UINT(/*lsb=*/13, /*width=*/19) u19;
 
-  BitStructUint</*lsb=*/0, /*width=*/32> alias_all;
+  BITSTRUCT_UINT(/*lsb=*/0, /*width=*/32) alias_all;
 BITSTRUCT_DEFINE_END(MixedSizeBitStruct);
 
 // static_assert(sizeof(MixedSizeBitStruct) == sizeof(uint32_t), "TestBitStructs#MixedSize");
@@ -256,10 +259,10 @@ TEST(BitStructs, Mixed) {
 }
 
 BITSTRUCT_DEFINE_START(TestBitStruct_u8, /* size= */ 8)
-  BitStructInt</*lsb=*/0, /*width=*/3> i3;
-  BitStructUint</*lsb=*/3, /*width=*/4> u4;
+  BITSTRUCT_INT(/*lsb=*/0, /*width=*/3) i3;
+  BITSTRUCT_UINT(/*lsb=*/3, /*width=*/4) u4;
 
-  BitStructUint</*lsb=*/0, /*width=*/8> alias_all;
+  BITSTRUCT_UINT(/*lsb=*/0, /*width=*/8) alias_all;
 BITSTRUCT_DEFINE_END(TestBitStruct_u8);
 
 TEST(BitStructs, FieldAssignment) {
@@ -283,11 +286,15 @@ TEST(BitStructs, FieldAssignment) {
   }
 }
 
-BITSTRUCT_DEFINE_START(NestedStruct, /* size= */ 64)
-  BitStructField<MixedSizeBitStruct, /*lsb=*/0> mixed_lower;
-  BitStructField<MixedSizeBitStruct, /*lsb=*/32> mixed_upper;
+BITSTRUCT_DEFINE_START(NestedStruct, /* size= */ 2 * MixedSizeBitStruct::BitStructSizeOf())
+  BITSTRUCT_FIELD(MixedSizeBitStruct,
+                  /*lsb=*/0,
+                  /*width=*/MixedSizeBitStruct::BitStructSizeOf()) mixed_lower;
+  BITSTRUCT_FIELD(MixedSizeBitStruct,
+                  /*lsb=*/MixedSizeBitStruct::BitStructSizeOf(),
+                  /*width=*/MixedSizeBitStruct::BitStructSizeOf()) mixed_upper;
 
-  BitStructUint</*lsb=*/0, /*width=*/64> alias_all;
+  BITSTRUCT_UINT(/*lsb=*/0, /*width=*/ 2 * MixedSizeBitStruct::BitStructSizeOf()) alias_all;
 BITSTRUCT_DEFINE_END(NestedStruct);
 
 TEST(BitStructs, DISABLED_NestedFieldAssignment) {
diff --git a/runtime/subtype_check_bits.h b/runtime/subtype_check_bits.h
index 23d8ac371e..7e73afb295 100644
--- a/runtime/subtype_check_bits.h
+++ b/runtime/subtype_check_bits.h
@@ -57,8 +57,8 @@ namespace art {
  * See subtype_check.h and subtype_check_info.h for more details.
  */
 BITSTRUCT_DEFINE_START(SubtypeCheckBits, /*size=*/ BitString::BitStructSizeOf() + 1u)
-  BitStructField<BitString, /*lsb=*/ 0> bitstring_;
-  BitStructUint</*lsb=*/ BitString::BitStructSizeOf(), /*width=*/ 1> overflow_;
+  BITSTRUCT_FIELD(BitString, /*lsb=*/ 0, /*width=*/ BitString::BitStructSizeOf()) bitstring_;
+  BITSTRUCT_UINT(/*lsb=*/ BitString::BitStructSizeOf(), /*width=*/ 1) overflow_;
 BITSTRUCT_DEFINE_END(SubtypeCheckBits);
 
 }  // namespace art
diff --git a/runtime/subtype_check_bits_and_status.h b/runtime/subtype_check_bits_and_status.h
index eec6e21832..e774955245 100644
--- a/runtime/subtype_check_bits_and_status.h
+++ b/runtime/subtype_check_bits_and_status.h
@@ -68,11 +68,13 @@ static constexpr size_t NonNumericBitSizeOf() {
 static constexpr size_t kClassStatusBitSize = MinimumBitsToStore(enum_cast<>(ClassStatus::kLast));
 static_assert(kClassStatusBitSize == 4u, "ClassStatus should need 4 bits.");
 BITSTRUCT_DEFINE_START(SubtypeCheckBitsAndStatus, BitSizeOf<BitString::StorageType>())
-  BitStructField<SubtypeCheckBits, /*lsb=*/ 0> subtype_check_info_;
-  BitStructField<ClassStatus,
-                 /*lsb=*/ SubtypeCheckBits::BitStructSizeOf(),
-                 /*width=*/ kClassStatusBitSize> status_;
-  BitStructInt</*lsb=*/ 0, /*width=*/ BitSizeOf<BitString::StorageType>()> int32_alias_;
+  BITSTRUCT_FIELD(SubtypeCheckBits,
+                  /*lsb=*/ 0,
+                  /*width=*/ SubtypeCheckBits::BitStructSizeOf()) subtype_check_info_;
+  BITSTRUCT_FIELD(ClassStatus,
+                  /*lsb=*/ SubtypeCheckBits::BitStructSizeOf(),
+                  /*width=*/ kClassStatusBitSize) status_;
+  BITSTRUCT_INT(/*lsb=*/ 0, /*width=*/ BitSizeOf<BitString::StorageType>()) int32_alias_;
 BITSTRUCT_DEFINE_END(SubtypeCheckBitsAndStatus);
 
 // Use the spare alignment from "ClassStatus" to store all the new SubtypeCheckInfo data.