[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

2817

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

2886–2889

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

2817

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

2886–2889

Do we also want to check packed structs as well?

[royjacobson]

clang/test/SemaCXX/type-traits.cpp

2886–2889

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

2886–2889

small ping :) @shafik

[shafik]

clang/test/SemaCXX/type-traits.cpp

2886–2889

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) {
 
   int64_t FieldSizeInBits =
       Context.toBits(Context.getTypeSizeInChars(Field->getType()));
   if (Field->isBitField()) {
     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;
   }
   return FieldSizeInBits;
 }
 
 static std::optional<int64_t>
 getSubobjectSizeInBits(const CXXRecordDecl *RD, const ASTContext &Context) {
   return structHasUniqueObjectRepresentations(Context, RD);
 }
 
-template <typename RangeT>
+template <bool CheckUnnamedBitFields, typename RangeT>
 static std::optional<int64_t> structSubobjectsHaveUniqueObjectRepresentations(
     const RangeT &Subobjects, int64_t CurOffsetInBits,
     const ASTContext &Context, const clang::ASTRecordLayout &Layout) {
   for (const auto *Subobject : Subobjects) {
     std::optional<int64_t> SizeInBits =
         getSubobjectSizeInBits(Subobject, Context);
+    if constexpr (CheckUnnamedBitFields) {
+      if (Subobject->isUnnamedBitfield())
+        return std::nullopt;
+    }
     if (!SizeInBits)
       return std::nullopt;
     if (*SizeInBits != 0) {
       int64_t Offset = getSubobjectOffset(Subobject, Context, Layout);
       if (Offset != CurOffsetInBits)
         return std::nullopt;
       CurOffsetInBits += *SizeInBits;
     }
for (const auto &Base : ClassDecl->bases()) {
       Bases.emplace_back(Base.getType()->getAsCXXRecordDecl());
     }
 
     llvm::sort(Bases, [&](const CXXRecordDecl *L, const CXXRecordDecl *R) {
       return Layout.getBaseClassOffset(L) < Layout.getBaseClassOffset(R);
     });
 
     std::optional<int64_t> OffsetAfterBases =
-        structSubobjectsHaveUniqueObjectRepresentations(Bases, CurOffsetInBits,
-                                                        Context, Layout);
+        structSubobjectsHaveUniqueObjectRepresentations<false>(
+            Bases, CurOffsetInBits, Context, Layout);
     if (!OffsetAfterBases)
       return std::nullopt;
     CurOffsetInBits = *OffsetAfterBases;
   }
 
   std::optional<int64_t> OffsetAfterFields =
-      structSubobjectsHaveUniqueObjectRepresentations(
+      structSubobjectsHaveUniqueObjectRepresentations<true>(
           RD->fields(), CurOffsetInBits, Context, Layout);
   if (!OffsetAfterFields)
     return std::nullopt;
   CurOffsetInBits = *OffsetAfterFields;
 
   return CurOffsetInBits;
 }
 

clang/test/SemaCXX/type-traits.cpp

 };
 
 struct AlignedPaddedBitfield {
   char c : 6;
   __attribute__((aligned(1)))
   char d : 2;
 };
 
+struct UnnamedBitfield {
+  int named : 8;
+  int : 24;
+};
+
 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");
 
 struct BoolBitfield {
   bool b : 8;
 };
 
 static_assert(has_unique_object_representations<BoolBitfield>::value, "Bitfield bool");
 
 struct BoolBitfield2 {