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

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC319446: Fix __has_unique_object_representations 
implementation (authored by erichkeane).

Repository:
  rC Clang

https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/CXXABI.h
  lib/AST/ItaniumCXXABI.cpp
  lib/AST/MicrosoftCXXABI.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/has_unique_object_reps_member_ptr.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -1856,7 +1856,9 @@
 break;
   case Type::MemberPointer: {
 const MemberPointerType *MPT = cast(T);
-std::tie(Width, Align) = ABI->getMemberPointerWidthAndAlign(MPT);
+CXXABI::MemberPointerInfo MPI = ABI->getMemberPointerInfo(MPT);
+Width = MPI.Width;
+Align = MPI.Align;
 break;
   }
   case Type::Complex: {
@@ -2138,6 +2140,168 @@
   }
 }
 
+static bool unionHasUniqueObjectRepresentations(const ASTContext ,
+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()->getDecl();
+
+  if (!RD->field_empty())
+return false;
+
+  if (const auto *ClassDecl = dyn_cast(RD))
+return ClassDecl->isEmpty();
+
+  return true;
+}
+
+static llvm::Optional
+structHasUniqueObjectRepresentations(const ASTContext ,
+ const RecordDecl *RD) {
+  assert(!RD->isUnion() && "Must be struct/class type");
+  const auto  = Context.getASTRecordLayout(RD);
+
+  int64_t CurOffsetInBits = 0;
+  if (const auto *ClassDecl = dyn_cast(RD)) {
+if (ClassDecl->isDynamicClass())
+  return llvm::None;
+
+SmallVector, 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 Size = structHasUniqueObjectRepresentations(
+Context, Base.getType()->getAs()->getDecl());
+if (!Size)
+  return llvm::None;
+Bases.emplace_back(Base.getType(), Size.getValue());
+  }
+}
+
+std::sort(
+Bases.begin(), Bases.end(), [&](const std::pair ,
+const std::pair ) {
+  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 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 

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In https://reviews.llvm.org/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!


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

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


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 124863.
erichkeane added a comment.

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


https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/CXXABI.h
  lib/AST/ItaniumCXXABI.cpp
  lib/AST/MicrosoftCXXABI.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/has_unique_object_reps_member_ptr.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2201,150 +2201,6 @@
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs()->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()->getDecl();
-
-  if (!RD->field_empty())
-return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
-return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs()->getDecl();
-
-  if (isStructEmpty(*this))
-return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(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 ) const {
-  // C++17 [meta.unary.prop]:
-  //   The predicate condition for a template specialization
-  //   has_unique_object_representations 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;
-
- 

[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:2200
+  Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+  CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+  if (BaseOffset != CurOffset)

rsmith wrote:
> 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 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  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.
Gah!  You're right!  Fix incoming.


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 124859.
erichkeane added a comment.

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


https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/CXXABI.h
  lib/AST/ItaniumCXXABI.cpp
  lib/AST/MicrosoftCXXABI.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/has_unique_object_reps_member_ptr.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2201,150 +2201,6 @@
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs()->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()->getDecl();
-
-  if (!RD->field_empty())
-return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
-return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs()->getDecl();
-
-  if (isStructEmpty(*this))
-return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(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 ) const {
-  // C++17 [meta.unary.prop]:
-  //   The predicate condition for a template specialization
-  //   has_unique_object_representations 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())
-

[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:2200
+  Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+  CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+  if (BaseOffset != CurOffset)

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


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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:
> > 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 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  char a; }; struct B : A { char b[7]; };
```


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked an inline comment as done.
erichkeane added inline comments.



Comment at: lib/AST/ASTContext.cpp:2200
+  Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+  CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+  if (BaseOffset != CurOffset)

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


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/AST/ASTContext.cpp:2200
+  Layout.getBaseClassOffset(Base->getAsCXXRecordDecl());
+  CharUnits BaseSize = Context.getTypeSizeInChars(Base);
+  if (BaseOffset != CurOffset)

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



Comment at: lib/AST/ASTContext.cpp:2231
+
+  // Handles tail-padding.  >= comparision takes care of EBO cases.
+  return CurOffsetInBits == Context.toBits(Layout.getSize());

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


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 124769.
erichkeane added a comment.

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


https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/CXXABI.h
  lib/AST/ItaniumCXXABI.cpp
  lib/AST/MicrosoftCXXABI.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/has_unique_object_reps_member_ptr.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2201,150 +2201,6 @@
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs()->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()->getDecl();
-
-  if (!RD->field_empty())
-return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
-return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs()->getDecl();
-
-  if (isStructEmpty(*this))
-return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(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 ) const {
-  // C++17 [meta.unary.prop]:
-  //   The predicate condition for a template specialization
-  //   has_unique_object_representations 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;

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 124765.

https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/CXXABI.h
  lib/AST/ItaniumCXXABI.cpp
  lib/AST/MicrosoftCXXABI.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2201,150 +2201,6 @@
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs()->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()->getDecl();
-
-  if (!RD->field_empty())
-return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
-return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs()->getDecl();
-
-  if (isStructEmpty(*this))
-return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(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 ) const {
-  // C++17 [meta.unary.prop]:
-  //   The predicate condition for a template specialization
-  //   has_unique_object_representations 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 

[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:2213
+Field->isBitField()
+? Field->getBitWidthValue(Context)
+: Context.toBits(Context.getTypeSizeInChars(Field->getType()));

erichkeane wrote:
> rsmith wrote:
> > erichkeane wrote:
> > > erichkeane wrote:
> > > > rsmith wrote:
> > > > > 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.
> > > > 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...
> > > 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.
> > 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, `bool`s 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.
> 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;
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.



Comment at: lib/AST/ASTContext.cpp:2277
+
+  return false;
+}

rsmith wrote:
> 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.)
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).


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/AST/ASTContext.cpp:2213
+Field->isBitField()
+? Field->getBitWidthValue(Context)
+: Context.toBits(Context.getTypeSizeInChars(Field->getType()));

erichkeane wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > 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.
> > 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...
> 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.
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, `bool`s 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.



Comment at: lib/AST/ASTContext.cpp:2277
+
+  return false;
+}

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


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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




Comment at: lib/AST/ASTContext.cpp:2213
+Field->isBitField()
+? Field->getBitWidthValue(Context)
+: Context.toBits(Context.getTypeSizeInChars(Field->getType()));

erichkeane wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > 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.
> > 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...
> 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.
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;


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 124679.
erichkeane added a comment.

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


https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/CXXABI.h
  lib/AST/ItaniumCXXABI.cpp
  lib/AST/MicrosoftCXXABI.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/has_unique_object_reps_member_ptr.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2201,150 +2201,6 @@
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs()->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()->getDecl();
-
-  if (!RD->field_empty())
-return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
-return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs()->getDecl();
-
-  if (isStructEmpty(*this))
-return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(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 ) const {
-  // C++17 [meta.unary.prop]:
-  //   The predicate condition for a template specialization
-  //   has_unique_object_representations 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 

[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:2213
+Field->isBitField()
+? Field->getBitWidthValue(Context)
+: Context.toBits(Context.getTypeSizeInChars(Field->getType()));

erichkeane wrote:
> rsmith wrote:
> > 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.
> 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...
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.


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/AST/MicrosoftCXXABI.cpp:253
+
+  MPI.HasPadding = MPI.Width % MPI.Align == 0;
 

erichkeane wrote:
> rsmith wrote:
> > rsmith wrote:
> > > erichkeane wrote:
> > > > rsmith wrote:
> > > > > 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.
> > > > 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?
> > > 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.)
> > See example here: https://godbolt.org/g/Nr8C2L
> Well... oh dear.  
> 
> That would mean that: 
>   auto p = ::N;
>   static_assert(!has_unique_object_representations_v); // not 
> unique, since it has padding, right?
>  
>   struct S {
>  decltype(p) s;
>   };
>   static_assert(!has_unique_object_representations_v); // also not unique, 
> since 's' has padding.
> 
>   struct S2 {
>  decltype(p) s;
>  int a;
>   };
>   static_assert(has_unique_object_representations_v); // Actually unique 
> again, since the padding is filled.
> 
> 
> Do I have this right?
The first `static_assert` looks wrong. Should be:

```
static_assert(has_unique_object_representations_v); // 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).


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:2183-2184
+  for (const auto *Field : RD->fields()) {
+if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+  return false;
+int64_t FieldSizeInBits =

rsmith wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > erichkeane wrote:
> > > > 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.
> > > 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.
> > 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.
> 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.)
That makes sense to me, I can change that.



Comment at: lib/AST/MicrosoftCXXABI.cpp:253
+
+  MPI.HasPadding = MPI.Width % MPI.Align == 0;
 

rsmith wrote:
> rsmith wrote:
> > erichkeane wrote:
> > > rsmith wrote:
> > > > 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.
> > > 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?
> > 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.)
> See example here: https://godbolt.org/g/Nr8C2L
Well... oh dear.  

That would mean that: 
  auto p = ::N;
  static_assert(!has_unique_object_representations_v); // not 
unique, since it has padding, right?
 
  struct S {
 decltype(p) s;
  };
  static_assert(!has_unique_object_representations_v); // also not unique, 
since 's' has padding.

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


Do I have this right?


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/AST/MicrosoftCXXABI.cpp:253
+
+  MPI.HasPadding = MPI.Width % MPI.Align == 0;
 

rsmith wrote:
> erichkeane wrote:
> > rsmith wrote:
> > > 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.
> > 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?
> 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.)
See example here: https://godbolt.org/g/Nr8C2L


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:2183-2184
+  for (const auto *Field : RD->fields()) {
+if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+  return false;
+int64_t FieldSizeInBits =

rsmith wrote:
> erichkeane wrote:
> > 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.
> 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.
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.



Comment at: lib/AST/ASTContext.cpp:2213
+Field->isBitField()
+? Field->getBitWidthValue(Context)
+: Context.toBits(Context.getTypeSizeInChars(Field->getType()));

rsmith wrote:
> 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.
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...



Comment at: lib/AST/MicrosoftCXXABI.cpp:253
+
+  MPI.HasPadding = MPI.Width % MPI.Align == 0;
 

rsmith wrote:
> 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.
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?


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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




Comment at: lib/AST/ASTContext.cpp:2183-2184
+  for (const auto *Field : RD->fields()) {
+if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+  return false;
+int64_t FieldSizeInBits =

erichkeane wrote:
> 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.
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.



Comment at: lib/AST/ASTContext.cpp:2213
+Field->isBitField()
+? Field->getBitWidthValue(Context)
+: Context.toBits(Context.getTypeSizeInChars(Field->getType()));

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.



Comment at: lib/AST/MicrosoftCXXABI.cpp:253
+
+  MPI.HasPadding = MPI.Width % MPI.Align == 0;
 

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.


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 124609.
erichkeane added a comment.

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


https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/CXXABI.h
  lib/AST/ItaniumCXXABI.cpp
  lib/AST/MicrosoftCXXABI.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2201,150 +2201,6 @@
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs()->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()->getDecl();
-
-  if (!RD->field_empty())
-return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
-return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs()->getDecl();
-
-  if (isStructEmpty(*this))
-return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(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 ) const {
-  // C++17 [meta.unary.prop]:
-  //   The predicate condition for a template specialization
-  //   has_unique_object_representations 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 

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/AST/ASTContext.cpp:2238-2240
+  // All other pointers are unique.
+  if (Ty->isPointerType() || Ty->isMemberPointerType())
+return true;

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


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 124490.
erichkeane marked an inline comment as done.

https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2201,150 +2201,6 @@
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs()->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()->getDecl();
-
-  if (!RD->field_empty())
-return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
-return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs()->getDecl();
-
-  if (isStructEmpty(*this))
-return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(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 ) const {
-  // C++17 [meta.unary.prop]:
-  //   The predicate condition for a template specialization
-  //   has_unique_object_representations 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 

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/AST/ASTContext.cpp:2129-2130
+bool isStructEmpty(QualType Ty) {
+  assert(Ty.getTypePtr()->isStructureOrClassType() &&
+ "Must be struct or class");
+  const RecordDecl *RD = Ty.getTypePtr()->getAs()->getDecl();

Remove this and change the `getAs` below to `castAs`.



Comment at: lib/AST/ASTContext.cpp:2131
+ "Must be struct or class");
+  const RecordDecl *RD = Ty.getTypePtr()->getAs()->getDecl();
+

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



Comment at: lib/AST/ASTContext.cpp:2136-2138
+  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
+return ClassDecl->isEmpty();
+  }

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



Comment at: lib/AST/ASTContext.cpp:2145
+ const RecordDecl *RD) {
+  assert((RD->isStruct() || RD->isClass()) && "Must be struct/class type");
+  const auto  = Context.getASTRecordLayout(RD);

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



Comment at: lib/AST/ASTContext.cpp:2148
+
+  CharUnits CurOffset{};
+  if (const auto *ClassDecl = dyn_cast(RD)) {

The LLVM coding style [does not 
permit](http://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor)
 this form of initialization. Use an initializer that expresses your intent 
instead, such as `CharUnits::Zero()`.



Comment at: lib/AST/ASTContext.cpp:2150
+  if (const auto *ClassDecl = dyn_cast(RD)) {
+if (ClassDecl->isDynamicClass() || ClassDecl->getNumVBases() != 0)
+  return false;

The `getNumVBases` check here is redundant; `isDynamicClass` checks that.



Comment at: lib/AST/ASTContext.cpp:2157-2161
+  if (!isStructEmpty(Base.getType()) &&
+  !Context.hasUniqueObjectRepresentations(Base.getType()))
+return false;
+  Bases.push_back(Base.getType());
+}

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



Comment at: lib/AST/ASTContext.cpp:2183-2184
+  for (const auto *Field : RD->fields()) {
+if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+  return false;
+int64_t FieldSizeInBits =

What should happen for fields of reference type?



Comment at: lib/AST/ASTContext.cpp:2219-2220
+  //   for unsigned integral types. -- end note ]
+  if (Ty.isNull())
+return false;
+

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.



Comment at: lib/AST/ASTContext.cpp:
+
+  // Arrays are unique only if their element type is unique.
+  if (Ty->isArrayType())

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



Comment at: lib/AST/ASTContext.cpp:2230-2232
+  // Functions are not unique.
+  if (Ty->isFunctionType())
+return false;

This is redundant; functions are not trivially copyable.



Comment at: lib/AST/ASTContext.cpp:2238-2240
+  // All other pointers are unique.
+  if (Ty->isPointerType() || Ty->isMemberPointerType())
+return true;

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.


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 122897.
erichkeane added a comment.

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


https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2166,151 +2166,6 @@
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs()->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()->getDecl();
-
-  if (!RD->field_empty())
-return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
-return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs()->getDecl();
-
-  if (isStructEmpty(*this))
-return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(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 ) const {
-  // C++17 [meta.unary.prop]:
-  //   The predicate condition for a template specialization
-  //   has_unique_object_representations 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;
-
- 

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

[David Majnemer via Phabricator via cfe-commits]

majnemer added a comment.

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


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 122883.
erichkeane added a comment.

Fixed for bitfields.  Review by anyone greatly appreciated :)


https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2166,151 +2166,6 @@
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs()->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()->getDecl();
-
-  if (!RD->field_empty())
-return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
-return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs()->getDecl();
-
-  if (isStructEmpty(*this))
-return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(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 ) const {
-  // C++17 [meta.unary.prop]:
-  //   The predicate condition for a template specialization
-  //   has_unique_object_representations 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 

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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.


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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


https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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




Comment at: include/clang/AST/ASTContext.h:2115
+  bool hasUniqueObjectRepresentations(QualType Ty) const;
+
   
//======//

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.



Comment at: lib/AST/Type.cpp:2212
-for (const auto Base : ClassDecl->bases()) {
-  if (Base.isVirtual())
-return false;

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



Comment at: lib/AST/Type.cpp:2218
-  // Empty base takes up 0 size.
-  if (!isStructEmpty(Base.getType())) {
-if (!Base.getType().structHasUniqueObjectRepresentations(Context))

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




Comment at: lib/AST/Type.cpp:2233
-return StructSize == BaseSize;
-  ;
-

@rsmith said: Stray semicolon?

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




Comment at: lib/AST/Type.cpp:2281
-  if ((*this)->isArrayType())
-return Context.getBaseElementType(*this).hasUniqueObjectRepresentations(
-Context);

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



Comment at: lib/AST/Type.cpp:2296
-
-  // All pointers are unique, since they're just integrals.
-  if ((*this)->isPointerType() || (*this)->isMemberPointerType())

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



Comment at: lib/AST/Type.cpp:2303
-
-// Lambda types are not unique, so exclude them immediately.
-if (Record->isLambda())

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




Comment at: lib/AST/Type.cpp:2308
-if (Record->isUnion())
-  return unionHasUniqueObjectRepresentations(Context);
-return structHasUniqueObjectRepresentations(Context);

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



https://reviews.llvm.org/D39347



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

erichkeane created this revision.

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


https://reviews.llvm.org/D39347

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  lib/AST/ASTContext.cpp
  lib/AST/Type.cpp
  lib/Sema/SemaExprCXX.cpp
  test/SemaCXX/type-traits.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2166,151 +2166,6 @@
   return false;
 }
 
-bool QualType::unionHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isUnionType() && "must be union type");
-  CharUnits UnionSize = Context.getTypeSizeInChars(*this);
-  const RecordDecl *Union = getTypePtr()->getAs()->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;
-}
-
-bool isStructEmpty(QualType Ty) {
-  assert(Ty.getTypePtr()->isStructureOrClassType() &&
- "Must be struct or class");
-  const RecordDecl *RD = Ty.getTypePtr()->getAs()->getDecl();
-
-  if (!RD->field_empty())
-return false;
-
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(RD)) {
-return ClassDecl->isEmpty();
-  }
-
-  return true;
-}
-
-bool QualType::structHasUniqueObjectRepresentations(
-const ASTContext ) const {
-  assert((*this)->isStructureOrClassType() && "Must be struct or class");
-  const RecordDecl *RD = getTypePtr()->getAs()->getDecl();
-
-  if (isStructEmpty(*this))
-return false;
-
-  // Check base types.
-  CharUnits BaseSize{};
-  if (const CXXRecordDecl *ClassDecl = dyn_cast(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 ) const {
-  // C++17 [meta.unary.prop]:
-  //   The predicate condition for a template specialization
-  //   has_unique_object_representations 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.