rsmith added inline comments.
================ Comment at: lib/AST/ASTContext.cpp:2200 + Layout.getBaseClassOffset(Base->getAsCXXRecordDecl()); + CharUnits BaseSize = Context.getTypeSizeInChars(Base); + if (BaseOffset != CurOffset) ---------------- 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. https://reviews.llvm.org/D39347 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits