[Clang][AST] Fix __has_unique_object_representations computation for unnamed bitfields.

[royjacobson]

Summary

As pointed out in https://github.com/llvm/llvm-project/issues/61336, objects with
unnamed bitfields aren't be uniquely representable.

[royjacobson]

Change to a simpler fix.

[Endill]

LGTM
I got confused at first why this fix is done in a function named getSubobjectSizeInBits, and why do we calculate size to produce a boolean value in the end. Then I realized that this function returns number of bits of value representation, or std::nullopt if there are any padding bits, and we take advantage of the latter to implement hasUniqueObjectRepresentations. I guess I'm either missing some context or that function could be named better (not in this patch, of course).

[royjacobson]

Handle 0-length unnamed bit fields as well.

[aaron.ballman]

clang/lib/AST/ASTContext.cpp

2822

I think the check for a non-zero bit-field width might not be completely correct in cases where it's used to split an allocation unit. I suggested a test case below.

clang/test/SemaCXX/type-traits.cpp

2897–2900

I think there's one more test to add:

struct UnnamedEmptyBitfieldSplit {
  short named;
  int : 0;
  short also_named;
};
static_assert(sizeof(UnnamedEmptyBitfieldSplit) != (sizeof(short) * 2));
static_assert(!has_unique_object_representations<UnnamedEmptyBitfieldSplit>::value, "Bitfield padding");

[royjacobson]

Add a test case, slightly simpler modeling.

[royjacobson]

clang/lib/AST/ASTContext.cpp

2822

That was actually fine: the padding bits would not be a part of the unnamed bit field so they're detected.

Then I realized that the surrounding code just sums the object representation bits and checks if that equals sizeof(T). But unnamed fields don't contribute any bits to the object representation. So a simpler way to fix this is to return 0 unconditionally.

[shafik]

clang/test/SemaCXX/type-traits.cpp

2897–2900

Do we also want to check packed structs as well?

[royjacobson]

clang/test/SemaCXX/type-traits.cpp

2897–2900

Do you have a test case where it would matter? Apparently packed is specified to ignore zero width bit fields.

[royjacobson]

clang/test/SemaCXX/type-traits.cpp

2897–2900

small ping :) @shafik

[shafik]

clang/test/SemaCXX/type-traits.cpp

2897–2900

Apologies, I just realized you thought I meant another zero sized bit-field case whereas I meant just test packed in general w/ non-zero sized bit-fields. The result should match for unpacked but it would be good to verify considering we seem to always get bitten by cases we don't cover.

[royjacobson]

Add some ((packed)) tests

[shafik]

LGTM, thank for adding the tests!

clang/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Fix crash on invalid code when looking up a destructor in a templated class
   inside a namespace.
   (`#59446 <https://github.com/llvm/llvm-project/issues/59446>`_)
 - Fix crash when evaluating consteval constructor of derived class whose base
   has more than one field.
   (`#60166 <https://github.com/llvm/llvm-project/issues/60166>`_)
+- Fix bug in the computation of the ``__has_unique_object_representations``
+  builtin for types with unnamed bitfields.
+  (`#61336 <https://github.com/llvm/llvm-project/issues/61336>`_)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Bug Fixes
 ^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Clang Crashes Fixed

clang/lib/AST/ASTContext.cpp

getSubobjectSizeInBits(const FieldDecl *Field, const ASTContext &Context) {
   bool IsBitIntType = Field->getType()->isBitIntType();
   if (!Field->getType()->isReferenceType() && !IsBitIntType &&
       !Context.hasUniqueObjectRepresentations(Field->getType()))
     return std::nullopt;
 
   int64_t FieldSizeInBits =
       Context.toBits(Context.getTypeSizeInChars(Field->getType()));
   if (Field->isBitField()) {
+    // If we have explicit padding bits, they don't contribute bits
+    // to the actual object representation, so return 0.
+    if (Field->isUnnamedBitfield())
+      return 0;
+
     int64_t BitfieldSize = Field->getBitWidthValue(Context);
     if (IsBitIntType) {
       if ((unsigned)BitfieldSize >
           cast<BitIntType>(Field->getType())->getNumBits())
         return std::nullopt;
     } else if (BitfieldSize > FieldSizeInBits) {
       return std::nullopt;
     }
     FieldSizeInBits = BitfieldSize;
   } else if (IsBitIntType &&
              !Context.hasUniqueObjectRepresentations(Field->getType())) {
     return std::nullopt;
   }

clang/test/SemaCXX/type-traits.cpp

 };
 union __attribute__((aligned(16))) WeirdAlignmentUnion {
   int i;
 };
 static_assert(!has_unique_object_representations<WeirdAlignment>::value, "Alignment causes padding");
 static_assert(!has_unique_object_representations<WeirdAlignmentUnion>::value, "Alignment causes padding");
 static_assert(!has_unique_object_representations<WeirdAlignment[42]>::value, "Also no arrays that have padding");
 
+struct __attribute__((packed)) PackedNoPadding1 {
+  short i;
+  int j;
+};
+struct __attribute__((packed)) PackedNoPadding2 {
+  int j;
+  short i;
+};
+static_assert(has_unique_object_representations<PackedNoPadding1>::value, "Packed structs have no padding");
+static_assert(has_unique_object_representations<PackedNoPadding2>::value, "Packed structs have no padding");
+
 static_assert(!has_unique_object_representations<int(int)>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) const>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) volatile>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) const volatile>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) &>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) const &>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) volatile &>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) const volatile &>::value, "Functions are not unique");

 };
 
 struct AlignedPaddedBitfield {
   char c : 6;
   __attribute__((aligned(1)))
   char d : 2;
 };
 
+struct UnnamedBitfield {
+  int named : 8;
+  int : 24;
+};
+
+struct __attribute__((packed)) UnnamedBitfieldPacked {
+  int named : 8;
+  int : 24;
+};
+
+struct UnnamedEmptyBitfield {
+  int named;
+  int : 0;
+};
+
+struct UnnamedEmptyBitfieldSplit {
+  short named;
+  int : 0;
+  short also_named;
+};
+
 static_assert(!has_unique_object_representations<PaddedBitfield>::value, "Bitfield padding");
 static_assert(has_unique_object_representations<UnPaddedBitfield>::value, "Bitfield padding");
 static_assert(!has_unique_object_representations<AlignedPaddedBitfield>::value, "Bitfield padding");
+static_assert(!has_unique_object_representations<UnnamedBitfield>::value, "Bitfield padding");
+static_assert(!has_unique_object_representations<UnnamedBitfieldPacked>::value, "Bitfield padding");
+static_assert(has_unique_object_representations<UnnamedEmptyBitfield>::value, "Bitfield padding");
+static_assert(sizeof(UnnamedEmptyBitfieldSplit) != (sizeof(short) * 2), "Wrong size");
+static_assert(!has_unique_object_representations<UnnamedEmptyBitfieldSplit>::value, "Bitfield padding");
 
 struct BoolBitfield {
   bool b : 8;
 };
 
 static_assert(has_unique_object_representations<BoolBitfield>::value, "Bitfield bool");
 
 struct BoolBitfield2 {