[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:2238-2240
+  // All other pointers are unique.
+  if (Ty->isPointerType() || Ty->isMemberPointerType())
+    return true;
----------------
rsmith wrote:
> erichkeane wrote:
> > 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.
> I don't think that will work; the MS implementation *sometimes* rounds the size up to the alignment. (... which sounds pretty dodgy to me; shouldn't the returned size always be a multiple of the returned alignment?)
Ah, right... I missed this part of the MicrosoftCXXABI:
  if (Target.getTriple().isArch64Bit())
    Width = llvm::alignTo(Width, Align);

So likely I can just do the same math as I implemented here, except before the width is reset in the alignTo here.

Thanks for getting back so quickly!  I'll get back on this once I get a patch for the array align error.


https://reviews.llvm.org/D39347