Skip to content

[clang] Fix PointerAuth semantics of cpp_trivially_relocatable #143796

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

Closed
wants to merge 3 commits into from
Closed

[clang] Fix PointerAuth semantics of cpp_trivially_relocatable #143796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4065271
[clang] Fix PointerAuth semantics of __builtin_is_cpp_trivially_reloc…
ojhunt Jun 11, 2025
fed1625
formatting
ojhunt Jun 11, 2025
ab3d7e8
Separating the cache for the existence of address discrimination
ojhunt Jun 12, 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
10 changes: 9 additions & 1 deletion clang/include/clang/AST/ASTContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,17 +621,24 @@ class ASTContext : public RefCountedBase<ASTContext> {

public:
struct CXXRecordDeclRelocationInfo {
static CXXRecordDeclRelocationInfo NonRelocatable() {
return {false, false};
}

unsigned IsRelocatable;
unsigned IsReplaceable;
Comment on lines 628 to 629
Copy link
Contributor

@cor3ntin cor3ntin Jun 12, 2025

Choose a reason for hiding this comment

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

Why are these not bool? bad, bad @cor3ntin

Copy link
Contributor Author

@ojhunt ojhunt Jun 12, 2025

Choose a reason for hiding this comment

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

I did wonder, but I assumed you had Reasons(tm)

};
std::optional<CXXRecordDeclRelocationInfo>
getRelocationInfoForCXXRecord(const CXXRecordDecl *) const;
void setRelocationInfoForCXXRecord(const CXXRecordDecl *,
CXXRecordDeclRelocationInfo);
const CXXRecordDeclRelocationInfo &);
bool containsAddressDiscriminatedPointerAuth(QualType T);

private:
llvm::DenseMap<const CXXRecordDecl *, CXXRecordDeclRelocationInfo>
RelocatableClasses;
llvm::DenseMap<const RecordDecl *, bool>
RecordContainsAddressDiscriminatedPointerAuth;

ImportDecl *FirstLocalImport = nullptr;
ImportDecl *LastLocalImport = nullptr;
Expand Down Expand Up @@ -3668,6 +3675,7 @@ OPT_LIST(V)
/// authentication policy for the specified record.
const CXXRecordDecl *
baseForVTableAuthentication(const CXXRecordDecl *ThisClass);
bool hasAddressDiscriminatedVTableAuthentication(const CXXRecordDecl *Class);
bool useAbbreviatedThunkName(GlobalDecl VirtualMethodDecl,
StringRef MangledName);

