Fix __has_unique_object_representations based on rsmith's input

[erichkeane]

Summary

Corrections done for @rsmith 's comments after-commit
on cfe-commits.

[erichkeane]

Added all of @rsmith's comments here, I believe I have them all and have properly responded to them.

include/clang/AST/ASTContext.h

2155

Moved because @rsmith said:

I think this would make more sense as a member of ASTContext. The Type object generally doesn't know or care about its representation.

lib/AST/Type.cpp

2247

@rsmith said: Hmm, are there any cases in which we might want to guarantee the vptr is identical across all instances?

I believe the answer to that is 'no', but if you have a case in mind, I can change this.

2253

@rsmith said: This seems really fragile to me. Empty bases may or may not occupy storage. But that's beside the point -- empty bases are just a special case of bases with tail padding, which are not properly handled here.

I really think you should be walking the record layout rather than trying to determine this from sizes alone.

Response:
I think I did that properly in this patch here. I re-did a bunch of the code around this, so hopefully it is to your liking.

2267–2268

@rsmith said: Stray semicolon?

Yep, don't know how that got in there...

2315

@rsmith said: I don't think this is correct. As a weird GNU behaviour, we can have, say, a type with size 3 and alignment 4 (via an alignment attribute on a typedef). An array of 1 such element has size 4, and has padding even if its element type does not.

I think I added a test that covers that here. Is there a way to mark non-record types with an alignment? If not, this cause problems, because the Layout.getSize() checks in the struct handling will take care of this, since 'getSize' includes alignment.

2330

@rsmith said: The second half of this comment doesn't seem right to me. They may be represented as sequences of bits, but that doesn't make them integral.

I think you're right, I removed this part.

2337

@rsmith said: Why?

Thats a good question that I lack the answer to... I decided to remove this and let the 'struct' handling take care of it, which seems to make sense, and even better, seems to work.

2342

@rsmith said: Making these members of QualType seems counterproductive. You already have the Record here; it'd be better to make these file-static and pass that in.

Done.

[erichkeane]

Also, I should probably validate bitfields. The following SHOULD?! be unique?

struct S {
unsigned a : 1;
unsigned b : 2;
unsigned c : 3;
unsigned d : 2;
};

static_assert(__has_unique_object_representations(S), "error");

But without 'd', it should not be (tail padding).

[erichkeane]

Corrected version of the previous example:
struct S {
char a : 1;
char b : 2;
char c : 3;
char d : 2;
};
static_assert(sizeof(S) == 1, "size");
static_assert(__has_unique_object_representations(S), "error");

I haven't tested this patch agaainst this repro, but it should likely be in the tests.

[erichkeane]

Fixed for bitfields. Review by anyone greatly appreciated :)

[majnemer]

It'd be good to have tests which have alignment directives on bitfields.

[erichkeane]

Added a test for aligned bitfield, as @majnemer requested. Is this sufficient?

[erichkeane]

Ping? This fixes a number of bugs with the previous implementation, so I'd like to see if we can get this in.

[rsmith]

lib/AST/ASTContext.cpp

2159–2160

Remove this and change the getAs below to castAs.

2161

Remove the .getTypePtr() here; QualType has an overloaded operator-> that does this for you.

2166–2168

No braces needed here, and use auto * when initializing from dyn_cast.

2175

I think you mean !RD->isUnion(), right? You shouldn't assert if given an interface.

2178

The LLVM coding style does not permit this form of initialization. Use an initializer that expresses your intent instead, such as CharUnits::Zero().

2180

The getNumVBases check here is redundant; isDynamicClass checks that.

2187–2191

Maybe just omit empty classes from the Bases list entirely? That'd remove the need to recompute isStructEmpty below.

2213–2214

What should happen for fields of reference type?

2249–2250

If you want to do something with this case, the thing to do is to assert; it's a programming error to pass a null type here.

2252

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 ;-(

2260–2262

This is redundant; functions are not trivially copyable.

2268–2270

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.

[erichkeane]

Patch incoming, I think I got everything. I'll do the error on array of aligned items in a separate patch, hopefully tomorrow AM.

lib/AST/ASTContext.cpp

2213–2214

References are not trivially copyable, so they will prevent the struct from having a unique object representation.

2252

As suggested on IRC, I'll be shortly putting together another patch to simply remove this case (as alluded to here).

2268–2270

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.

[rsmith]

lib/AST/ASTContext.cpp

2268–2270

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?)

[erichkeane]

lib/AST/ASTContext.cpp

2268–2270

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.

[erichkeane]

Add has padding to CXXABI getMemberPointerWidthAndAlign to correct padding behavior here.

[rsmith]

Please add tests for the member pointer cases across various different targets.

lib/AST/ASTContext.cpp

2213

This isn't quite right; you want min(bitfield length, bit size of type) here. Eg, a bool b : 8 or int n : 1000 bitfield has padding bits.

2213–2214

That sounds like the wrong behavior to me. If two structs have references that bind to the same object, then they have the same object representation, so the struct does have unique object representations.

lib/AST/MicrosoftCXXABI.cpp

254

This seems to be backwards?

Also, I'm not sure whether this is correct. In the strange case where Width is not a multiple of Align (because we don't round up the width), there is no padding. We should only set HasPadding to true in the alignTo case below.

[erichkeane]

