erichkeane marked 10 inline comments as done.
erichkeane added a comment.
Patch incoming, I think I got everything. I'll do the error on array of
aligned items in a separate patch, hopefully tomorrow AM.
================
Comment at: lib/AST/ASTContext.cpp:2183-2184
+ for (const auto *Field : RD->fields()) {
+ if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+ return false;
+ int64_t FieldSizeInBits =
----------------
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.
================
Comment at: lib/AST/ASTContext.cpp:2222
+
+ // Arrays are unique only if their element type is unique.
+ if (Ty->isArrayType())
----------------
rsmith wrote:
> As noted on the prior version of the patch, I don't think that's quite true.
> Here's an example:
>
> ```
> typedef __attribute__((aligned(4))) char bad[3]; // type with size 3, align 4
> (!!!)
> static_assert(!__has_unique_object_representations(bad[2]));
> ```
>
> Here, `ch` has unique object representations. But `bad[2]` does not, because
> forming the array type inserts one byte of padding after each element to keep
> the `ch` subobjects 4-byte aligned.
>
> Now, this is clearly a very problematic "extension", but we're being
> intentionally compatible with older versions of GCC here. At some point we
> should start rejecting this (as recent versions of GCC do), but until we do
> so, we should treat it properly, which means you can't assume that array
> types don't insert padding between elements. Sorry this is so horrible ;-(
As suggested on IRC, I'll be shortly putting together another patch to simply
remove this case (as alluded to here).
================
Comment at: lib/AST/ASTContext.cpp:2238-2240
+ // All other pointers are unique.
+ if (Ty->isPointerType() || Ty->isMemberPointerType())
+ return true;
----------------
rsmith wrote:
> This is not correct: member pointer representations can have padding bits. In
> the MS ABI, a pointer to member function is a pointer followed by 0-3 ints,
> and if there's an odd number of ints, I expect there'll be 4 bytes of padding
> at the end of the representation on 64-bit targets.
>
> I think you'll need to either add a `clang::CXXABI` entry point for
> determining whether a `MemberPointerType` has padding, or perhaps extend the
> existing `getMemberPointerWidthAndAlign` to also provide this information.
Based on looking through the two, I think I can just do this as Width%Align ==
0, right? I've got an incoming patch that does that, so hopefully that is
sufficient.
https://reviews.llvm.org/D39347
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
