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

2628–2629

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

2638

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

[gbencze]

clang/lib/AST/ASTContext.cpp

2628–2629

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

1

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

2582–2584

Why do you return None here?

2595–2598

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

1

I think it's better to specify the triple exactly

42

Why is this outside of the namespace declaration?

[gbencze]

clang/lib/AST/ASTContext.cpp

2582–2584

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]

2595–2598

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

42

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

2570–2571

clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp

42

No reason not to.

clang/lib/AST/ASTContext.cpp

if (!Context.hasUniqueObjectRepresentations(Field->getType()))
       return false;
     CharUnits FieldSize = Context.getTypeSizeInChars(Field->getType());
     if (FieldSize != UnionSize)
       return false;
   }
   return !RD->field_empty();
 }
 
-static bool isStructEmpty(QualType Ty) {
-  const RecordDecl *RD = Ty->castAs<RecordType>()->getDecl();
+static int64_t getSubobjectOffset(const FieldDecl *Field,
+                                  const ASTContext &Context,
+                                  const clang::ASTRecordLayout & /*Layout*/) {
+  return Context.getFieldOffset(Field);
+}
 
-  if (!RD->field_empty())
-    return false;
+static int64_t getSubobjectOffset(const CXXRecordDecl *RD,
+                                  const ASTContext &Context,
+                                  const clang::ASTRecordLayout &Layout) {
+  return Context.toBits(Layout.getBaseClassOffset(RD));
+}
 
-  if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RD))
-    return ClassDecl->isEmpty();
+static llvm::Optional<int64_t>
+structHasUniqueObjectRepresentations(const ASTContext &Context,
+                                     const RecordDecl *RD);
 
-  return true;
+static llvm::Optional<int64_t>
+getSubobjectSizeInBits(const FieldDecl *Field, const ASTContext &Context) {
+  if (Field->getType()->isRecordType()) {
+    const RecordDecl *RD = Field->getType()->getAsRecordDecl();
+    if (!RD->isUnion())
+      return structHasUniqueObjectRepresentations(Context, RD);
+  }
+  if (!Field->getType()->isReferenceType() &&
+      !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;
+    FieldSizeInBits = BitfieldSize;
+  }
+  return FieldSizeInBits;
+}
+
+static llvm::Optional<int64_t>
+getSubobjectSizeInBits(const CXXRecordDecl *RD, const ASTContext &Context) {
+  return structHasUniqueObjectRepresentations(Context, RD);
+}
+
+template <typename RangeT>
+static llvm::Optional<int64_t> structSubobjectsHaveUniqueObjectRepresentations(
+    const RangeT &Subobjects, int64_t CurOffsetInBits, const ASTContext &Context,
+    const clang::ASTRecordLayout &Layout) {
+  for (const auto *Subobject : Subobjects) {
+    llvm::Optional<int64_t> SizeInBits =
+        getSubobjectSizeInBits(Subobject, Context);
+    if (!SizeInBits)
+      return llvm::None;
+    if (*SizeInBits != 0) {
+      int64_t Offset = getSubobjectOffset(Subobject, Context, Layout);
+      if (Offset != CurOffsetInBits)
+        return llvm::None;
+      CurOffsetInBits += *SizeInBits;
+    }
+  }
+  return CurOffsetInBits;
 }
 
 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;
+    SmallVector<CXXRecordDecl*, 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());
-      }
+      Bases.emplace_back(Base.getType()->getAsCXXRecordDecl ());
     }
 
-    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());
+    llvm::sort(Bases, [&](const CXXRecordDecl *L, const CXXRecordDecl *R) {
+      return Layout.getBaseClassOffset(L) < Layout.getBaseClassOffset(R);
     });
 
-    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;
-    }
+    llvm::Optional<int64_t> OffsetAfterBases =
+        structSubobjectsHaveUniqueObjectRepresentations(Bases, CurOffsetInBits,
+                                                        Context, Layout);
+    if (!OffsetAfterBases)
+    CurOffsetInBits = *OffsetAfterBases;
   }
 
-  for (const auto *Field : RD->fields()) {
-    if (!Field->getType()->isReferenceType() &&
-        !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;
-      FieldSizeInBits = BitfieldSize;
-    }
-
-    int64_t FieldOffsetInBits = Context.getFieldOffset(Field);
-
-    if (FieldOffsetInBits != CurOffsetInBits)
-      return llvm::None;
-
-    CurOffsetInBits = FieldSizeInBits + FieldOffsetInBits;
-  }
+  llvm::Optional<int64_t> OffsetAfterFields =
+      structSubobjectsHaveUniqueObjectRepresentations(
+          RD->fields(), CurOffsetInBits, Context, Layout);
+  if (!OffsetAfterFields)
+  CurOffsetInBits = *OffsetAfterFields;
 
   return CurOffsetInBits;
 }
 
 bool ASTContext::hasUniqueObjectRepresentations(QualType Ty) const {
   // C++17 [meta.unary.prop]:
   //   The predicate condition for a template specialization
   //   has_unique_object_representations<T> shall be

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));
+
+namespace TailPaddingReuse {
+struct A {
+private:
+  int a;
+
+public:
+  char b;
+};
+
+struct B {
+  [[no_unique_address]] A a;
+  char c[3];
+};
+} // namespace TailPaddingReuse
+static_assert(__has_unique_object_representations(TailPaddingReuse::B));