lib/AST/ASTContext.cpp

2213

I guess I'm missing something with this comment... why would a bitfield with bool b: 8 have padding?

Additionally, isn't int n : 1000 illegal, since 1000 > 32 (# bits in an int)?
Both clang and GCC reject the 2nd case. Curiously, GCC rejects bool b: 9, but Clang seems to allow 'bool' to have ANY size. Is that an error itself?

Finally: Should we consider ANY bool to be not a unique object representation? It has 1 bit of data, but 8 bits of storage. GCC's implementation of this trait accepts them, but I'm second guessing that at the moment...

2213–2214

I didn't think of it that way... I WILL note that GCC rejects references in their implementation, but that could be a bug on their part.

lib/AST/MicrosoftCXXABI.cpp

254

I think you're right about it being backwards.

However, doesn't the struct with a single Ptr and either 1 or 3 Ints have tail padding as well? I do believe you're right about the alignTo situation below, but only if Width changes, right?

[rsmith]

lib/AST/ASTContext.cpp

2213–2214

The wording you quote below is defective in this regard (it doesn't discuss what "same value" means for reference members) -- that's at least partly my wording, sorry about that! But at least my intent when we were working on the rules was that "same value" would have the obvious structurally-recursive meaning, which for references means we're considering whether they have the same referent.

So I think references, like pointers, should always be considered to have unique object representations when considered as members of objects of class type. (But __has_unique_object_representations(T&) should still return false because T& is not a trivially-copyable type, even though a class containing a T& might be.)

lib/AST/MicrosoftCXXABI.cpp

254

I think the idea is that a struct with one pointer and an odd number of ints, on 32-bit Windows, will have no padding per se, but its size will simply not be a multiple of its alignment. (So struct layout can put things in the four bytes after it, but will insert padding before it to place it at an 8 byte boundary.)

[rsmith]

lib/AST/MicrosoftCXXABI.cpp

254

See example here: https://godbolt.org/g/Nr8C2L

[erichkeane]

lib/AST/ASTContext.cpp

2213–2214

That makes sense to me, I can change that.

lib/AST/MicrosoftCXXABI.cpp

254

Well... oh dear.

That would mean that:

auto p = &A::N;
static_assert(!has_unique_object_representations_v<decltype(p)>); // not unique, since it has padding, right?
 
struct S {
   decltype(p) s;
};
static_assert(!has_unique_object_representations_v<S>); // also not unique, since 's' has padding.

struct S2 {
   decltype(p) s;
   int a;
};
static_assert(has_unique_object_representations_v<S2>); // Actually unique again, since the padding is filled.

Do I have this right?

[rsmith]

lib/AST/MicrosoftCXXABI.cpp

254

The first static_assert looks wrong. Should be:

static_assert(has_unique_object_representations_v<decltype(p)>); // unique, has no padding

Note that sizeof(decltype(p)) is the size without any padding. The type does not have padding itself, but might induce lead padding (due to alignment) and tail padding (due to size not being divisible by alignment) in objects it's placed within. But I think your current approach will get this all right, so long as MPI.HasPadding is only set to true in the cases where there actually is padding in the MPI.Width bytes of the representation (which only happens when Width is rounded up for alignment).

[erichkeane]

lib/AST/ASTContext.cpp

2213

I did a bit more looking into this... In the 'TYPE b: X' case, 'X' is always the size of the field, so :

struct Bitfield {
  bool b: 16;
};
static_assert(sizeof(Bitfield) == 2);

The rest of my padding detection works correctly.

[erichkeane]

thanks for the comments rsmith, think this is all caught up to your comments. Also added the test for member ptr differences.

[erichkeane]

Did ore looking into the bitfield shennangins. Will update this patch tomorrow. @rsmith if you get a chance, do you perhaps have an opinion on 'bool' fields? It seems to me that a bool could either be considered an 8 bit value, or a 1 bit value with 7 bits of padding. So:

bool b;
struct S { bool b; };
 // Patch currently lists these as unique.  Should they be: 
static_assert(!__has_unique_object_representations(b));
static_assert(!__has_unique_object_representations(S));

lib/AST/ASTContext.cpp

2213

Umm... so holy crap. We actually reserve the object spece for the whole thing, but ONLY the first sizeof(field) fields are actually stored to! Everything else is truncated!

I think the correct solution is to do:

// too large of a bitfield size causes truncation, and thus 'padding' bits.
if (FieldSizeInBits > Context.getTypeSizeInBits(Field->getType()) return false;

[rsmith]

lib/AST/ASTContext.cpp

2213

Sounds like you've found more GCC bugs. In C++ (unlike in C), there is no restriction on the maximum length of a bit-field. Specifically, [class.bit]p1 says: "The value of the integral constant expression may be larger than the number of bits in the object
representation (6.7) of the bit-field’s type; in such cases the extra bits are padding bits (6.7)."

For non-bool bit-fields, or bool bit-fields with more than 8 bits, the extra bits are padding, and the type does not have unique object representations. You'll need to check for this.

For bool bit-fields of length 1, we obviously have unique object representations for that one bit.

The interesting case is bool bit-fields with length between 2 and 8 inclusive. It would seem reasonable to assume they have unique representations, as we do for plain bool values, but this is really an ABI question (as, actually, is the plain bool case). The x86-64 psABI is not very clear on the bit-field question, but it does say "Booleans, when stored in a memory object, are stored as single byte objects the value of which is always 0 (false) or 1 (true).", so for at least x86-64, bools of up to 8 bits should be considered to have unique object representations. The PPC64 psABI appears to say the same thing, but is also somewhat unclear about the bit-field case.

2277

More cases to handle here:

• vectors (careful about, eg, vector of 3 foo)
• _Complex int and friends
• _Atomic T
• Obj-C block pointers
• Obj-C object pointers
• and perhaps OpenCL's various builtin types (pipe, sampler_t, event_t, clk_event_t, queue_t, reserve_id_t)

It's fine to leave these for another change, but we shouldn't forget about them. (There're also Obj-C class types and the Obj-C selector type, but I think it makes sense for those to return false here.)

[erichkeane]

lib/AST/ASTContext.cpp

2213

I suspect that the 'bool' 2-8 condition is a case where we can let the sleeping dogs lie here :) Incoming patch has the other condition, where the bitfield size is greater than the type size.

2277

I'm not sure what the correct answer is in any of those cases, since i'm not particularly familiar with any of them. I'll put a 'fixme' with the contents of your comment in the code, and revisit it (or hope someone more knowledgable can).

[erichkeane]

Woops, missed an 'svn add' and lost the member ptr test. Back now!

[rsmith]

lib/AST/ASTContext.cpp

2200

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).

2231

What >= comparison? I think the comment has got out of date relative to the code.

[erichkeane]

lib/AST/ASTContext.cpp

2200

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

[rsmith]

lib/AST/ASTContext.cpp

2200

Fixed example:

struct A { int &r; char a; }; struct B : A { char b[7]; };

[erichkeane]

lib/AST/ASTContext.cpp

2200

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?

[erichkeane]

Fixed the 'tail padding' mechanism to work correctly as suggested by @rsmith

[rsmith]

lib/AST/ASTContext.cpp

2200

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.

[erichkeane]

lib/AST/ASTContext.cpp

2200

Gah! You're right! Fix incoming.

[erichkeane]

Fix for trailing padding BITS, which obviously won't be merged.

[rsmith]

Thanks for your patience, this turned out to be surprisingly complicated.

[erichkeane]

In D39347#940015, @rsmith wrote:

Thanks for your patience, this turned out to be surprisingly complicated.

Of course! Thank you as well for your input, I was surprised at how complicated this ended up being as well!

include/clang/AST/ASTContext.h

public:
 
   void DeepCollectObjCIvars(const ObjCInterfaceDecl *OI, bool leafClass,
                             SmallVectorImpl<const ObjCIvarDecl*> &Ivars) const;
 
   unsigned CountNonClassIvars(const ObjCInterfaceDecl *OI) const;
   void CollectInheritedProtocols(const Decl *CDecl,
                           llvm::SmallPtrSet<ObjCProtocolDecl*, 8> &Protocols);
 
+  /// \brief Return true if the specified type has unique object representations
+  /// according to (C++17 [meta.unary.prop]p9)
+  bool hasUniqueObjectRepresentations(QualType Ty) const;
+
   //===--------------------------------------------------------------------===//
   //                            Type Operators
   //===--------------------------------------------------------------------===//
 
   /// \brief Return the canonical (structural) type corresponding to the
   /// specified potentially non-canonical type \p T.
   ///
   /// The non-canonical version of a type may have many "decorated" versions of

include/clang/AST/Type.h

public:
   bool isCXX11PODType(const ASTContext &Context) const;
 
   /// Return true if this is a trivial type per (C++0x [basic.types]p9)
   bool isTrivialType(const ASTContext &Context) const;
 
   /// Return true if this is a trivially copyable type (C++0x [basic.types]p9)
   bool isTriviallyCopyableType(const ASTContext &Context) const;
 
-  /// Return true if this has unique object representations according to (C++17
-  /// [meta.unary.prop]p9)
-  bool hasUniqueObjectRepresentations(const ASTContext &Context) const;
-
   // Don't promise in the API that anything besides 'const' can be
   // easily added.
 
   /// Add the `const` type qualifier to this QualType.
   void addConst() {
     addFastQualifiers(Qualifiers::Const);
   }
   QualType withConst() const {
public:
 
   /// Strip Objective-C "__kindof" types from the given type.
   QualType stripObjCKindOfType(const ASTContext &ctx) const;
 
   /// Remove all qualifiers including _Atomic.
   QualType getAtomicUnqualifiedType() const;
 
 private:
-  bool unionHasUniqueObjectRepresentations(const ASTContext& Context) const;
-  bool structHasUniqueObjectRepresentations(const ASTContext& Context) const;
-
   // These methods are implemented in a separate translation unit;
   // "static"-ize them to avoid creating temporary QualTypes in the
   // caller.
   static bool isConstant(QualType T, const ASTContext& Ctx);
   static QualType getDesugaredType(QualType T, const ASTContext &Context);
   static SplitQualType getSplitDesugaredType(QualType T);
   static SplitQualType getSplitUnqualifiedTypeImpl(QualType type);
   static QualType getSingleStepDesugaredTypeImpl(QualType type,

lib/AST/ASTContext.cpp

case Type::RValueReference:
     break;
   case Type::Pointer:
     AS = getTargetAddressSpace(cast<PointerType>(T)->getPointeeType());
     Width = Target->getPointerWidth(AS);
     Align = Target->getPointerAlign(AS);
     break;
   case Type::MemberPointer: {
     const MemberPointerType *MPT = cast<MemberPointerType>(T);
-    std::tie(Width, Align) = ABI->getMemberPointerWidthAndAlign(MPT);
+    CXXABI::MemberPointerInfo MPI = ABI->getMemberPointerInfo(MPT);
+    Width = MPI.Width;
+    Align = MPI.Align;
     break;
   }
   case Type::Complex: {
     // Complex types have the same alignment as their elements, but twice the
     // size.
     TypeInfo EltInfo = getTypeInfo(cast<ComplexType>(T)->getElementType());
     Width = EltInfo.Width * 2;
     Align = EltInfo.Align;
if (!Protocols.insert(
           const_cast<ObjCProtocolDecl *>(OP->getCanonicalDecl())).second)
       return;
 
     for (auto *Proto : OP->protocols())
       CollectInheritedProtocols(Proto, Protocols);
   }
 }
 
+static bool unionHasUniqueObjectRepresentations(const ASTContext &Context,
+                                                const RecordDecl *RD) {
+  assert(RD->isUnion() && "Must be union type");
+  CharUnits UnionSize = Context.getTypeSizeInChars(RD->getTypeForDecl());
+
+  for (const auto *Field : RD->fields()) {
+    if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+      return false;
+    CharUnits FieldSize = Context.getTypeSizeInChars(Field->getType());
+    if (FieldSize != UnionSize)
+      return false;
+  }
+  return true;
+}
+
+bool isStructEmpty(QualType Ty) {
+  const RecordDecl *RD = Ty->castAs<RecordType>()->getDecl();
+
+  if (!RD->field_empty())
+    return false;
+
+  if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RD))
+    return ClassDecl->isEmpty();
+
+  return true;
+}
+
+static llvm::Optional<int64_t>
+structHasUniqueObjectRepresentations(const ASTContext &Context,
+                                     const RecordDecl *RD) {
+  assert(!RD->isUnion() && "Must be struct/class type");
+  const auto &Layout = Context.getASTRecordLayout(RD);
+
+  int64_t CurOffsetInBits = 0;
+  if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RD)) {
+    if (ClassDecl->isDynamicClass())
+      return llvm::None;
+
+    SmallVector<std::pair<QualType, int64_t>, 4> Bases;
+    for (const auto Base : ClassDecl->bases()) {
+      // Empty types can be inherited from, and non-empty types can potentially
+      // have tail padding, so just make sure there isn't an error.
+      if (!isStructEmpty(Base.getType())) {
+        llvm::Optional<int64_t> Size = structHasUniqueObjectRepresentations(
+            Context, Base.getType()->getAs<RecordType>()->getDecl());
+        if (!Size)
+          return llvm::None;
+        Bases.emplace_back(Base.getType(), Size.getValue());
+      }
+    }
+
+    std::sort(
+        Bases.begin(), Bases.end(), [&](const std::pair<QualType, int64_t> &L,
+                                        const std::pair<QualType, int64_t> &R) {
+          return Layout.getBaseClassOffset(L.first->getAsCXXRecordDecl()) <
+                 Layout.getBaseClassOffset(R.first->getAsCXXRecordDecl());
+        });
+
+    for (const auto Base : Bases) {
+      int64_t BaseOffset = Context.toBits(
+          Layout.getBaseClassOffset(Base.first->getAsCXXRecordDecl()));
+      int64_t BaseSize = Base.second;
+      if (BaseOffset != CurOffsetInBits)
+        return llvm::None;
+      CurOffsetInBits = BaseOffset + BaseSize;
+    }
+  }
+
+  for (const auto *Field : RD->fields()) {
+    if (!Field->getType()->isReferenceType() &&
+        !Context.hasUniqueObjectRepresentations(Field->getType()))
+      return llvm::None;
+
+    int64_t FieldSizeInBits =
+        Context.toBits(Context.getTypeSizeInChars(Field->getType()));
+    if (Field->isBitField()) {
+      int64_t BitfieldSize = Field->getBitWidthValue(Context);
+
+      if (BitfieldSize > FieldSizeInBits)
+        return llvm::None;
+      FieldSizeInBits = BitfieldSize;
+    }
+
+    int64_t FieldOffsetInBits = Context.getFieldOffset(Field);
+
+    if (FieldOffsetInBits != CurOffsetInBits)
+      return llvm::None;
+
+    CurOffsetInBits = FieldSizeInBits + FieldOffsetInBits;
+  }
+
+  return CurOffsetInBits;
+}
+
+bool ASTContext::hasUniqueObjectRepresentations(QualType Ty) const {
+  // C++17 [meta.unary.prop]:
+  //   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, where two objects
+  //   of array or non-union class type are considered to have the same value
+  //   if their respective sequences of
+  //   direct subobjects have the same values, and two objects of union type
+  //   are considered to have the same
+  //   value if they have the same active member and the corresponding members
+  //   have the same value.
+  //   The set of scalar types for which this condition holds is
+  //   implementation-defined. [ Note: If a type has padding
+  //   bits, the condition does not hold; otherwise, the condition holds true
+  //   for unsigned integral types. -- end note ]
+  assert(!Ty.isNull() && "Null QualType sent to unique object rep check");
+
+  // Arrays are unique only if their element type is unique.
+  if (Ty->isArrayType())
+    return hasUniqueObjectRepresentations(getBaseElementType(Ty));
+
+  // (9.1) - T is trivially copyable...
+  if (!Ty.isTriviallyCopyableType(*this))
+    return false;
+
+  // All integrals and enums are unique.
+  if (Ty->isIntegralOrEnumerationType())
+    return true;
+
+  // All other pointers are unique.
+  if (Ty->isPointerType())
+    return true;
+
+  if (Ty->isMemberPointerType()) {
+    const MemberPointerType *MPT = Ty->getAs<MemberPointerType>();
+    return !ABI->getMemberPointerInfo(MPT).HasPadding;
+  }
+
+  if (Ty->isRecordType()) {
+    const RecordDecl *Record = Ty->getAs<RecordType>()->getDecl();
+
+    if (Record->isUnion())
+      return unionHasUniqueObjectRepresentations(*this, Record);
+
+    Optional<int64_t> StructSize =
+        structHasUniqueObjectRepresentations(*this, Record);
+
+    return StructSize &&
+           StructSize.getValue() == static_cast<int64_t>(getTypeSize(Ty));
+  }
+
+  // FIXME: More cases to handle here (list by rsmith):
+  // vectors (careful about, eg, vector of 3 foo)
+  // _Complex int and friends
+  // _Atomic T
+  // Obj-C block pointers
+  // Obj-C object pointers
+  // and perhaps OpenCL's various builtin types (pipe, sampler_t, event_t,
+  // clk_event_t, queue_t, reserve_id_t)
+  // There're also Obj-C class types and the Obj-C selector type, but I think it
+  // makes sense for those to return false here.
+
+  return false;
+}
+
 unsigned ASTContext::CountNonClassIvars(const ObjCInterfaceDecl *OI) const {
   unsigned count = 0;  
   // Count ivars declared in class extension.
   for (const auto *Ext : OI->known_extensions())
     count += Ext->ivar_size();
   
   // Count ivar defined in this class's implementation.  This
   // includes synthesized ivars.

lib/AST/CXXABI.h

 class MemberPointerType;
 class MangleNumberingContext;
 
 /// Implements C++ ABI-specific semantic analysis functions.
 class CXXABI {
 public:
   virtual ~CXXABI();
 
-  /// Returns the width and alignment of a member pointer in bits.
-  virtual std::pair<uint64_t, unsigned>
-  getMemberPointerWidthAndAlign(const MemberPointerType *MPT) const = 0;
+  struct MemberPointerInfo {
+    uint64_t Width;
+    unsigned Align;
+    bool HasPadding;
+  };
+
+  /// Returns the width and alignment of a member pointer in bits, as well as
+  /// whether it has padding.
+  virtual MemberPointerInfo
+  getMemberPointerInfo(const MemberPointerType *MPT) const = 0;
 
   /// Returns the default calling convention for C++ methods.
   virtual CallingConv getDefaultMethodCallConv(bool isVariadic) const = 0;
 
   /// Returns whether the given class is nearly empty, with just virtual
   /// pointers and no data except possibly virtual bases.
   virtual bool isNearlyEmpty(const CXXRecordDecl *RD) const = 0;
 

lib/AST/ItaniumCXXABI.cpp

 };
 
 class ItaniumCXXABI : public CXXABI {
 protected:
   ASTContext &Context;
 public:
   ItaniumCXXABI(ASTContext &Ctx) : Context(Ctx) { }
 
-  std::pair<uint64_t, unsigned>
-  getMemberPointerWidthAndAlign(const MemberPointerType *MPT) const override {
+  MemberPointerInfo
+  getMemberPointerInfo(const MemberPointerType *MPT) const override {
     const TargetInfo &Target = Context.getTargetInfo();
     TargetInfo::IntType PtrDiff = Target.getPtrDiffType(0);
-    uint64_t Width = Target.getTypeWidth(PtrDiff);
-    unsigned Align = Target.getTypeAlign(PtrDiff);
+    MemberPointerInfo MPI;
+    MPI.Width = Target.getTypeWidth(PtrDiff);
+    MPI.Align = Target.getTypeAlign(PtrDiff);
+    MPI.HasPadding = false;
     if (MPT->isMemberFunctionPointer())
-      Width = 2 * Width;
-    return std::make_pair(Width, Align);
+      MPI.Width *= 2;
+    return MPI;
   }
 
   CallingConv getDefaultMethodCallConv(bool isVariadic) const override {
     const llvm::Triple &T = Context.getTargetInfo().getTriple();
     if (!isVariadic && T.isWindowsGNUEnvironment() &&
         T.getArch() == llvm::Triple::x86)
       return CC_X86ThisCall;
     return CC_C;

lib/AST/MicrosoftCXXABI.cpp

class MicrosoftCXXABI : public CXXABI {
   llvm::SmallDenseMap<TagDecl *, DeclaratorDecl *>
       UnnamedTagDeclToDeclaratorDecl;
   llvm::SmallDenseMap<TagDecl *, TypedefNameDecl *>
       UnnamedTagDeclToTypedefNameDecl;
 
 public:
   MicrosoftCXXABI(ASTContext &Ctx) : Context(Ctx) { }
 
-  std::pair<uint64_t, unsigned>
-  getMemberPointerWidthAndAlign(const MemberPointerType *MPT) const override;
+  MemberPointerInfo
+  getMemberPointerInfo(const MemberPointerType *MPT) const override;
 
   CallingConv getDefaultMethodCallConv(bool isVariadic) const override {
     if (!isVariadic &&
         Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86)
       return CC_X86ThisCall;
     return CC_C;
   }
 
if (MSInheritanceAttr::hasNVOffsetField(MPT->isMemberFunctionPointer(),
     Ints++;
   if (MSInheritanceAttr::hasVBPtrOffsetField(Inheritance))
     Ints++;
   if (MSInheritanceAttr::hasVBTableOffsetField(Inheritance))
     Ints++;
   return std::make_pair(Ptrs, Ints);
 }
 
-std::pair<uint64_t, unsigned> MicrosoftCXXABI::getMemberPointerWidthAndAlign(
+CXXABI::MemberPointerInfo MicrosoftCXXABI::getMemberPointerInfo(
     const MemberPointerType *MPT) const {
   // The nominal struct is laid out with pointers followed by ints and aligned
   // to a pointer width if any are present and an int width otherwise.
   const TargetInfo &Target = Context.getTargetInfo();
   unsigned PtrSize = Target.getPointerWidth(0);
   unsigned IntSize = Target.getIntWidth();
 
   unsigned Ptrs, Ints;
   std::tie(Ptrs, Ints) = getMSMemberPointerSlots(MPT);
-  uint64_t Width = Ptrs * PtrSize + Ints * IntSize;
-  unsigned Align;
+  MemberPointerInfo MPI;
+  MPI.HasPadding = false;
+  MPI.Width = Ptrs * PtrSize + Ints * IntSize;
 
   // When MSVC does x86_32 record layout, it aligns aggregate member pointers to
   // 8 bytes.  However, __alignof usually returns 4 for data memptrs and 8 for
   // function memptrs.
   if (Ptrs + Ints > 1 && Target.getTriple().isArch32Bit())
-    Align = 64;
+    MPI.Align = 64;
   else if (Ptrs)
-    Align = Target.getPointerAlign(0);
+    MPI.Align = Target.getPointerAlign(0);
   else
-    Align = Target.getIntAlign();
+    MPI.Align = Target.getIntAlign();
 
-  if (Target.getTriple().isArch64Bit())
-    Width = llvm::alignTo(Width, Align);
-  return std::make_pair(Width, Align);
+  if (Target.getTriple().isArch64Bit()) {
+    MPI.Width = llvm::alignTo(MPI.Width, MPI.Align);
+    MPI.HasPadding = MPI.Width != (Ptrs * PtrSize + Ints * IntSize);
+  }
+  return MPI;
 }
 
 CXXABI *clang::CreateMicrosoftCXXABI(ASTContext &Ctx) {
   return new MicrosoftCXXABI(Ctx);
 }
 

lib/AST/Type.cpp

if (const RecordType *RT = CanonicalType->getAs<RecordType>()) {
 
     return true;
   }
 
   // No other types can match.
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-    const ASTContext &Context) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs<RecordType>()->getDecl();
-
-  for (const auto *Field : Union->fields()) {
-    if (!Field->getType().hasUniqueObjectRepresentations(Context))
-      return false;
-    CharUnits FieldSize = Context.getTypeSizeInChars(Field->getType());
-    if (FieldSize != UnionSize)
-      return false;
-  }
-  return true;
-}
-
-static bool isStructEmpty(QualType Ty) {
-  assert(Ty.getTypePtr()->isStructureOrClassType() &&
-         "Must be struct or class");
-  const RecordDecl *RD = Ty.getTypePtr()->getAs<RecordType>()->getDecl();
-
-  if (!RD->field_empty())
-    return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast<CXXRecordDecl>(RD)) {
-    return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-    const ASTContext &Context) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs<RecordType>()->getDecl();
-
-  if (isStructEmpty(*this))
-    return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast<CXXRecordDecl>(RD)) {
-    for (const auto Base : ClassDecl->bases()) {
-      if (Base.isVirtual())
-        return false;
-
-      // Empty bases are permitted, otherwise ensure base has unique
-      // representation. Also, Empty Base Optimization means that an
-      // Empty base takes up 0 size.
-      if (!isStructEmpty(Base.getType())) {
-        if (!Base.getType().structHasUniqueObjectRepresentations(Context))
-          return false;
-        BaseSize += Context.getTypeSizeInChars(Base.getType());
-      }
-    }
-  }
-
-  CharUnits StructSize = Context.getTypeSizeInChars(*this);
-
-  // This struct obviously has bases that keep it from being 'empty', so
-  // checking fields is no longer required.  Ensure that the struct size
-  // is the sum of the bases.
-  if (RD->field_empty())
-    return StructSize == BaseSize;
-
-  CharUnits CurOffset =
-      Context.toCharUnitsFromBits(Context.getFieldOffset(*RD->field_begin()));
-
-  // If the first field isn't at the sum of the size of the bases, there
-  // is padding somewhere.
-  if (BaseSize != CurOffset)
-    return false;
-
-  for (const auto *Field : RD->fields()) {
-    if (!Field->getType().hasUniqueObjectRepresentations(Context))
-      return false;
-    CharUnits FieldSize = Context.getTypeSizeInChars(Field->getType());
-    CharUnits FieldOffset =
-        Context.toCharUnitsFromBits(Context.getFieldOffset(Field));
-    // Has padding between fields.
-    if (FieldOffset != CurOffset)
-      return false;
-    CurOffset += FieldSize;
-  }
-  // Check for tail padding.
-  return CurOffset == StructSize;
-}
-
-bool QualType::hasUniqueObjectRepresentations(const ASTContext &Context) const {
-  // C++17 [meta.unary.prop]:
-  //   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, where two objects
-  //   of array or non-union class type are considered to have the same value
-  //   if their respective sequences of
-  //   direct subobjects have the same values, and two objects of union type
-  //   are considered to have the same
-  //   value if they have the same active member and the corresponding members
-  //   have the same value.
-  //   The set of scalar types for which this condition holds is
-  //   implementation-defined. [ Note: If a type has padding
-  //   bits, the condition does not hold; otherwise, the condition holds true
-  //   for unsigned integral types. -- end note ]
-  if (isNull())
-    return false;
-
-  // Arrays are unique only if their element type is unique.
-  if ((*this)->isArrayType())
-    return Context.getBaseElementType(*this).hasUniqueObjectRepresentations(
-        Context);
-
-  // (9.1) - T is trivially copyable, and
-  if (!isTriviallyCopyableType(Context))
-    return false;
-
-  // Functions are not unique.
-  if ((*this)->isFunctionType())
-    return false;
-
-  // All integrals and enums are unique!
-  if ((*this)->isIntegralOrEnumerationType())
-    return true;
-
-  // All pointers are unique, since they're just integrals.
-  if ((*this)->isPointerType() || (*this)->isMemberPointerType())
-    return true;
-
-  if ((*this)->isRecordType()) {
-    const RecordDecl *Record = getTypePtr()->getAs<RecordType>()->getDecl();
-
-    // Lambda types are not unique, so exclude them immediately.
-    if (Record->isLambda())
-      return false;
-
-    if (Record->isUnion())
-      return unionHasUniqueObjectRepresentations(Context);
-    return structHasUniqueObjectRepresentations(Context);
-  }
-  return false;
-}
-
 bool QualType::isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const {
   return !Context.getLangOpts().ObjCAutoRefCount &&
          Context.getLangOpts().ObjCWeak &&
          getObjCLifetime() != Qualifiers::OCL_Weak;
 }
 
 bool Type::isLiteralType(const ASTContext &Ctx) const {
   if (isDependentType())

lib/Sema/SemaExprCXX.cpp

case UTT_HasVirtualDestructor:
     // Embarcadero C++0x type trait functions:
     //   http://docwiki.embarcadero.com/RADStudio/XE/en/Type_Trait_Functions_(C%2B%2B0x)_Index
   case UTT_IsCompleteType:
     // http://docwiki.embarcadero.com/RADStudio/XE/en/Is_complete_type_(typename_T_):
     //   Returns True if and only if T is a complete type at the point of the
     //   function call.
     return !T->isIncompleteType();
   case UTT_HasUniqueObjectRepresentations:
-    return T.hasUniqueObjectRepresentations(C);
+    return C.hasUniqueObjectRepresentations(T);
   }
 }
 
 static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT, QualType LhsT,
                                     QualType RhsT, SourceLocation KeyLoc);
 
 static bool evaluateTypeTrait(Sema &S, TypeTrait Kind, SourceLocation KWLoc,
                               ArrayRef<TypeSourceInfo *> Args,

test/SemaCXX/has_unique_object_reps_member_ptr.cpp

+// RUN: %clang_cc1 -triple x86_64-linux-pc -DIS64 -fsyntax-only -verify -std=c++17 %s 
+// RUN: %clang_cc1 -triple x86_64-windows-pc -DIS64 -fsyntax-only -verify -std=c++17 %s
+// RUN: %clang_cc1 -triple i386-linux-pc -fsyntax-only -verify -std=c++17 %s
+// RUN: %clang_cc1 -triple i386-windows-pc -DW32 -fsyntax-only -verify -std=c++17 %s
+// expected-no-diagnostics
+
+struct Base {};
+struct A : virtual Base {
+  virtual void n() {}
+};
+
+auto p = &A::n;
+static_assert(__has_unique_object_representations(decltype(p)));
+
+struct B {
+  decltype(p) x;
+  int b;
+#ifdef IS64
+  // required on 64 bit to fill out the tail padding.
+  int c;
+#endif
+};
+static_assert(__has_unique_object_representations(B));
+
+struct C { // has padding on Win32, but nothing else.
+  decltype(p) x;
+};
+#ifdef W32
+static_assert(!__has_unique_object_representations(C));
+#else
+static_assert(__has_unique_object_representations(C));
+#endif

test/SemaCXX/type-traits.cpp

 
 static_assert(!has_unique_object_representations<VirtuallyInheritsFromNoPadding>::value, "No virtual inheritence");
 
 struct Padding {
   char a;
   int b;
 };
 
-static_assert(!has_unique_object_representations<Padding>::value, "but not with padding");
+//static_assert(!has_unique_object_representations<Padding>::value, "but not with padding");
 
 struct InheritsFromPadding : Padding {
   int c;
   int d;
 };
 
 static_assert(!has_unique_object_representations<InheritsFromPadding>::value, "or its subclasses");
 

 enum class ExampleEnumClass { xExample,
                               yExample };
 enum class LLEnumClass : long long { xLongExample,
                                      yLongExample };
 
 static_assert(has_unique_object_representations<ExampleEnumClass>::value, "Enums are integrals, so unique!");
 static_assert(has_unique_object_representations<LLEnumClass>::value, "Enums are integrals, so unique!");
 
-// because reference types aren't object types
+// because references aren't trivially copyable.
 static_assert(!has_unique_object_representations<int &>::value, "No references!");
 static_assert(!has_unique_object_representations<const int &>::value, "No references!");
 static_assert(!has_unique_object_representations<volatile int &>::value, "No references!");
 static_assert(!has_unique_object_representations<const volatile int &>::value, "No references!");
-
 static_assert(!has_unique_object_representations<Empty>::value, "No empty types!");
 
 class Compressed : Empty {
   int x;
 };
 
 static_assert(has_unique_object_representations<Compressed>::value, "But inheriting from one is ok");
 

 
 static_assert(has_unique_object_representations<int[42]>::value, "Arrays are fine, as long as their value type is");
 static_assert(has_unique_object_representations<int[]>::value, "Arrays are fine, as long as their value type is");
 static_assert(has_unique_object_representations<int[][42]>::value, "Arrays are fine, as long as their value type is");
 static_assert(!has_unique_object_representations<double[42]>::value, "So no array of doubles!");
 static_assert(!has_unique_object_representations<double[]>::value, "So no array of doubles!");
 static_assert(!has_unique_object_representations<double[][42]>::value, "So no array of doubles!");
 
+struct __attribute__((aligned(16))) WeirdAlignment {
+  int i;
+};
+union __attribute__((aligned(16))) WeirdAlignmentUnion {
+  int i;
+};
+static_assert(!has_unique_object_representations<WeirdAlignment>::value, "Alignment causes padding");
+static_assert(!has_unique_object_representations<WeirdAlignmentUnion>::value, "Alignment causes padding");
+static_assert(!has_unique_object_representations<WeirdAlignment[42]>::value, "Also no arrays that have padding");
+
 static_assert(!has_unique_object_representations<int(int)>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) const>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) volatile>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) const volatile>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) &>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) const &>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) volatile &>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int) const volatile &>::value, "Functions are not unique");

 static_assert(!has_unique_object_representations<int(int, ...) const &>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int, ...) volatile &>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int, ...) const volatile &>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int, ...) &&>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int, ...) const &&>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int, ...) volatile &&>::value, "Functions are not unique");
 static_assert(!has_unique_object_representations<int(int, ...) const volatile &&>::value, "Functions are not unique");
 