Expand Down
5 changes: 3 additions & 2 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4343,8 +4343,10 @@ class Sema final : public SemaBase {
void ActOnTagFinishDefinition(Scope *S, Decl *TagDecl,
SourceRange BraceRange);

ASTContext::CXXRecordDeclRelocationInfo
using CXXRecordDeclRelocationInfo = ASTContext::CXXRecordDeclRelocationInfo;
CXXRecordDeclRelocationInfo
CheckCXX2CRelocatableAndReplaceable(const clang::CXXRecordDecl *D);
CXXRecordDeclRelocationInfo CheckCXX2CRelocatableAndReplaceable(QualType T);

void ActOnTagFinishSkippedDefinition(SkippedDefinitionContext Context);

Expand Down Expand Up @@ -8680,7 +8682,6 @@ class Sema final : public SemaBase {
// FIXME: This is in Sema because it requires
// overload resolution, can we move to ASTContext?
bool IsCXXTriviallyRelocatableType(QualType T);
bool IsCXXTriviallyRelocatableType(const CXXRecordDecl &RD);

//// Determines if a type is replaceable
/// according to the C++26 rules.
Expand Down
51 changes: 50 additions & 1 deletion clang/lib/AST/ASTContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1698,13 +1698,47 @@ ASTContext::getRelocationInfoForCXXRecord(const CXXRecordDecl *RD) const {
}

void ASTContext::setRelocationInfoForCXXRecord(
const CXXRecordDecl *RD, CXXRecordDeclRelocationInfo Info) {
const CXXRecordDecl *RD, const CXXRecordDeclRelocationInfo &Info) {
assert(RD);
CXXRecordDecl *D = RD->getDefinition();
assert(RelocatableClasses.find(D) == RelocatableClasses.end());
RelocatableClasses.insert({D, Info});
}

bool ASTContext::containsAddressDiscriminatedPointerAuth(QualType T) {
if (!LangOpts.PointerAuthCalls && !LangOpts.PointerAuthIntrinsics)
return false;

T = T.getCanonicalType();
if (T.hasAddressDiscriminatedPointerAuth())
return true;
const RecordDecl *RD = T->getAsRecordDecl();
if (!RD)
return false;

auto SaveReturn = [this, RD](bool Result) {
RecordContainsAddressDiscriminatedPointerAuth.insert({RD, Result});
return Result;
};
if (auto Existing = RecordContainsAddressDiscriminatedPointerAuth.find(RD);
Existing != RecordContainsAddressDiscriminatedPointerAuth.end())
return Existing->second;
if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
if (CXXRD->isPolymorphic() &&
hasAddressDiscriminatedVTableAuthentication(CXXRD))
return SaveReturn(true);
for (auto Base : CXXRD->bases()) {
if (containsAddressDiscriminatedPointerAuth(Base.getType()))
return SaveReturn(true);
}
}
for (auto *FieldDecl : RD->fields()) {
if (containsAddressDiscriminatedPointerAuth(FieldDecl->getType()))
return SaveReturn(true);
}
Comment on lines +1726 to +1738
Copy link
Contributor

@cor3ntin cor3ntin Jun 12, 2025

Choose a reason for hiding this comment

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

Do we care about the C case at all? (ie, the case where a RecordDecl is not a CXXRecord decl?)
C support so far is not something I considered

Copy link
Contributor

@cor3ntin cor3ntin Jun 12, 2025

Choose a reason for hiding this comment

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

Establishing relocatability needs to do lookups, so it's expensive - this is why we only compute it on demand.
However, for "containsAddressDiscriminatedPointerAuth", maybe that's something we can establish as we build the class, and have the result stored in DefinitionData. Is that something you considered?

Copy link
Contributor

@cor3ntin cor3ntin Jun 12, 2025

Choose a reason for hiding this comment

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

I think it's reasonable to cache the result in a map, and optimize later.
Maybe we can make it work for C++ now, and open an issue for C ?

return SaveReturn(false);
}

void ASTContext::addedLocalImportDecl(ImportDecl *Import) {
assert(!Import->getNextLocalImport() &&
"Import declaration already in the chain");
Expand Down Expand Up @@ -15121,6 +15155,21 @@ ASTContext::baseForVTableAuthentication(const CXXRecordDecl *ThisClass) {
return PrimaryBase;
}

bool ASTContext::hasAddressDiscriminatedVTableAuthentication(
const CXXRecordDecl *Class) {
assert(Class->isPolymorphic());
const CXXRecordDecl *BaseType = baseForVTableAuthentication(Class);
using AuthAttr = VTablePointerAuthenticationAttr;
const auto *ExplicitAuth = BaseType->getAttr<AuthAttr>();
if (!ExplicitAuth)
return LangOpts.PointerAuthVTPtrAddressDiscrimination;
AuthAttr::AddressDiscriminationMode AddressDiscrimination =
ExplicitAuth->getAddressDiscrimination();
if (AddressDiscrimination == AuthAttr::DefaultAddressDiscrimination)
return LangOpts.PointerAuthVTPtrAddressDiscrimination;
return AddressDiscrimination == AuthAttr::AddressDiscrimination;
}

bool ASTContext::useAbbreviatedThunkName(GlobalDecl VirtualMethodDecl,
StringRef MangledName) {
auto *Method = cast<CXXMethodDecl>(VirtualMethodDecl.getDecl());
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaDeclCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10615,7 +10615,7 @@ void Sema::checkIllFormedTrivialABIStruct(CXXRecordDecl &RD) {
}
}

if (IsCXXTriviallyRelocatableType(RD))
if (CheckCXX2CRelocatableAndReplaceable(&RD).IsRelocatable)
Copy link
Contributor

@cor3ntin cor3ntin Jun 12, 2025

Choose a reason for hiding this comment

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

I think we should keep that interface even if we deleguate to something under the hood

Copy link
Contributor Author

@ojhunt ojhunt Jun 12, 2025

Choose a reason for hiding this comment

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

I think I did restore it for QualType and was too lazy to add the RD wrapper as well

return;

// Ill-formed if the copy and move constructors are deleted.
Expand Down
170 changes: 116 additions & 54 deletions clang/lib/Sema/SemaTypeTraits.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,10 +228,11 @@ static bool IsEligibleForReplacement(Sema &SemaRef, const CXXRecordDecl *D) {

ASTContext::CXXRecordDeclRelocationInfo
Sema::CheckCXX2CRelocatableAndReplaceable(const CXXRecordDecl *D) {
ASTContext::CXXRecordDeclRelocationInfo Info{false, false};
if (auto ExistingInfo = Context.getRelocationInfoForCXXRecord(D))
return *ExistingInfo;

if (!getLangOpts().CPlusPlus || D->isInvalidDecl())
return Info;
return CXXRecordDeclRelocationInfo::NonRelocatable();

assert(D->hasDefinition());

Expand All @@ -244,7 +245,7 @@ Sema::CheckCXX2CRelocatableAndReplaceable(const CXXRecordDecl *D) {
*this, D, /*AllowUserDefined=*/true);
};

auto IsUnion = [&, Is = std::optional<bool>{}]() mutable {
auto IsTrivialUnion = [&, Is = std::optional<bool>{}]() mutable {
if (!Is.has_value())
Is = D->isUnion() && !D->hasUserDeclaredCopyConstructor() &&
!D->hasUserDeclaredCopyAssignment() &&
Expand All @@ -259,6 +260,7 @@ Sema::CheckCXX2CRelocatableAndReplaceable(const CXXRecordDecl *D) {
return *Is;
};

ASTContext::CXXRecordDeclRelocationInfo Info;
Info.IsRelocatable = [&] {
if (D->isDependentType())
return false;
Expand All @@ -272,7 +274,7 @@ Sema::CheckCXX2CRelocatableAndReplaceable(const CXXRecordDecl *D) {
return true;

// is a union with no user-declared special member functions, or
if (IsUnion())
if (IsTrivialUnion())
return true;

// is default-movable.
Expand All @@ -292,77 +294,129 @@ Sema::CheckCXX2CRelocatableAndReplaceable(const CXXRecordDecl *D) {
return HasSuitableSMP();

// is a union with no user-declared special member functions, or
if (IsUnion())
if (IsTrivialUnion())
return HasSuitableSMP();

// is default-movable.
return IsDefaultMovable();
}();

bool PtrauthMatters = LangOpts.PointerAuthIntrinsics ||
LangOpts.PointerAuthVTPtrAddressDiscrimination;
if (PtrauthMatters) {
bool IsUnion = D->isUnion();
auto RecordPointerAuth = [&](bool HasAddressDiscrimination) {
if (HasAddressDiscrimination && IsUnion) {
Info.IsRelocatable = false;
Info.IsReplaceable = false;
}
};
auto IsBottomRelocationInfo = [](const CXXRecordDeclRelocationInfo &Info) {
Copy link
Contributor

@cor3ntin cor3ntin Jun 12, 2025

Choose a reason for hiding this comment

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

This name does not really speak to me

Copy link
Contributor Author

@ojhunt ojhunt Jun 12, 2025

Choose a reason for hiding this comment

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

Yeah, I really wasn't sure what to call it

return !Info.IsReplaceable && !Info.IsRelocatable;
};

if (D->isPolymorphic())
RecordPointerAuth(Context.hasAddressDiscriminatedVTableAuthentication(D));
for (auto Base : D->bases()) {
if (IsBottomRelocationInfo(Info))
break;
bool BaseHasPtrauth =
Context.containsAddressDiscriminatedPointerAuth(Base.getType());
RecordPointerAuth(BaseHasPtrauth);
}
for (auto *FieldDecl : D->fields()) {
if (IsBottomRelocationInfo(Info))
break;
bool FieldHasPtrauth =
Context.containsAddressDiscriminatedPointerAuth(FieldDecl->getType());
RecordPointerAuth(FieldHasPtrauth);
}
}
Context.setRelocationInfoForCXXRecord(D, Info);
return Info;
}

bool Sema::IsCXXTriviallyRelocatableType(const CXXRecordDecl &RD) {
if (std::optional<ASTContext::CXXRecordDeclRelocationInfo> Info =
getASTContext().getRelocationInfoForCXXRecord(&RD))
return Info->IsRelocatable;
ASTContext::CXXRecordDeclRelocationInfo Info =
CheckCXX2CRelocatableAndReplaceable(&RD);
getASTContext().setRelocationInfoForCXXRecord(&RD, Info);
return Info.IsRelocatable;
}
ASTContext::CXXRecordDeclRelocationInfo
Sema::CheckCXX2CRelocatableAndReplaceable(QualType T) {
T = T.getCanonicalType();
enum class DirectRelocationInformation { Yes, No, Unknown };
DirectRelocationInformation Relocatable =
DirectRelocationInformation::Unknown;
DirectRelocationInformation Replaceable =
DirectRelocationInformation::Unknown;
DirectRelocationInformation ContainsAddressDiscriminatedValues =
DirectRelocationInformation::Unknown;

auto UpdateRelocatable = [&](DirectRelocationInformation DRI) {
if (Relocatable == DirectRelocationInformation::Unknown ||
Relocatable == DirectRelocationInformation::Yes)
Relocatable = DRI;
};
auto UpdateReplaceable = [&](DirectRelocationInformation DRI) {
if (Replaceable == DirectRelocationInformation::Unknown ||
Replaceable == DirectRelocationInformation::Yes)
Replaceable = DRI;
};
auto UpdateAddressDiscrimination = [&](DirectRelocationInformation DRI) {
if (ContainsAddressDiscriminatedValues == DirectRelocationInformation::Yes)
Replaceable = DirectRelocationInformation::No;
};

bool Sema::IsCXXTriviallyRelocatableType(QualType Type) {
if (T->isVariableArrayType()) {
UpdateRelocatable(DirectRelocationInformation::No);
UpdateReplaceable(DirectRelocationInformation::No);
}

QualType BaseElementType = getASTContext().getBaseElementType(Type);
if (T.isConstQualified() || T.isVolatileQualified())
UpdateReplaceable(DirectRelocationInformation::No);

if (Type->isVariableArrayType())
return false;
if (T.hasAddressDiscriminatedPointerAuth())
UpdateAddressDiscrimination(DirectRelocationInformation::Yes);

QualType BaseElementType =
SemaRef.getASTContext().getBaseElementType(T.getUnqualifiedType());

if (BaseElementType->isIncompleteType()) {
Relocatable = DirectRelocationInformation::No;
Replaceable = DirectRelocationInformation::No;
}

if (BaseElementType.hasNonTrivialObjCLifetime())
return false;
UpdateRelocatable(DirectRelocationInformation::No);

if (BaseElementType.hasAddressDiscriminatedPointerAuth())
return false;
if (BaseElementType->isScalarType()) {
UpdateRelocatable(DirectRelocationInformation::Yes);
UpdateReplaceable(DirectRelocationInformation::Yes);
UpdateAddressDiscrimination(DirectRelocationInformation::No);
}

if (BaseElementType->isIncompleteType())
return false;
if (BaseElementType->isVectorType())
UpdateRelocatable(DirectRelocationInformation::Yes);

if (BaseElementType->isScalarType() || BaseElementType->isVectorType())
return true;
auto CreateInfo = [=]() -> CXXRecordDeclRelocationInfo {
return {Relocatable == DirectRelocationInformation::Yes,
Replaceable == DirectRelocationInformation::Yes};
};

if (const auto *RD = BaseElementType->getAsCXXRecordDecl())
return IsCXXTriviallyRelocatableType(*RD);
if (BaseElementType->isIncompleteType())
return CreateInfo();

return false;
const CXXRecordDecl *RD = BaseElementType->getAsCXXRecordDecl();
if (!RD)
return CreateInfo();

CXXRecordDeclRelocationInfo Info = CheckCXX2CRelocatableAndReplaceable(RD);
Info.IsRelocatable &= Relocatable != DirectRelocationInformation::No;
Info.IsReplaceable &= Replaceable != DirectRelocationInformation::No;
return Info;
Comment on lines +340 to +411
Copy link
Contributor

@cor3ntin cor3ntin Jun 12, 2025

Choose a reason for hiding this comment

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

Github ate my comment earlier.
I'm not convinced that trying to merge replaceable and relocatable improves readability.
Especially given we compute them lazily, and I suspect needing to know if a type is replaceable will be fairly less frequent. I think having to (mentally) track these 3 tribools is not easy and we don;t really know how these properties will diverge over time.

So I would prefer they remain separate for the time being.

Copy link
Contributor Author

@ojhunt ojhunt Jun 12, 2025

Choose a reason for hiding this comment

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

I think I agree - when the merging happened I though the same logic also needed to track pointer auth characteristics, but later on I separated that into its own separate query via the context, which calls the earlier merge into question.

As noted on discord I may just revert Sema.h and SemaTypeTraits.cpp and just add the required union checks now that the context checks are an option

}

static bool IsCXXReplaceableType(Sema &S, const CXXRecordDecl *RD) {
if (std::optional<ASTContext::CXXRecordDeclRelocationInfo> Info =
S.getASTContext().getRelocationInfoForCXXRecord(RD))
return Info->IsReplaceable;
ASTContext::CXXRecordDeclRelocationInfo Info =
S.CheckCXX2CRelocatableAndReplaceable(RD);
S.getASTContext().setRelocationInfoForCXXRecord(RD, Info);
return Info.IsReplaceable;
bool Sema::IsCXXTriviallyRelocatableType(QualType Type) {
return CheckCXX2CRelocatableAndReplaceable(Type).IsRelocatable;
}

bool Sema::IsCXXReplaceableType(QualType Type) {
if (Type.isConstQualified() || Type.isVolatileQualified())
return false;

if (Type->isVariableArrayType())
return false;

QualType BaseElementType =
getASTContext().getBaseElementType(Type.getUnqualifiedType());
if (BaseElementType->isIncompleteType())
return false;
if (BaseElementType->isScalarType())
return true;
if (const auto *RD = BaseElementType->getAsCXXRecordDecl())
return ::IsCXXReplaceableType(*this, RD);
return false;
return CheckCXX2CRelocatableAndReplaceable(Type).IsReplaceable;
}

/// Checks that type T is not a VLA.
Expand Down Expand Up @@ -663,7 +717,8 @@ static bool isTriviallyEqualityComparableType(Sema &S, QualType Type,
}

static bool IsTriviallyRelocatableType(Sema &SemaRef, QualType T) {
QualType BaseElementType = SemaRef.getASTContext().getBaseElementType(T);
ASTContext &Ctx = SemaRef.getASTContext();
QualType BaseElementType = Ctx.getBaseElementType(T);

if (BaseElementType->isIncompleteType())
return false;
Expand All @@ -673,9 +728,16 @@ static bool IsTriviallyRelocatableType(Sema &SemaRef, QualType T) {
if (T.hasAddressDiscriminatedPointerAuth())
return false;

if (Ctx.containsAddressDiscriminatedPointerAuth(BaseElementType))
return false;

if (const auto *RD = BaseElementType->getAsCXXRecordDecl();
RD && !RD->isPolymorphic() && SemaRef.IsCXXTriviallyRelocatableType(*RD))
return true;
RD && !RD->isPolymorphic()) {
ASTContext::CXXRecordDeclRelocationInfo Info =
SemaRef.CheckCXX2CRelocatableAndReplaceable(RD);
if (Info.IsRelocatable)
return true;
}

if (const auto *RD = BaseElementType->getAsRecordDecl())
return RD->canPassInRegisters();
Expand Down
1 change: 1 addition & 0 deletions clang/test/SemaCXX/cxx2c-trivially-relocatable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -std=c++2c -verify %s
// RUN: %clang_cc1 -triple aarch64-linux-gnu -fptrauth-intrinsics -fptrauth-calls -std=c++2c -verify %s

class Trivial {};
static_assert(__builtin_is_cpp_trivially_relocatable(Trivial));
Expand Down
Loading
Loading