Fix __has_unique_object_representations with no_unique_address

[gbencze]

Summary

Fix incorrect behavior of __has_unique_object_representations when using the no_unique_address attribute.

Based on the bug report: https://bugs.llvm.org/show_bug.cgi?id=47722

[rsmith]

This looks fine as far as it goes, but it doesn't fix all cases of incorrect behavior of __has_unique_object_representations due to [[no_unique_address]]. Feel free to either to land this as-is and leave the other case to a separate patch, or fix it as a revision of this same change. I would expect we'll want to unify the code paths for base classes and non-static data members, which might be better done all at once.

clang/lib/AST/ASTContext.cpp

2580–2581

We need to do this for non-empty [[no_unique_address]] members of class type too, to handle tail padding reuse in cases such as:

struct A {
    ~A();
    int a;
    char b;
};
struct B {
    [[no_unique_address]] A a;
    char c[3];
};
static_assert(sizeof(B) == 8, "");
static_assert(__has_unique_object_representations(B), "");

[aaron.ballman]

clang/lib/AST/ASTContext.cpp

2608

You can drop the else after this sorta-return if you'd like.

[gbencze]

clang/lib/AST/ASTContext.cpp

2580–2581

You're right, I missed the case of reusing tail padding. I'll try to update this review to include this soon.

But I don't think that this exact example is incorrect now as A is not trivially copyable due to the user defined destructor.
It should return true when the class type member is trivially copyable but non-standard-layout though:

struct A {
private:
    int a;
public:
    char b;
};
struct B {
    [[no_unique_address]] A a;
    char c[3];
};
static_assert(sizeof(B) == 8, "");
static_assert(__has_unique_object_representations(B), "");

[gbencze]

Sorry for the slow update on this.
I fixed the behavior when reusing tail padding as mentioned by @rsmith and took a shot at unifying the code paths for base classes and fields. Let me know your thoughts on the approach!

[gbencze]

clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp

2

Just to be sure: is the specifying the triple here enough to ensure that this always uses the Itanium ABI? I believe MSVC currently ignores the no_unique_address attribute. Or do I need to add some more flags?
Alternatively, the static_asserts could be modified to check sizeof(T) > [expected size with Itanium ABI] || __has_unique_object_representations(T)

[gbencze]

Pinging @rsmith - could you please check the updates to this patch? Thanks!

[gbencze]

Ping @rsmith

[steakhal]

It looks great.

clang/lib/AST/ASTContext.cpp

2575–2577

Why do you return None here?

2588–2591

Why is this a template?
Can't you just take the field_range?

By the same token, structSubobjectsHaveUniqueObjectRepresentations returns an optional int. I'm confused.

clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp

2

I think it's better to specify the triple exactly

43

Why is this outside of the namespace declaration?

[gbencze]

clang/lib/AST/ASTContext.cpp

2575–2577

These bits were just extracted from structHasUniqueObjectRepresentations, the underlying logic was not changed here. (originally in line 2615)
I believe this handles the case "If the width of a bit-field is larger than the width of the bit-field's type (or, in case of an enumeration type, of its underlying type), the extra bits are padding bits" from [class.bit]

2588–2591

This function is also called with (non virtual) base class subobjects, not just fields.
A None return value indicates that the subobjects do not have unique object representations, otherwise the size of the subobjects is returned. This allows us to detect for example padding between a base class and the first field.

clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp

43

Tbh I'm not sure. I can move it in the namespace if you think it'd be better there.

[steakhal]

I'm adding a couple of reviewers to get this going.

[whisperity]

Adding a few subscribers I could find from the original bugzilla entry, + perhaps a few reviewers to help us.

[whisperity]

Gentle boop. Bugzilla is offline for me right now (and rumour has it that it will be completely shut down in the near future), so it would be good to know what is the status of this patch and how it related to the mentioned (and right now unavailable) bug report, as it is also blocking a Clang-Tidy check.

[aaron.ballman]

LGTM!

clang/lib/AST/ASTContext.cpp

2563–2564

clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp

43

No reason not to.

clang/lib/AST/ASTContext.cpp

static bool isStructEmpty(QualType Ty) {
   if (!RD->field_empty())
     return false;
 
   if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RD))
     return ClassDecl->isEmpty();
 
   return true;
 }
 
 static llvm::Optional<int64_t>
 structHasUniqueObjectRepresentations(const ASTContext &Context,
                                      const RecordDecl *RD) {
   assert(!RD->isUnion() && "Must be struct/class type");
   const auto &Layout = Context.getASTRecordLayout(RD);
 
   int64_t CurOffsetInBits = 0;
   if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RD)) {
     if (ClassDecl->isDynamicClass())
       return llvm::None;
 
     SmallVector<std::pair<QualType, int64_t>, 4> Bases;
     for (const auto &Base : ClassDecl->bases()) {
       // Empty types can be inherited from, and non-empty types can potentially
       // have tail padding, so just make sure there isn't an error.
       if (!isStructEmpty(Base.getType())) {
         llvm::Optional<int64_t> Size = structHasUniqueObjectRepresentations(
             Context, Base.getType()->castAs<RecordType>()->getDecl());
         if (!Size)
           return llvm::None;
         Bases.emplace_back(Base.getType(), Size.getValue());
       }
     }
 
     llvm::sort(Bases, [&](const std::pair<QualType, int64_t> &L,
                           const std::pair<QualType, int64_t> &R) {
       return Layout.getBaseClassOffset(L.first->getAsCXXRecordDecl()) <
              Layout.getBaseClassOffset(R.first->getAsCXXRecordDecl());
     });
 
     for (const auto &Base : Bases) {
       int64_t BaseOffset = Context.toBits(
           Layout.getBaseClassOffset(Base.first->getAsCXXRecordDecl()));
       int64_t BaseSize = Base.second;
       if (BaseOffset != CurOffsetInBits)
         return llvm::None;
       CurOffsetInBits = BaseOffset + BaseSize;
     }
   }
 
   for (const auto *Field : RD->fields()) {
-    if (!Field->getType()->isReferenceType() &&
-        !Context.hasUniqueObjectRepresentations(Field->getType()))
+    if (!Field->getType()->isReferenceType()) {
+      if (Field->isZeroSize(Context))
+        continue;
+      else if (!Context.hasUniqueObjectRepresentations(Field->getType()))
         return llvm::None;
+    }
 
     int64_t FieldSizeInBits =
         Context.toBits(Context.getTypeSizeInChars(Field->getType()));
     if (Field->isBitField()) {
       int64_t BitfieldSize = Field->getBitWidthValue(Context);
 
       if (BitfieldSize > FieldSizeInBits)
         return llvm::None;

clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -std=c++2a %s
+//  expected-no-diagnostics
+
+struct Empty {};
+
+struct A {
+  [[no_unique_address]] Empty e;
+  char x;
+};
+
+static_assert(__has_unique_object_representations(A));
+
+struct B {
+  char x;
+  [[no_unique_address]] Empty e;
+};
+
+static_assert(__has_unique_object_representations(B));
+
+struct C {
+  char x;
+  [[no_unique_address]] Empty e1;
+  [[no_unique_address]] Empty e2;
+};
+
+static_assert(!__has_unique_object_representations(C));