-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang] Fix PointerAuth semantics of cpp_trivially_relocatable #143969
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.
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
Changes from 2 commits
f476803
3190a52
15b1a81
3ba1e39
6b5de16
ea3af8a
54f93cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1705,6 +1705,41 @@ void ASTContext::setRelocationInfoForCXXRecord( | |||||
| RelocatableClasses.insert({D, Info}); | ||||||
| } | ||||||
|
|
||||||
| bool ASTContext::containsAddressDiscriminatedPointerAuth(QualType T) { | ||||||
| if (!isPointerAuthenticationAvailable()) | ||||||
| return false; | ||||||
|
|
||||||
| T = T.getCanonicalType(); | ||||||
| if (T.hasAddressDiscriminatedPointerAuth()) | ||||||
| return true; | ||||||
| const RecordDecl *RD = T->getAsRecordDecl(); | ||||||
| if (!RD) | ||||||
| return false; | ||||||
|
|
||||||
| if (auto Existing = RecordContainsAddressDiscriminatedPointerAuth.find(RD); | ||||||
| Existing != RecordContainsAddressDiscriminatedPointerAuth.end()) | ||||||
| return Existing->second; | ||||||
|
|
||||||
| auto SaveReturn = [this, RD](bool Result) { | ||||||
| RecordContainsAddressDiscriminatedPointerAuth.insert({RD, Result}); | ||||||
|
||||||
| RecordContainsAddressDiscriminatedPointerAuth.insert({RD, Result}); | |
| RecordContainsAddressDiscriminatedPointerAuth.emplace(RD, Result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move SaveReturn after the map lookup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm's map does not have emplace afaict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hum, sorry, try_emplace
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that can just be a (static) function declared in this file, taking ASTContext as parameter - it should not be called directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasAddressDiscriminatedVTableAuthentication
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| // RUN: %clang_cc1 -triple arm64 -fptrauth-calls -fptrauth-intrinsics -std=c++26 -verify %s | ||
|
|
||
| // This test intentionally does not enable the global address discrimination | ||
| // of vtable pointers. This lets us configure them with different schemas | ||
| // and verify that we're correctly tracking the existence of address discrimination | ||
|
|
||
| // expected-no-diagnostics | ||
|
|
||
| struct NonAddressDiscPtrauth { | ||
| void * __ptrauth(1, 0, 1234) p; | ||
| }; | ||
|
|
||
| static_assert(__builtin_is_cpp_trivially_relocatable(NonAddressDiscPtrauth)); | ||
|
|
||
| struct AddressDiscPtrauth { | ||
| void * __ptrauth(1, 1, 1234) p; | ||
| }; | ||
|
|
||
| static_assert(__builtin_is_cpp_trivially_relocatable(AddressDiscPtrauth)); | ||
|
|
||
| struct MultipleBaseClasses : NonAddressDiscPtrauth, AddressDiscPtrauth { | ||
|
|
||
| }; | ||
|
|
||
| static_assert(__builtin_is_cpp_trivially_relocatable(MultipleBaseClasses)); | ||
|
|
||
| struct MultipleMembers { | ||
| NonAddressDiscPtrauth field0; | ||
| AddressDiscPtrauth field1; | ||
| }; | ||
|
|
||
| static_assert(__builtin_is_cpp_trivially_relocatable(MultipleMembers)); | ||
|
|
||
| struct UnionOfPtrauth { | ||
| union { | ||
| NonAddressDiscPtrauth field0; | ||
| AddressDiscPtrauth field1; | ||
| } u; | ||
| }; | ||
|
|
||
| static_assert(!__builtin_is_cpp_trivially_relocatable(UnionOfPtrauth)); | ||
|
|
||
| struct [[clang::ptrauth_vtable_pointer(process_independent,address_discrimination,no_extra_discrimination)]] Polymorphic trivially_relocatable_if_eligible { | ||
| virtual ~Polymorphic(); | ||
| }; | ||
|
|
||
| struct Foo : Polymorphic { | ||
| Foo(const Foo&); | ||
| ~Foo(); | ||
| }; | ||
|
|
||
|
|
||
| static_assert(__builtin_is_cpp_trivially_relocatable(Polymorphic)); | ||
|
|
||
| struct [[clang::ptrauth_vtable_pointer(process_independent,no_address_discrimination,no_extra_discrimination)]] NonAddressDiscriminatedPolymorphic trivially_relocatable_if_eligible { | ||
| virtual ~NonAddressDiscriminatedPolymorphic(); | ||
| }; | ||
|
|
||
| static_assert(__builtin_is_cpp_trivially_relocatable(NonAddressDiscriminatedPolymorphic)); | ||
|
|
||
|
|
||
| struct PolymorphicMembers { | ||
| Polymorphic field; | ||
| }; | ||
|
|
||
| static_assert(__builtin_is_cpp_trivially_relocatable(PolymorphicMembers)); | ||
|
|
||
| struct UnionOfPolymorphic { | ||
| union trivially_relocatable_if_eligible { | ||
| Polymorphic p; | ||
| int i; | ||
| } u; | ||
| }; | ||
|
|
||
| static_assert(!__builtin_is_cpp_trivially_relocatable(UnionOfPolymorphic)); | ||
|
|
||
|
|
||
| struct UnionOfNonAddressDiscriminatedPolymorphic { | ||
| union trivially_relocatable_if_eligible { | ||
| NonAddressDiscriminatedPolymorphic p; | ||
| int i; | ||
| } u; | ||
| }; | ||
| static_assert(!__builtin_is_cpp_trivially_relocatable(UnionOfNonAddressDiscriminatedPolymorphic)); | ||
|
|
||
| struct UnionOfNonAddressDiscriminatedPtrauth { | ||
| union { | ||
| NonAddressDiscPtrauth p; | ||
| int i; | ||
| } u; | ||
| }; | ||
|
|
||
| static_assert(__builtin_is_cpp_trivially_relocatable(UnionOfNonAddressDiscriminatedPtrauth)); | ||
|
|
||
| struct UnionOfAddressDisriminatedPtrauth { | ||
| union { | ||
| AddressDiscPtrauth p; | ||
| int i; | ||
| } u; | ||
| }; | ||
|
|
||
| static_assert(!__builtin_is_cpp_trivially_relocatable(UnionOfAddressDisriminatedPtrauth)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 questions; Why do we have 3 options, and why do we care about anything but
PointerAuthVTPtrAddressDiscriminationhere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first is historical sadness, we can probably make this better when I move the pointer auth options and similar into the target info rather than the obtuse combination of codegen and langopts we currently have. When we do that we can probably just have a single "the target supports pointer auth" flag which is what this is trying to approximate.
The reason that
PointerAuthVTPtrAddressDiscriminationis not sufficient is that there is a qualifier that can be specified explicitly.That said as we discussed (for people following at home @cor3ntin and I are talking at wg21) if someone had
PointerAuthVTPtrAddressDiscriminationenabled withoutPointerAuthCallsandPointerAuthIntrinsicswould likely be in a world of hurt, so I'll remove that separate check.