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
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
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

@pcc pcc Sep 4, 2025

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
4 changes: 4 additions & 0 deletions clang/lib/AST/DeclCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1438,6 +1438,10 @@ void CXXRecordDecl::addedMember(Decl *D) {
data().StructuralIfLiteral = false;
}

if (!data().HasTrivialSpecialMembers &&
T.hasAddressDiscriminatedPointerAuth())
data().HasTrivialSpecialMembers = true;
Copy link
Contributor

@cor3ntin cor3ntin Aug 21, 2025

Choose a reason for hiding this comment

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

isn't that doing the opposite of what we want?
ie a discriminated field should not be trivial

Copy link
Contributor Author

@ojhunt ojhunt Aug 21, 2025

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@ojhunt ojhunt Aug 21, 2025

Choose a reason for hiding this comment

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

That is actually just complete nonsense, and it seems really weird that nothing broke at all.

Copy link
Contributor Author

@ojhunt ojhunt Aug 21, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@cor3ntin cor3ntin Aug 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@ojhunt ojhunt Aug 23, 2025

Choose a reason for hiding this comment

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

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.


// C++14 [meta.unary.prop]p4:
// T is a class type [...] with [...] no non-static data members other
// than subobjects of zero size
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

@rjmccall rjmccall Aug 22, 2025

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

@ojhunt ojhunt Aug 22, 2025

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

@rjmccall rjmccall Aug 22, 2025

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

@ojhunt ojhunt Aug 22, 2025

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

@ojhunt ojhunt Aug 22, 2025

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

@rjmccall rjmccall Aug 26, 2025

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