Skip to content

[clang][PAC] Fix builtins that claim address discriminated types are bitwise compatible #154490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 26, 2025
+448 −16
Merged

[clang][PAC] Fix builtins that claim address discriminated types are bitwise compatible #154490

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
689d70c
[clang][PAC] Fix builtins that claim Address discriminated types are …
ojhunt Aug 20, 2025
5c4a30b
Remove dumb code
ojhunt Aug 21, 2025
18f035b
Explicitly acknowled the impact of pointer auth on non-trivial
ojhunt Aug 24, 2025
587864c
Add a .
ojhunt Aug 25, 2025
fb069dd
Add comment correction from Joggn
ojhunt Aug 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions clang/include/clang/AST/ASTContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,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;
Expand All @@ -656,8 +656,7 @@ class ASTContext : public RefCountedBase<ASTContext> {
bool containsNonRelocatablePointerAuth(QualType T) {
if (!isPointerAuthenticationAvailable())
return false;
return findPointerAuthContent(T) ==
PointerAuthContent::AddressDiscriminatedData;
return findPointerAuthContent(T) != PointerAuthContent::None;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this function and containsAddressDiscriminatedPointerAuth now do the same thing, can we remove one of them?

}

private:
Expand All @@ -675,8 +674,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;
Expand Down Expand Up @@ -3723,7 +3722,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);
Expand Down
9 changes: 5 additions & 4 deletions clang/lib/AST/ASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -1723,7 +1723,8 @@ 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();
Expand Down Expand Up @@ -3040,7 +3041,7 @@ bool ASTContext::hasUniqueObjectRepresentations(
return true;
}

// All other pointers (except __ptrauth pointers) are unique.
// All other pointers are unique.
if (Ty->isPointerType())
return !Ty.hasAddressDiscriminatedPointerAuth();

Expand Down Expand Up @@ -15203,7 +15204,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;
Expand Down
20 changes: 20 additions & 0 deletions clang/lib/AST/Type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 never POD.
if (Context.containsAddressDiscriminatedPointerAuth(CanonicalType))
return false;

switch (CanonicalType->getTypeClass()) {
// Everything not explicitly mentioned is not POD.
default:
Expand Down Expand Up @@ -2829,6 +2834,11 @@ bool QualType::isTrivialType(const ASTContext &Context) const {
if (CanonicalType->isDependentType())
return false;

// Any type that is, or contains, address discriminated data is never a
// trivial type.
if (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
Expand Down Expand Up @@ -2930,6 +2940,12 @@ bool QualType::isBitwiseCloneableType(const ASTContext &Context) const {

if (CanonicalType->isIncompleteType())
return false;

// Any type that is, or contains, address discriminated data is never
// bitwise clonable.
if (Context.containsAddressDiscriminatedPointerAuth(CanonicalType))
return false;

const auto *RD = CanonicalType->getAsRecordDecl(); // struct/union/class
if (!RD)
return true;
Expand Down Expand Up @@ -3179,6 +3195,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 (Context.containsAddressDiscriminatedPointerAuth(*this))
return false;

// As an extension, Clang treats vector types as Scalar types.
if (BaseTy->isScalarType() || BaseTy->isVectorType())
return true;
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19779,6 +19779,7 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,
Q && Q.isAddressDiscriminated()) {
Record->setArgPassingRestrictions(
RecordArgPassingKind::CanNeverPassInRegs);
Record->setNonTrivialToPrimitiveCopy(true);
}
}

Expand Down
5 changes: 4 additions & 1 deletion clang/lib/Sema/SemaTypeTraits.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1766,7 +1766,10 @@ static bool EvaluateBinaryTypeTrait(Sema &Self, TypeTrait BTT,
// Objective-C lifetime, this is a non-trivial assignment.
if (LhsT.getNonReferenceType().hasNonTrivialObjCLifetime())
return false;

const ASTContext &Context = Self.getASTContext();
if (Context.containsAddressDiscriminatedPointerAuth(LhsT) ||
Context.containsAddressDiscriminatedPointerAuth(RhsT))
return false;
return !Result.get()->hasNonTrivialCall(Self.Context);
}

Expand Down
6 changes: 3 additions & 3 deletions clang/test/SemaCXX/ptrauth-triviality.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible we could argue against it in Kona - I think @ldionne already has a few problems with it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'm fine with the plan here.

static_assert(!__is_trivially_equality_comparable(S3));


Expand All @@ -84,7 +84,7 @@ static_assert(!__is_trivially_assignable(Holder<S3>, const Holder<S3>&));
static_assert(__is_trivially_destructible(Holder<S3>));
static_assert(!__is_trivially_copyable(Holder<S3>));
static_assert(!__is_trivially_relocatable(Holder<S3>)); // expected-warning{{deprecated}}
static_assert(__builtin_is_cpp_trivially_relocatable(Holder<S3>));
static_assert(!__builtin_is_cpp_trivially_relocatable(Holder<S3>));
static_assert(!__is_trivially_equality_comparable(Holder<S3>));

struct IA S4 {
Expand Down Expand Up @@ -207,7 +207,7 @@ template <class T> struct UnionWrapper trivially_relocatable_if_eligible {
} u;
};

static_assert(test_is_trivially_relocatable_v<AddressDiscriminatedPolymorphicBase>);
static_assert(!test_is_trivially_relocatable_v<AddressDiscriminatedPolymorphicBase>);
static_assert(test_is_trivially_relocatable_v<NoAddressDiscriminatedPolymorphicBase>);
static_assert(inheritance_relocatability_matches_bases_v<AddressDiscriminatedPolymorphicBase, NoAddressDiscriminatedPolymorphicBase>);
static_assert(inheritance_relocatability_matches_bases_v<NoAddressDiscriminatedPolymorphicBase, AddressDiscriminatedPolymorphicBase>);
Expand Down
Loading