[PATCH] D39347: Fix __has_unique_object_representations based on rsmith's input

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

rsmith added inline comments.


================
Comment at: lib/AST/ASTContext.cpp:2183-2184
+  for (const auto *Field : RD->fields()) {
+    if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+      return false;
+    int64_t FieldSizeInBits =
----------------
erichkeane wrote:
> rsmith wrote:
> > erichkeane wrote:
> > > rsmith wrote:
> > > > What should happen for fields of reference type?
> > > References are not trivially copyable, so they will prevent the struct from having a unique object representation.
> > That sounds like the wrong behavior to me. If two structs have references that bind to the same object, then they have the same object representation, so the struct does have unique object representations.
> I didn't think of it that way... I WILL note that GCC rejects references in their implementation, but that could be a bug on their part.
The wording you quote below is defective in this regard (it doesn't discuss what "same value" means for reference members) -- that's at least partly my wording, sorry about that! But at least my intent when we were working on the rules was that "same value" would have the obvious structurally-recursive meaning, which for references means we're considering whether they have the same referent.

So I think references, like pointers, should always be considered to have unique object representations when considered as members of objects of class type. (But `__has_unique_object_representations(T&)` should still return `false` because `T&` is not a trivially-copyable type, even though a class containing a `T&` might be.)


================
Comment at: lib/AST/MicrosoftCXXABI.cpp:253
+
+  MPI.HasPadding = MPI.Width % MPI.Align == 0;
 
----------------
erichkeane wrote:
> rsmith wrote:
> > This seems to be backwards?
> > 
> > Also, I'm not sure whether this is correct. In the strange case where `Width` is not a multiple of `Align` (because we don't round up the width), there is no padding. We should only set `HasPadding` to `true` in the `alignTo` case below.
> I think you're right about it being backwards.
> 
> However, doesn't the struct with a single Ptr and either 1 or 3 Ints have tail padding as well?  I do believe you're right about the alignTo situation below, but only if Width changes, right?
I think the idea is that a struct with one pointer and an odd number of ints, on 32-bit Windows, will have no padding per se, but its size will simply not be a multiple of its alignment. (So struct layout can put things in the four bytes after it, but will insert padding before it to place it at an 8 byte boundary.)


https://reviews.llvm.org/D39347