[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Balázs Benics via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGad59735f9d15: Fix __has_unique_object_representations with 
no_unique_address (authored by gbencze, committed by steakhal).

Changed prior to commit:
  https://reviews.llvm.org/D89649?vs=303082=368823#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp

Index: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
@@ -0,0 +1,42 @@
+// 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));
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -2633,16 +2633,66 @@
   return !RD->field_empty();
 }
 
-static bool isStructEmpty(QualType Ty) {
-  const RecordDecl *RD = Ty->castAs()->getDecl();
+static int64_t getSubobjectOffset(const FieldDecl *Field,
+  const ASTContext ,
+  const clang::ASTRecordLayout & /*Layout*/) {
+  return Context.getFieldOffset(Field);
+}
 
-  if (!RD->field_empty())
-return false;
+static int64_t getSubobjectOffset(const CXXRecordDecl *RD,
+  const ASTContext ,
+  const clang::ASTRecordLayout ) {
+  return Context.toBits(Layout.getBaseClassOffset(RD));
+}
 
-  if (const auto *ClassDecl = dyn_cast(RD))
-return ClassDecl->isEmpty();
+static llvm::Optional
+structHasUniqueObjectRepresentations(const ASTContext ,
+ const RecordDecl *RD);
 
-  return true;
+static llvm::Optional
+getSubobjectSizeInBits(const FieldDecl *Field, const ASTContext ) {
+  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
+getSubobjectSizeInBits(const CXXRecordDecl *RD, const ASTContext ) {
+  return structHasUniqueObjectRepresentations(Context, RD);
+}
+
+template 
+static llvm::Optional structSubobjectsHaveUniqueObjectRepresentations(
+const RangeT , int64_t CurOffsetInBits,
+const ASTContext , const clang::ASTRecordLayout ) {
+  for (const auto *Subobject : Subobjects) {
+llvm::Optional 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
@@ -2656,58 +2706,32 @@
 if (ClassDecl->isDynamicClass())
   return llvm::None;
 
-SmallVector, 4> Bases;
+SmallVector Bases;
 for (const auto  : 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 Size = structHasUniqueObjectRepresentations(
-Context, Base.getType()->castAs()->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 ,
-  

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!




Comment at: clang/lib/AST/ASTContext.cpp:2570-2571
+getSubobjectSizeInBits(const FieldDecl *Field, const ASTContext ) {
+  if (Field->getType()->isRecordType()) {
+const RecordDecl *RD = Field->getType()->getAsRecordDecl();
+if (!RD->isUnion())





Comment at: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp:42
+} // namespace TailPaddingReuse
+static_assert(__has_unique_object_representations(TailPaddingReuse::B));

gbencze wrote:
> steakhal wrote:
> > Why is this outside of the namespace declaration?
> Tbh I'm not sure. I can move it in the namespace if you think it'd be better 
> there. 
No reason not to.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

//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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Whisperity via Phabricator via cfe-commits]

whisperity added subscribers: dblaikie, erik.pilkington, martong, whisperity.
whisperity added a reviewer: shafik.
whisperity added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Balázs Benics via Phabricator via cfe-commits]

steakhal added reviewers: erichkeane, rnk, majnemer, EricWF.
steakhal added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2582-2584
+int64_t BitfieldSize = Field->getBitWidthValue(Context);
+if (BitfieldSize > FieldSizeInBits)
+  return llvm::None;

steakhal wrote:
> Why do you return `None` here?
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]



Comment at: clang/lib/AST/ASTContext.cpp:2595-2598
+template 
+static llvm::Optional structSubobjectsHaveUniqueObjectRepresentations(
+const RangeT , int64_t CurOffsetInBits, const ASTContext 
,
+const clang::ASTRecordLayout ) {

steakhal wrote:
> 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.
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. 



Comment at: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp:42
+} // namespace TailPaddingReuse
+static_assert(__has_unique_object_representations(TailPaddingReuse::B));

steakhal wrote:
> Why is this outside of the namespace declaration?
Tbh I'm not sure. I can move it in the namespace if you think it'd be better 
there. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

It looks great.




Comment at: clang/lib/AST/ASTContext.cpp:2582-2584
+int64_t BitfieldSize = Field->getBitWidthValue(Context);
+if (BitfieldSize > FieldSizeInBits)
+  return llvm::None;

Why do you return `None` here?



Comment at: clang/lib/AST/ASTContext.cpp:2595-2598
+template 
+static llvm::Optional structSubobjectsHaveUniqueObjectRepresentations(
+const RangeT , int64_t CurOffsetInBits, const ASTContext 
,
+const clang::ASTRecordLayout ) {

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.



Comment at: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify 
-std=c++2a %s
+//  expected-no-diagnostics

gbencze wrote:
> 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)`
I think it's better to specify the triple exactly 



Comment at: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp:42
+} // namespace TailPaddingReuse
+static_assert(__has_unique_object_representations(TailPaddingReuse::B));

Why is this outside of the namespace declaration?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

Ping @rsmith


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added inline comments.



Comment at: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify 
-std=c++2a %s
+//  expected-no-diagnostics

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)`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 303082.
gbencze added a comment.
Herald added a subscriber: mgrang.

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!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp

Index: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
@@ -0,0 +1,42 @@
+// 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));
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -2549,16 +2549,66 @@
   return !RD->field_empty();
 }
 
-static bool isStructEmpty(QualType Ty) {
-  const RecordDecl *RD = Ty->castAs()->getDecl();
+static int64_t getSubobjectOffset(const FieldDecl *Field,
+  const ASTContext ,
+  const clang::ASTRecordLayout & /*Layout*/) {
+  return Context.getFieldOffset(Field);
+}
 
-  if (!RD->field_empty())
-return false;
+static int64_t getSubobjectOffset(const CXXRecordDecl *RD,
+  const ASTContext ,
+  const clang::ASTRecordLayout ) {
+  return Context.toBits(Layout.getBaseClassOffset(RD));
+}
 
-  if (const auto *ClassDecl = dyn_cast(RD))
-return ClassDecl->isEmpty();
+static llvm::Optional
+structHasUniqueObjectRepresentations(const ASTContext ,
+ const RecordDecl *RD);
 
-  return true;
+static llvm::Optional
+getSubobjectSizeInBits(const FieldDecl *Field, const ASTContext ) {
+  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
+getSubobjectSizeInBits(const CXXRecordDecl *RD, const ASTContext ) {
+  return structHasUniqueObjectRepresentations(Context, RD);
+}
+
+template 
+static llvm::Optional structSubobjectsHaveUniqueObjectRepresentations(
+const RangeT , int64_t CurOffsetInBits, const ASTContext ,
+const clang::ASTRecordLayout ) {
+  for (const auto *Subobject : Subobjects) {
+llvm::Optional 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
@@ -2572,58 +2622,32 @@
 if (ClassDecl->isDynamicClass())
   return llvm::None;
 
-SmallVector, 4> Bases;
+SmallVector Bases;
 for (const auto  : 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 Size = structHasUniqueObjectRepresentations(
-Context, Base.getType()->castAs()->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 ,
-  const 

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2580-2581
   if (!isStructEmpty(Base.getType())) {
 llvm::Optional Size = structHasUniqueObjectRepresentations(
 Context, Base.getType()->castAs()->getDecl());
 if (!Size)

rsmith wrote:
> 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), "");
> ```
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), "");
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2608
+continue;
+  else if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+return llvm::None;

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

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.




Comment at: clang/lib/AST/ASTContext.cpp:2580-2581
   if (!isStructEmpty(Base.getType())) {
 llvm::Optional Size = structHasUniqueObjectRepresentations(
 Context, Base.getType()->castAs()->getDecl());
 if (!Size)

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), "");
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze created this revision.
gbencze added reviewers: rsmith, aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
gbencze requested review of this revision.

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 



Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89649

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp


Index: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
@@ -0,0 +1,26 @@
+// 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));
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -2602,9 +2602,12 @@
   }
 
   for (const auto *Field : RD->fields()) {
-if (!Field->getType()->isReferenceType() &&
-!Context.hasUniqueObjectRepresentations(Field->getType()))
-  return llvm::None;
+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()));


Index: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
@@ -0,0 +1,26 @@
+// 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));
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -2602,9 +2602,12 @@
   }
 
   for (const auto *Field : RD->fields()) {
-if (!Field->getType()->isReferenceType() &&
-!Context.hasUniqueObjectRepresentations(Field->getType()))
-  return llvm::None;
+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()));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits