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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: lib/AST/ASTContext.cpp:2200
+          Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+      CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+      if (BaseOffset != CurOffset)
----------------
rsmith wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > On reflection, I don't think this is right, and likewise I don't think the check for unique object representations of the base class above is quite right.
> > > 
> > > A class can be standard-layout but not POD for the purposes of layout (eg, by declaring a special member function). If so, the derived class can reuse the base class's tail padding, and if it does, the derived class can have unique object representations even when the base class does not. Example:
> > > 
> > > ```
> > > struct A { ~A(); int n; char a; }; struct B : A { char b, c, d; };
> > > ```
> > > 
> > > Here, under the Itanium C++ ABI, `sizeof(B) == 8`, and `B` has unique object representations even though its base class `A` does not.
> > > 
> > > 
> > > This should be fairly easy to fix. One approach would be to change the recursive call to `hasUniqueObjectRepresentations` above to return the actual size occupied by the struct and its fields (rather than checking for tail padding), and add that size here instead of using the base size. Or you could query the DataSize in the record layout rather than getting the (complete object) type size (but you'd then still need to check that there's no bits of tail padding before the end of the dsize, since we still pad out trailing bit-fields in the dsize computation).
> > According to the standard, the above case isn't, because it is non-trivial, right?  9.1 requires that "T" (B in your case) be trivially copyable, which it isn't, right?
> > 
> > The predicate condition for a template specialization
> > has_unique_object_representations<T> shall be
> > satisfied if and only if:
> > (9.1) - T is trivially copyable, and
> > (9.2) - any two objects of type T with the same value have the same
> > object representation
> Fixed example:
> 
> ```
> struct A { int &r; char a; }; struct B : A { char b[7]; };
> ```
AH!  I got caught up by the destructor as the reason it wasn't unique, but the actual thing you meant is that "A" has tail padding, so it is NOT unique.  However, on Itanium, the tail padding gets used when inheriting from it.

Do I have that correct? I just have to fix the behavior of inheriting-removing-tail-padding?


https://reviews.llvm.org/D39347