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:
> > > 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?
> You have fallen into a trap :) There can be (up to 7 bits of) padding between
> the end of the members and the end of the data size, specifically if the
> struct ends in a bit-field. You could check for that before you return
> `CurOffsetInBits` at the end of this function, but I think it'd be better to
> store the size in `Bases` and just use that down here rather than grabbing
> and using the data size.
Gah! You're right! Fix incoming.
https://reviews.llvm.org/D39347
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
