[clang][PAC] Fix builtins that claim address discriminated types are bitwise compatible #154490
Conversation
|
@llvm/pr-subscribers-clang Author: Oliver Hunt (ojhunt) ChangesA number of builtins report some variation of "this type is compatibile with some bitwise equivalent operation", but this is not true for address discriminated values. We had address a number of cases, but not all of them. This PR corrects the remaining builtins. Fixes #154394 Full diff: https://github.com/llvm/llvm-project/pull/154490.diff 5 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 7c2566a09665d..d5944c19b1c15 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -640,7 +640,7 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// contain data that is address discriminated. This includes
/// implicitly authenticated values like vtable pointers, as well as
/// explicitly qualified fields.
- bool containsAddressDiscriminatedPointerAuth(QualType T) {
+ bool containsAddressDiscriminatedPointerAuth(QualType T) const {
if (!isPointerAuthenticationAvailable())
return false;
return findPointerAuthContent(T) != PointerAuthContent::None;
@@ -654,8 +654,8 @@ class ASTContext : public RefCountedBase<ASTContext> {
bool containsNonRelocatablePointerAuth(QualType T) {
if (!isPointerAuthenticationAvailable())
return false;
- return findPointerAuthContent(T) ==
- PointerAuthContent::AddressDiscriminatedData;
+ return findPointerAuthContent(T) !=
+ PointerAuthContent::None;
}
private:
@@ -673,8 +673,8 @@ class ASTContext : public RefCountedBase<ASTContext> {
bool isPointerAuthenticationAvailable() const {
return LangOpts.PointerAuthCalls || LangOpts.PointerAuthIntrinsics;
}
- PointerAuthContent findPointerAuthContent(QualType T);
- llvm::DenseMap<const RecordDecl *, PointerAuthContent>
+ PointerAuthContent findPointerAuthContent(QualType T) const;
+ mutable llvm::DenseMap<const RecordDecl *, PointerAuthContent>
RecordContainsAddressDiscriminatedPointerAuth;
ImportDecl *FirstLocalImport = nullptr;
@@ -3720,7 +3720,7 @@ OPT_LIST(V)
/// Resolve the root record to be used to derive the vtable pointer
/// authentication policy for the specified record.
const CXXRecordDecl *
- baseForVTableAuthentication(const CXXRecordDecl *ThisClass);
+ baseForVTableAuthentication(const CXXRecordDecl *ThisClass) const;
bool useAbbreviatedThunkName(GlobalDecl VirtualMethodDecl,
StringRef MangledName);
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 2f2685495a8f1..69c1489eef76b 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -1708,7 +1708,7 @@ void ASTContext::setRelocationInfoForCXXRecord(
}
static bool primaryBaseHaseAddressDiscriminatedVTableAuthentication(
- ASTContext &Context, const CXXRecordDecl *Class) {
+ const ASTContext &Context, const CXXRecordDecl *Class) {
if (!Class->isPolymorphic())
return false;
const CXXRecordDecl *BaseType = Context.baseForVTableAuthentication(Class);
@@ -1723,7 +1723,7 @@ static bool primaryBaseHaseAddressDiscriminatedVTableAuthentication(
return AddressDiscrimination == AuthAttr::AddressDiscrimination;
}
-ASTContext::PointerAuthContent ASTContext::findPointerAuthContent(QualType T) {
+ASTContext::PointerAuthContent ASTContext::findPointerAuthContent(QualType T) const {
assert(isPointerAuthenticationAvailable());
T = T.getCanonicalType();
@@ -15175,7 +15175,7 @@ StringRef ASTContext::getCUIDHash() const {
}
const CXXRecordDecl *
-ASTContext::baseForVTableAuthentication(const CXXRecordDecl *ThisClass) {
+ASTContext::baseForVTableAuthentication(const CXXRecordDecl *ThisClass) const {
assert(ThisClass);
assert(ThisClass->isPolymorphic());
const CXXRecordDecl *PrimaryBase = ThisClass;
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 50b1a1d000090..5b032cb44ba76 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -1438,6 +1438,12 @@ void CXXRecordDecl::addedMember(Decl *D) {
data().StructuralIfLiteral = false;
}
+ if (!data().HasTrivialSpecialMembers && T.hasAddressDiscriminatedPointerAuth()) {
+ // Address discriminated fields mean that a class is no longer
+ // standard layout.
+ data().HasTrivialSpecialMembers = true;
+ }
+
// C++14 [meta.unary.prop]p4:
// T is a class type [...] with [...] no non-static data members other
// than subobjects of zero size
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 5fbf1999ed725..1071a372162aa 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2771,6 +2771,11 @@ bool QualType::isCXX98PODType(const ASTContext &Context) const {
return false;
QualType CanonicalType = getTypePtr()->CanonicalType;
+
+ // Any type that is, or contains, address discriminated data is non-POD
+ if (CanonicalType.hasAddressDiscriminatedPointerAuth())// Context.containsAddressDiscriminatedPointerAuth(*this))
+ return false;
+
switch (CanonicalType->getTypeClass()) {
// Everything not explicitly mentioned is not POD.
default:
@@ -2829,6 +2834,10 @@ bool QualType::isTrivialType(const ASTContext &Context) const {
if (CanonicalType->isDependentType())
return false;
+ // Any type that is, or contains, address discriminated data is non-POD
+ if (CanonicalType.hasAddressDiscriminatedPointerAuth()) // Context.containsAddressDiscriminatedPointerAuth(CanonicalType))
+ return false;
+
// C++0x [basic.types]p9:
// Scalar types, trivial class types, arrays of such types, and
// cv-qualified versions of these types are collectively called trivial
@@ -3179,6 +3188,10 @@ bool QualType::isCXX11PODType(const ASTContext &Context) const {
if (BaseTy->isIncompleteType())
return false;
+ // Any type that is, or contains, address discriminated data is non-POD
+ if (getCanonicalType().hasAddressDiscriminatedPointerAuth()) // Context.containsAddressDiscriminatedPointerAuth(*this))
+ return false;
+
// As an extension, Clang treats vector types as Scalar types.
if (BaseTy->isScalarType() || BaseTy->isVectorType())
return true;
diff --git a/clang/test/SemaCXX/ptrauth-type-traits.cpp b/clang/test/SemaCXX/ptrauth-type-traits.cpp
new file mode 100644
index 0000000000000..602ac3790a557
--- /dev/null
+++ b/clang/test/SemaCXX/ptrauth-type-traits.cpp
@@ -0,0 +1,143 @@
+// RUN: %clang_cc1 -triple arm64 -std=c++26 -Wno-deprecated-builtins \
+// RUN: -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple arm64-apple-darwin -fptrauth-calls -fptrauth-intrinsics \
+// RUN: -fptrauth-vtable-pointer-address-discrimination \
+// RUN: -std=c++26 -Wno-deprecated-builtins \
+// RUN: -fsyntax-only -verify %s
+
+// expected-no-diagnostics
+
+#ifdef __PTRAUTH__
+
+#define NonAddressDiscriminatedVTablePtrAttr \
+ [[clang::ptrauth_vtable_pointer(process_independent, no_address_discrimination, no_extra_discrimination)]]
+#define AddressDiscriminatedVTablePtrAttr \
+ [[clang::ptrauth_vtable_pointer(process_independent, address_discrimination, no_extra_discrimination)]]
+#define ADDR_DISC_ENABLED true
+#else
+#define NonAddressDiscriminatedVTablePtrAttr
+#define AddressDiscriminatedVTablePtrAttr
+#define ADDR_DISC_ENABLED false
+#define __ptrauth(...)
+#endif
+
+
+typedef int* __ptrauth(1,1,1) AddressDiscriminatedPtr;
+typedef __UINT64_TYPE__ __ptrauth(1,1,1) AddressDiscriminatedInt64;
+struct AddressDiscriminatedFields {
+ AddressDiscriminatedPtr ptr;
+};
+struct RelocatableAddressDiscriminatedFields trivially_relocatable_if_eligible {
+ AddressDiscriminatedPtr ptr;
+};
+struct AddressDiscriminatedFieldInBaseClass : AddressDiscriminatedFields {
+ void *newfield;
+};
+
+struct NonAddressDiscriminatedVTablePtrAttr NonAddressDiscriminatedVTablePtr {
+ virtual ~NonAddressDiscriminatedVTablePtr();
+ void *i;
+};
+
+struct NonAddressDiscriminatedVTablePtrAttr NonAddressDiscriminatedVTablePtr2 {
+ virtual ~NonAddressDiscriminatedVTablePtr2();
+ void *j;
+};
+
+struct NonAddressDiscriminatedVTablePtrAttr RelocatableNonAddressDiscriminatedVTablePtr trivially_relocatable_if_eligible {
+ virtual ~RelocatableNonAddressDiscriminatedVTablePtr();
+ void *i;
+};
+
+struct NonAddressDiscriminatedVTablePtrAttr RelocatableNonAddressDiscriminatedVTablePtr2 trivially_relocatable_if_eligible {
+ virtual ~RelocatableNonAddressDiscriminatedVTablePtr2();
+ void *j;
+};
+
+struct AddressDiscriminatedVTablePtrAttr AddressDiscriminatedVTablePtr {
+ virtual ~AddressDiscriminatedVTablePtr();
+ void *k;
+};
+
+struct AddressDiscriminatedVTablePtrAttr RelocatableAddressDiscriminatedVTablePtr trivially_relocatable_if_eligible {
+ virtual ~RelocatableAddressDiscriminatedVTablePtr();
+ void *k;
+};
+
+struct NoAddressDiscriminatedBaseClasses : NonAddressDiscriminatedVTablePtr,
+ NonAddressDiscriminatedVTablePtr2 {
+ void *l;
+};
+
+struct RelocatableNoAddressDiscriminatedBaseClasses trivially_relocatable_if_eligible :
+ NonAddressDiscriminatedVTablePtr,
+ NonAddressDiscriminatedVTablePtr2 {
+ void *l;
+};
+
+struct AddressDiscriminatedPrimaryBase : AddressDiscriminatedVTablePtr,
+ NonAddressDiscriminatedVTablePtr {
+ void *l;
+};
+struct AddressDiscriminatedSecondaryBase : NonAddressDiscriminatedVTablePtr,
+ AddressDiscriminatedVTablePtr {
+ void *l;
+};
+
+struct RelocatableAddressDiscriminatedPrimaryBase : RelocatableAddressDiscriminatedVTablePtr,
+ RelocatableNonAddressDiscriminatedVTablePtr {
+ void *l;
+};
+struct RelocatableAddressDiscriminatedSecondaryBase : RelocatableNonAddressDiscriminatedVTablePtr,
+ RelocatableAddressDiscriminatedVTablePtr {
+ void *l;
+};
+struct EmbdeddedAddressDiscriminatedPolymorphicClass {
+ AddressDiscriminatedVTablePtr field;
+};
+struct RelocatableEmbdeddedAddressDiscriminatedPolymorphicClass trivially_relocatable_if_eligible {
+ AddressDiscriminatedVTablePtr field;
+};
+
+#define ASSERT_BUILTIN_EQUALS(Expression, Predicate, Info) \
+ static_assert((Expression) == (([](bool Polymorphic, bool AddrDisc, bool Relocatable, bool HasNonstandardLayout){ return (Predicate); })Info), #Expression);
+ //, #Expression " did not match " #Predicate);
+
+
+#define TEST_BUILTINS(Builtin, Predicate) \
+ ASSERT_BUILTIN_EQUALS(Builtin(AddressDiscriminatedPtr), Predicate, (false, ADDR_DISC_ENABLED, true, false)) \
+ ASSERT_BUILTIN_EQUALS(Builtin(AddressDiscriminatedInt64), Predicate, (false, ADDR_DISC_ENABLED, true, false))\
+ ASSERT_BUILTIN_EQUALS(Builtin(AddressDiscriminatedFields), Predicate, (false, ADDR_DISC_ENABLED, true, false))\
+ ASSERT_BUILTIN_EQUALS(Builtin(RelocatableAddressDiscriminatedFields), Predicate, (false, ADDR_DISC_ENABLED, true, false))\
+ ASSERT_BUILTIN_EQUALS(Builtin(AddressDiscriminatedFieldInBaseClass), Predicate, (false, ADDR_DISC_ENABLED, true, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(NonAddressDiscriminatedVTablePtr), Predicate, (true, false, false, false))\
+ ASSERT_BUILTIN_EQUALS(Builtin(NonAddressDiscriminatedVTablePtr2), Predicate, (true, false, false, false))\
+ ASSERT_BUILTIN_EQUALS(Builtin(RelocatableNonAddressDiscriminatedVTablePtr), Predicate, (true, false, true, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(RelocatableNonAddressDiscriminatedVTablePtr2), Predicate, (true, false, true, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(AddressDiscriminatedVTablePtr), Predicate, (true, ADDR_DISC_ENABLED, false, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(RelocatableAddressDiscriminatedVTablePtr), Predicate, (true, ADDR_DISC_ENABLED, true, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(NoAddressDiscriminatedBaseClasses), Predicate, (true, false, false, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(RelocatableNoAddressDiscriminatedBaseClasses), Predicate, (true, false, false, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(AddressDiscriminatedPrimaryBase), Predicate, (true, ADDR_DISC_ENABLED, false, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(AddressDiscriminatedSecondaryBase), Predicate, (true, ADDR_DISC_ENABLED, false, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(RelocatableAddressDiscriminatedPrimaryBase), Predicate, (true, ADDR_DISC_ENABLED, true, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(RelocatableAddressDiscriminatedSecondaryBase), Predicate, (true, ADDR_DISC_ENABLED, true, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(EmbdeddedAddressDiscriminatedPolymorphicClass), Predicate, (true, ADDR_DISC_ENABLED, false, true))\
+ ASSERT_BUILTIN_EQUALS(Builtin(RelocatableEmbdeddedAddressDiscriminatedPolymorphicClass), Predicate, (true, ADDR_DISC_ENABLED, false, true))
+
+TEST_BUILTINS(__is_pod, !(Polymorphic || AddrDisc || HasNonstandardLayout))
+TEST_BUILTINS(__is_standard_layout, !(Polymorphic || HasNonstandardLayout))
+TEST_BUILTINS(__has_trivial_move_constructor, !(Polymorphic || AddrDisc))
+TEST_BUILTINS(__has_trivial_copy, !(Polymorphic || AddrDisc))
+TEST_BUILTINS(__has_trivial_assign, !(Polymorphic || AddrDisc))
+TEST_BUILTINS(__has_trivial_move_assign, !(Polymorphic || AddrDisc))
+TEST_BUILTINS(__is_trivial, !(Polymorphic || AddrDisc))
+TEST_BUILTINS(__is_trivially_copyable, !(Polymorphic || AddrDisc))
+TEST_BUILTINS(__is_trivially_copyable, !(Polymorphic || AddrDisc))
+TEST_BUILTINS(__is_trivially_relocatable, !((Polymorphic) || AddrDisc))
+TEST_BUILTINS(__builtin_is_cpp_trivially_relocatable, !((Polymorphic && !Relocatable) || AddrDisc))
+TEST_BUILTINS(__builtin_is_replaceable, !(Polymorphic || AddrDisc))
+TEST_BUILTINS(__is_bitwise_cloneable, !AddrDisc);
+
+#define ASSIGNABLE_WRAPPER(Type) __is_trivially_assignable(Type&, Type)
+TEST_BUILTINS(ASSIGNABLE_WRAPPER, !(Polymorphic || AddrDisc))
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
36d7697 to
b0ac8bf
Compare
…copyable A number of builtins report some variation of "this type is compatibile with some bitwise equivalent operation", but this is not true for address discriminated values. We had address a number of cases, but not all of them. This PR corrects the remaining builtins. Fixes #154394
b0ac8bf to
689d70c
Compare
clang/lib/AST/DeclCXX.cpp
Outdated
| if (!data().HasTrivialSpecialMembers && | ||
| T.hasAddressDiscriminatedPointerAuth()) | ||
| data().HasTrivialSpecialMembers = true; |
There was a problem hiding this comment.
isn't that doing the opposite of what we want?
ie a discriminated field should not be trivial
There was a problem hiding this comment.
yes, so the question is why did nothing seem to break (as in what test would this break - it's possible that it gets masked by other pointer auth state)
There was a problem hiding this comment.
That is actually just complete nonsense, and it seems really weird that nothing broke at all.
There was a problem hiding this comment.
I think I put this in while fighting battery life last night, but I'm super annoyed that nothing got triggered warning wise (maybe need to add yet more warnings for preferred type?).
But more annoyingly I can't find a way to make it do the wrong thing - basically what this code ends up doing is saying none of the special member functions are defaulted, which is already going to be true due to the address discriminated value - except the addition of the default constructor which is set arbitrarily. This should make it possible to have a class that has a non-defaulted default constructor be treated as if it were trivially defaulted. But to hit the code paths that actually change behavior due to that you need to get past things like pod type checks (which always fail with address discrimination).
Annoying that something this dumb is wrong, and not caught by the compiler, and cannot be tested because the same state that leads to the error means the error cannot impact anything.
There was a problem hiding this comment.
I think i would prefer that piece of code to remain (fixed!) - I suspect (and you should check) that your call to setNonTrivialToPrimitiveCopy is why this does not break anything.
However, afaict, setNonTrivialToPrimitiveCopy is mostly a C thing, and i think in C++ it would be better to say that a type with a ptrauth member is not trivially copyable (and not trivially assignable, movable, presumably)
There was a problem hiding this comment.
In C we already essentially introduce a copy constructor :D
We could in principle 0 out that field. I may actually just add an assert to that fact as the vtable and/or address discriminated fields should already result in this.
| @@ -74,7 +74,7 @@ static_assert(__is_trivially_destructible(S3)); | |||
| static_assert(!__is_trivially_copyable(S3)); | |||
| static_assert(!__is_trivially_relocatable(S3)); // expected-warning{{deprecated}} | |||
| //FIXME | |||
| static_assert(__builtin_is_cpp_trivially_relocatable(S3)); | |||
| static_assert(!__builtin_is_cpp_trivially_relocatable(S3)); | |||
There was a problem hiding this comment.
Hmm, is this right? My understanding is that, while the pre-standardization concept of trivial relocatability implies bitwise relocation, the standardized concept does not and allows for arbitrary compiler-synthesized logic as long as that logic can't cause arbitrary side-effects. I would expect address-discriminated ptrauth to not disqualify a type from that.
There was a problem hiding this comment.
@rjmccall (I have no idea if pinging like this impacts GH behavior so this is more a test than anything else)
So the issue here is that we weren't sure how to handle explicitly ptrauth qualified values, and then with everything else didn't end up landing the trivially_relocate support at all, and that PR is large enough that I would be worried about landing it in llvm21 at this point.
@cor3ntin and I discussed this and thought that having this return false until we actually supported trivial relocation was the best approach even though it could in principle impact abi when we do enable it.
The trivially_relocate PR is over at #144420 and you can see it is quite a big one.
On the other hand it really only impact types containing address discriminated values so if there were any issues the impact would be extremely restricted, and assuming that the use of explicit pointer auth in code built with llvm21, the most common place it would be encountered is when trivially relocating polymorphic types which is something that I really don't like at all, and wish was not permitted as IMO it's a bigger hazard than copy behavior due to it maintaining the vtable pointer for a potentially truncated object.
There was a problem hiding this comment.
I see. The idea is that it should work, and while we don't support it today, we do intend to in the future? That seems reasonable.
Wait, trivial relocation of a polymorphic object just copies the v-table pointers? That seems obviously incorrect unless there's some semantic reason relocation can only happen on complete objects.
There was a problem hiding this comment.
Wait, trivial relocation of a polymorphic object just copies the v-table pointers? That seems obviously incorrect unless there's some semantic reason relocation can only happen on complete objects.
I also dislike it - but a bunch of people believe that it absolutely must be a memcpy. Allowing a fixup for ptrauth was a struggle as it was.
A non-zero part of me wants it to be an error if there's a constant copy size of one element - on the basis that a variable count implies that you have an array so they must all be the same type.
But yeah, blind copy of the vtable is questionable, but once again just slap a "it's UB to truncate the object" is the solution :-/
There was a problem hiding this comment.
It's possible we could argue against it in Kona - I think @ldionne already has a few problems with it.
There was a problem hiding this comment.
Alright, I'm fine with the plan here.
members Any address discriminated values mean that the type is never trivially copyable or movable.
clang/lib/AST/DeclCXX.cpp
Outdated
| @@ -1438,6 +1438,13 @@ void CXXRecordDecl::addedMember(Decl *D) { | |||
| data().StructuralIfLiteral = false; | |||
| } | |||
|
|
|||
| // If this type contains any address discriminated values we should | |||
| // have already indicated that the only special member functions that | |||
| // can possibly be trivial are the constructor and destructor. | |||
There was a problem hiding this comment.
| // can possibly be trivial are the constructor and destructor. | |
| // can possibly be trivial are the default constructor and destructor. |
| @@ -74,7 +74,7 @@ static_assert(__is_trivially_destructible(S3)); | |||
| static_assert(!__is_trivially_copyable(S3)); | |||
| static_assert(!__is_trivially_relocatable(S3)); // expected-warning{{deprecated}} | |||
| //FIXME | |||
| static_assert(__builtin_is_cpp_trivially_relocatable(S3)); | |||
| static_assert(!__builtin_is_cpp_trivially_relocatable(S3)); | |||
There was a problem hiding this comment.
Alright, I'm fine with the plan here.
…bitwise compatible (#154490) A number of builtins report some variation of "this type is compatibile with some bitwise equivalent operation", but this is not true for address discriminated values. We had address a number of cases, but not all of them. This PR corrects the remaining builtins. Fixes #154394 (cherry picked from commit d66b537)
…bitwise compatible (#154490) A number of builtins report some variation of "this type is compatibile with some bitwise equivalent operation", but this is not true for address discriminated values. We had address a number of cases, but not all of them. This PR corrects the remaining builtins. Fixes #154394
A number of builtins report some variation of "this type is compatibile with some bitwise equivalent operation", but this is not true for address discriminated values. We had address a number of cases, but not all of them. This PR corrects the remaining builtins.
Fixes #154394