+void foo(){
   static auto lambda = []() {};
-static_assert(!has_unique_object_representations<decltype(lambda)>::value, "Lambdas are not unique");
+  static_assert(!has_unique_object_representations<decltype(lambda)>::value, "Lambdas follow struct rules");
+  int i;
+  static auto lambda2 = [i]() {};
+  static_assert(has_unique_object_representations<decltype(lambda2)>::value, "Lambdas follow struct rules");
+}
+
+struct PaddedBitfield {
+  char c : 6;
+  char d : 1;
+};
+
+struct UnPaddedBitfield {
+  char c : 6;
+  char d : 2;
+};
+
+struct AlignedPaddedBitfield {
+  char c : 6;
+  __attribute__((aligned(1)))
+  char d : 2;
+};
+
+static_assert(!has_unique_object_representations<PaddedBitfield>::value, "Bitfield padding");
+static_assert(has_unique_object_representations<UnPaddedBitfield>::value, "Bitfield padding");
+static_assert(!has_unique_object_representations<AlignedPaddedBitfield>::value, "Bitfield padding");
+
+struct BoolBitfield {
+  bool b : 8;
+};
+
+static_assert(has_unique_object_representations<BoolBitfield>::value, "Bitfield bool");
+
+struct BoolBitfield2 {
+  bool b : 16;
+};
+
+static_assert(!has_unique_object_representations<BoolBitfield2>::value, "Bitfield bool");
+
+struct GreaterSizeBitfield {
+  //expected-warning@+1 {{width of bit-field 'n'}}
+  int n : 1024;
+};
+
+static_assert(sizeof(GreaterSizeBitfield) == 128, "Bitfield Size");
+static_assert(!has_unique_object_representations<GreaterSizeBitfield>::value, "Bitfield padding");
+
+struct StructWithRef {
+  int &I;
+};
+
+static_assert(has_unique_object_representations<StructWithRef>::value, "References are still unique");
+
+struct NotUniqueBecauseTailPadding {
+  int &r;
+  char a;
+};
+struct CanBeUniqueIfNoPadding : NotUniqueBecauseTailPadding {
+  char b[7];
+};
+
+static_assert(!has_unique_object_representations<NotUniqueBecauseTailPadding>::value, 
+              "non trivial");
+// Can be unique on Itanium, since the is child class' data is 'folded' into the
+// parent's tail padding.
+static_assert(sizeof(CanBeUniqueIfNoPadding) != 16 ||
+              has_unique_object_representations<CanBeUniqueIfNoPadding>::value,
+              "inherit from std layout");