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

[Erich Keane via Phabricator via cfe-commits]

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