[PAC][clang] Define PointerAuthQualifier and PointerAuthenticationMode
#84384
Conversation
|
I've left 4 commits to distinguish code coming from different sources during Commits from @ahmedbougacha's repo are taken from the eng/arm64e-upstream-llvmorg branch. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Includes changes from the following commits from the branch https://github.com/ahmedbougacha/llvm-project/tree/eng/arm64e-upstream-llvmorg - Initial support ahmedbougacha@cc7ba7e - ObjC isa signing ahmedbougacha@c9ce0d4 Also applies a fix from access-softek/llvm-project#75 Co-authored-by: Ahmed Bougacha <[email protected]>
kovdan01
force-pushed
the
clang-pointer-auth-basic-declarations
branch
from
0b75c0a to
cbfde21
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Daniil Kovalev (kovdan01) ChangesThis brings declarations of Co-authored-by: Ahmed Bougacha <[email protected]> Full diff: https://github.com/llvm/llvm-project/pull/84384.diff 6 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index ff6b64c7f72d57..4fd214592d1f27 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -2182,6 +2182,16 @@ class ASTContext : public RefCountedBase<ASTContext> {
return getQualifiedType(type.getUnqualifiedType(), Qs);
}
+ /// \brief Return a type with the given __ptrauth qualifier.
+ QualType getPointerAuthType(QualType type, PointerAuthQualifier pointerAuth) {
+ assert(!type.getPointerAuth());
+ assert(pointerAuth);
+
+ Qualifiers qs;
+ qs.setPointerAuth(pointerAuth);
+ return getQualifiedType(type, qs);
+ }
+
unsigned char getFixedPointScale(QualType Ty) const;
unsigned char getFixedPointIBits(QualType Ty) const;
llvm::FixedPointSemantics getFixedPointSemantics(QualType Ty) const;
diff --git a/clang/include/clang/AST/AbstractBasicReader.h b/clang/include/clang/AST/AbstractBasicReader.h
index 1f2797cc701458..ab036f1d445acc 100644
--- a/clang/include/clang/AST/AbstractBasicReader.h
+++ b/clang/include/clang/AST/AbstractBasicReader.h
@@ -213,9 +213,9 @@ class DataStreamBasicReader : public BasicReaderBase<Impl> {
}
Qualifiers readQualifiers() {
- static_assert(sizeof(Qualifiers().getAsOpaqueValue()) <= sizeof(uint32_t),
+ static_assert(sizeof(Qualifiers().getAsOpaqueValue()) <= sizeof(uint64_t),
"update this if the value size changes");
- uint32_t value = asImpl().readUInt32();
+ uint64_t value = asImpl().readUInt64();
return Qualifiers::fromOpaqueValue(value);
}
diff --git a/clang/include/clang/AST/AbstractBasicWriter.h b/clang/include/clang/AST/AbstractBasicWriter.h
index 07afa388de2c17..8e42fcaad1d388 100644
--- a/clang/include/clang/AST/AbstractBasicWriter.h
+++ b/clang/include/clang/AST/AbstractBasicWriter.h
@@ -196,9 +196,9 @@ class DataStreamBasicWriter : public BasicWriterBase<Impl> {
}
void writeQualifiers(Qualifiers value) {
- static_assert(sizeof(value.getAsOpaqueValue()) <= sizeof(uint32_t),
+ static_assert(sizeof(value.getAsOpaqueValue()) <= sizeof(uint64_t),
"update this if the value size changes");
- asImpl().writeUInt32(value.getAsOpaqueValue());
+ asImpl().writeUInt64(value.getAsOpaqueValue());
}
void writeExceptionSpecInfo(
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 1942b0e67f65a3..48a51dd851ac36 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -25,8 +25,10 @@
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/ExceptionSpecificationType.h"
#include "clang/Basic/LLVM.h"
+#include "clang/Basic/LangOptions.h"
#include "clang/Basic/Linkage.h"
#include "clang/Basic/PartialDiagnostic.h"
+#include "clang/Basic/PointerAuthOptions.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/Specifiers.h"
#include "clang/Basic/Visibility.h"
@@ -138,6 +140,165 @@ using CanQualType = CanQual<Type>;
#define TYPE(Class, Base) class Class##Type;
#include "clang/AST/TypeNodes.inc"
+/// Pointer-authentication qualifiers.
+class PointerAuthQualifier {
+ enum : uint32_t {
+ EnabledShift = 0,
+ EnabledBits = 1,
+ EnabledMask = 1 << EnabledShift,
+ AddressDiscriminatedShift = EnabledShift + EnabledBits,
+ AddressDiscriminatedBits = 1,
+ AddressDiscriminatedMask = 1 << AddressDiscriminatedShift,
+ AuthenticationModeShift =
+ AddressDiscriminatedShift + AddressDiscriminatedBits,
+ AuthenticationModeBits = 2,
+ AuthenticationModeMask = ((1 << AuthenticationModeBits) - 1)
+ << AuthenticationModeShift,
+ IsaPointerShift = AuthenticationModeShift + AuthenticationModeBits,
+ IsaPointerBits = 1,
+ IsaPointerMask = ((1 << IsaPointerBits) - 1) << IsaPointerShift,
+ AuthenticatesNullValuesShift = IsaPointerShift + IsaPointerBits,
+ AuthenticatesNullValuesBits = 1,
+ AuthenticatesNullValuesMask = ((1 << AuthenticatesNullValuesBits) - 1)
+ << AuthenticatesNullValuesShift,
+ KeyShift = AuthenticatesNullValuesShift + AuthenticatesNullValuesBits,
+ KeyBits = 10,
+ KeyMask = ((1 << KeyBits) - 1) << KeyShift,
+ DiscriminatorShift = KeyShift + KeyBits,
+ DiscriminatorBits = 16,
+ DiscriminatorMask = ((1u << DiscriminatorBits) - 1) << DiscriminatorShift,
+ };
+
+ // bits: |0 |1 |2..3 |4 |
+ // |Enabled|Address|AuthenticationMode|ISA pointer|
+ // bits: |5 |6..15| 16...31 |
+ // |AuthenticatesNull|Key |Discriminator|
+ uint32_t Data;
+
+ static_assert((EnabledBits + AddressDiscriminatedBits +
+ AuthenticationModeBits + IsaPointerBits +
+ AuthenticatesNullValuesBits + KeyBits + DiscriminatorBits) ==
+ 32,
+ "PointerAuthQualifier should be exactly 32 bits");
+ static_assert((EnabledMask + AddressDiscriminatedMask +
+ AuthenticationModeMask + IsaPointerMask +
+ AuthenticatesNullValuesMask + KeyMask + DiscriminatorMask) ==
+ 0xFFFFFFFF,
+ "All masks should cover the entire bits");
+ static_assert((EnabledMask ^ AddressDiscriminatedMask ^
+ AuthenticationModeMask ^ IsaPointerMask ^
+ AuthenticatesNullValuesMask ^ KeyMask ^ DiscriminatorMask) ==
+ 0xFFFFFFFF,
+ "All masks should cover the entire bits");
+
+ PointerAuthQualifier(unsigned key, bool isAddressDiscriminated,
+ unsigned extraDiscriminator,
+ PointerAuthenticationMode authenticationMode,
+ bool isIsaPointer, bool authenticatesNullValues)
+ : Data(EnabledMask |
+ (isAddressDiscriminated
+ ? static_cast<uint32_t>(AddressDiscriminatedMask)
+ : 0) |
+ (key << KeyShift) |
+ (unsigned(authenticationMode) << AuthenticationModeShift) |
+ (extraDiscriminator << DiscriminatorShift) |
+ (isIsaPointer << IsaPointerShift) |
+ (authenticatesNullValues << AuthenticatesNullValuesShift)) {
+ assert(key <= KeyNoneInternal);
+ assert(extraDiscriminator <= MaxDiscriminator);
+ }
+
+public:
+ enum {
+ KeyNoneInternal = (1u << KeyBits) - 1,
+
+ /// The maximum supported pointer-authentication key.
+ MaxKey = KeyNoneInternal - 1,
+
+ /// The maximum supported pointer-authentication discriminator.
+ MaxDiscriminator = (1u << DiscriminatorBits) - 1
+ };
+
+public:
+ PointerAuthQualifier() : Data(0) {}
+
+ static PointerAuthQualifier
+ Create(int key, bool isAddressDiscriminated, unsigned extraDiscriminator,
+ PointerAuthenticationMode authenticationMode, bool isIsaPointer,
+ bool authenticatesNullValues) {
+ if (key == PointerAuthKeyNone)
+ key = KeyNoneInternal;
+ assert((key >= 0 && key <= KeyNoneInternal) && "out-of-range key value");
+ return PointerAuthQualifier(key, isAddressDiscriminated, extraDiscriminator,
+ authenticationMode, isIsaPointer,
+ authenticatesNullValues);
+ }
+
+ bool isPresent() const {
+ return getAuthenticationMode() != PointerAuthenticationMode::None;
+ }
+
+ explicit operator bool() const { return isPresent(); }
+
+ unsigned getKey() const {
+ assert(isPresent());
+ return (Data & KeyMask) >> KeyShift;
+ }
+
+ bool hasKeyNone() const { return isPresent() && getKey() == KeyNoneInternal; }
+
+ bool isAddressDiscriminated() const {
+ assert(isPresent());
+ return (Data & AddressDiscriminatedMask) >> AddressDiscriminatedShift;
+ }
+
+ unsigned getExtraDiscriminator() const {
+ assert(isPresent());
+ return (Data >> DiscriminatorShift);
+ }
+
+ PointerAuthenticationMode getAuthenticationMode() const {
+ return PointerAuthenticationMode((Data & AuthenticationModeMask) >>
+ AuthenticationModeShift);
+ }
+
+ bool isIsaPointer() const {
+ assert(isPresent());
+ return (Data & IsaPointerMask) >> IsaPointerShift;
+ }
+
+ bool authenticatesNullValues() const {
+ assert(isPresent());
+ return (Data & AuthenticatesNullValuesMask) >> AuthenticatesNullValuesShift;
+ }
+
+ PointerAuthQualifier withoutKeyNone() const {
+ return hasKeyNone() ? PointerAuthQualifier() : *this;
+ }
+
+ friend bool operator==(PointerAuthQualifier Lhs, PointerAuthQualifier Rhs) {
+ return Lhs.Data == Rhs.Data;
+ }
+ friend bool operator!=(PointerAuthQualifier Lhs, PointerAuthQualifier Rhs) {
+ return Lhs.Data != Rhs.Data;
+ }
+
+ bool isEquivalent(PointerAuthQualifier Other) const {
+ return withoutKeyNone() == Other.withoutKeyNone();
+ }
+
+ uint32_t getAsOpaqueValue() const { return Data; }
+
+ // Deserialize pointer-auth qualifiers from an opaque representation.
+ static PointerAuthQualifier fromOpaqueValue(uint32_t opaque) {
+ PointerAuthQualifier result;
+ result.Data = opaque;
+ return result;
+ }
+
+ void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(Data); }
+};
+
/// The collection of all-type qualifiers we support.
/// Clang supports five independent qualifiers:
/// * C99: const, volatile, and restrict
@@ -193,19 +354,27 @@ class Qualifiers {
FastMask = (1 << FastWidth) - 1
};
+ Qualifiers() : Mask(0), PtrAuth() {}
+
/// Returns the common set of qualifiers while removing them from
/// the given sets.
static Qualifiers removeCommonQualifiers(Qualifiers &L, Qualifiers &R) {
+ Qualifiers Q;
+ if (L.getPointerAuth().isEquivalent(R.getPointerAuth())) {
+ Q.setPointerAuth(L.getPointerAuth().withoutKeyNone());
+ PointerAuthQualifier Empty;
+ L.setPointerAuth(Empty);
+ R.setPointerAuth(Empty);
+ }
+
// If both are only CVR-qualified, bit operations are sufficient.
if (!(L.Mask & ~CVRMask) && !(R.Mask & ~CVRMask)) {
- Qualifiers Q;
Q.Mask = L.Mask & R.Mask;
L.Mask &= ~Q.Mask;
R.Mask &= ~Q.Mask;
return Q;
}
- Qualifiers Q;
unsigned CommonCRV = L.getCVRQualifiers() & R.getCVRQualifiers();
Q.addCVRQualifiers(CommonCRV);
L.removeCVRQualifiers(CommonCRV);
@@ -250,15 +419,16 @@ class Qualifiers {
}
// Deserialize qualifiers from an opaque representation.
- static Qualifiers fromOpaqueValue(unsigned opaque) {
+ static Qualifiers fromOpaqueValue(uint64_t opaque) {
Qualifiers Qs;
- Qs.Mask = opaque;
+ Qs.Mask = uint32_t(opaque);
+ Qs.PtrAuth = PointerAuthQualifier::fromOpaqueValue(uint32_t(opaque >> 32));
return Qs;
}
// Serialize these qualifiers into an opaque representation.
- unsigned getAsOpaqueValue() const {
- return Mask;
+ uint64_t getAsOpaqueValue() const {
+ return uint64_t(Mask) | (uint64_t(PtrAuth.getAsOpaqueValue()) << 32);
}
bool hasConst() const { return Mask & Const; }
@@ -406,6 +576,10 @@ class Qualifiers {
setAddressSpace(space);
}
+ PointerAuthQualifier getPointerAuth() const { return PtrAuth; }
+ void setPointerAuth(PointerAuthQualifier q) { PtrAuth = q; }
+ void removePtrAuth() { PtrAuth = PointerAuthQualifier(); }
+
// Fast qualifiers are those that can be allocated directly
// on a QualType object.
bool hasFastQualifiers() const { return getFastQualifiers(); }
@@ -428,7 +602,7 @@ class Qualifiers {
/// Return true if the set contains any qualifiers which require an ExtQuals
/// node to be allocated.
- bool hasNonFastQualifiers() const { return Mask & ~FastMask; }
+ bool hasNonFastQualifiers() const { return (Mask & ~FastMask) || PtrAuth; }
Qualifiers getNonFastQualifiers() const {
Qualifiers Quals = *this;
Quals.setFastQualifiers(0);
@@ -436,8 +610,8 @@ class Qualifiers {
}
/// Return true if the set contains any qualifiers.
- bool hasQualifiers() const { return Mask; }
- bool empty() const { return !Mask; }
+ bool hasQualifiers() const { return Mask || PtrAuth; }
+ bool empty() const { return !hasQualifiers(); }
/// Add the qualifiers from the given set to this set.
void addQualifiers(Qualifiers Q) {
@@ -454,6 +628,9 @@ class Qualifiers {
if (Q.hasObjCLifetime())
addObjCLifetime(Q.getObjCLifetime());
}
+
+ if (Q.PtrAuth)
+ PtrAuth = Q.PtrAuth;
}
/// Remove the qualifiers from the given set from this set.
@@ -471,6 +648,9 @@ class Qualifiers {
if (getAddressSpace() == Q.getAddressSpace())
removeAddressSpace();
}
+
+ if (PtrAuth == Q.PtrAuth)
+ PtrAuth = PointerAuthQualifier();
}
/// Add the qualifiers from the given set to this set, given that
@@ -482,7 +662,10 @@ class Qualifiers {
!hasObjCGCAttr() || !qs.hasObjCGCAttr());
assert(getObjCLifetime() == qs.getObjCLifetime() ||
!hasObjCLifetime() || !qs.hasObjCLifetime());
+ assert(!PtrAuth || !qs.PtrAuth || PtrAuth == qs.PtrAuth);
Mask |= qs.Mask;
+ if (qs.PtrAuth)
+ PtrAuth = qs.PtrAuth;
}
/// Returns true if address space A is equal to or a superset of B.
@@ -535,6 +718,8 @@ class Qualifiers {
// be changed.
(getObjCGCAttr() == other.getObjCGCAttr() || !hasObjCGCAttr() ||
!other.hasObjCGCAttr()) &&
+ // Pointer-auth qualifiers must match exactly.
+ PtrAuth == other.PtrAuth &&
// ObjC lifetime qualifiers must match exactly.
getObjCLifetime() == other.getObjCLifetime() &&
// CVR qualifiers may subset.
@@ -567,8 +752,12 @@ class Qualifiers {
/// another set of qualifiers, not considering qualifier compatibility.
bool isStrictSupersetOf(Qualifiers Other) const;
- bool operator==(Qualifiers Other) const { return Mask == Other.Mask; }
- bool operator!=(Qualifiers Other) const { return Mask != Other.Mask; }
+ bool operator==(Qualifiers Other) const {
+ return Mask == Other.Mask && PtrAuth == Other.PtrAuth;
+ }
+ bool operator!=(Qualifiers Other) const {
+ return Mask != Other.Mask || PtrAuth != Other.PtrAuth;
+ }
explicit operator bool() const { return hasQualifiers(); }
@@ -606,6 +795,7 @@ class Qualifiers {
void Profile(llvm::FoldingSetNodeID &ID) const {
ID.AddInteger(Mask);
+ PtrAuth.Profile(ID);
}
private:
@@ -613,6 +803,9 @@ class Qualifiers {
// |C R V|U|GCAttr|Lifetime|AddressSpace|
uint32_t Mask = 0;
+ PointerAuthQualifier PtrAuth;
+ static_assert(sizeof(PointerAuthQualifier) == sizeof(uint32_t),
+ "PointerAuthQualifier must be 32 bits");
static const uint32_t UMask = 0x8;
static const uint32_t UShift = 3;
static const uint32_t GCAttrMask = 0x30;
@@ -1241,6 +1434,10 @@ class QualType {
// true when Type is objc's weak and weak is enabled but ARC isn't.
bool isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const;
+ PointerAuthQualifier getPointerAuth() const {
+ return getQualifiers().getPointerAuth();
+ }
+
enum PrimitiveDefaultInitializeKind {
/// The type does not fall into any of the following categories. Note that
/// this case is zero-valued so that values of this enum can be used as a
diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h
index 862952d336ef31..6fe7472d8ad0ca 100644
--- a/clang/include/clang/Basic/LangOptions.h
+++ b/clang/include/clang/Basic/LangOptions.h
@@ -57,6 +57,13 @@ enum class ShaderStage {
Invalid,
};
+enum class PointerAuthenticationMode : unsigned {
+ None,
+ Strip,
+ SignAndStrip,
+ SignAndAuth
+};
+
/// Bitfields of LangOptions, split out from LangOptions in order to ensure that
/// this large collection of bitfields is a trivial class type.
class LangOptionsBase {
diff --git a/clang/include/clang/Basic/PointerAuthOptions.h b/clang/include/clang/Basic/PointerAuthOptions.h
new file mode 100644
index 00000000000000..c69847ac704a41
--- /dev/null
+++ b/clang/include/clang/Basic/PointerAuthOptions.h
@@ -0,0 +1,23 @@
+//===--- PointerAuthOptions.h -----------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines options for configuring pointer-auth technologies
+// like ARMv8.3.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_BASIC_POINTERAUTHOPTIONS_H
+#define LLVM_CLANG_BASIC_POINTERAUTHOPTIONS_H
+
+namespace clang {
+
+constexpr int PointerAuthKeyNone = -1;
+
+} // end namespace clang
+
+#endif
|
|
A kind reminder regarding the PR - would be glad to see feedback from everyone interested. |
|
A kind reminder regarding the PR - this is a prerequisite for #84387, so it would be nice if those who are interested leave a feedback and we can merge both this PR and the dependent one. |
There was a problem hiding this comment.
Seems reasonable to me but would prefer if someone more familiar with this (e.g., @rjmccall) gave final approval
clang/include/clang/AST/Type.h
Outdated
| bool isIsaPointer, bool authenticatesNullValues) | ||
| : Data(EnabledMask | | ||
| (isAddressDiscriminated | ||
| ? static_cast<uint32_t>(AddressDiscriminatedMask) |
There was a problem hiding this comment.
| ? static_cast<uint32_t>(AddressDiscriminatedMask) | |
| ? llvm::to_underlying(AddressDiscriminatedMask) |
There was a problem hiding this comment.
Fixed, thanks, see 3517033
clang/include/clang/AST/Type.h
Outdated
| ? static_cast<uint32_t>(AddressDiscriminatedMask) | ||
| : 0) | | ||
| (key << KeyShift) | | ||
| (unsigned(authenticationMode) << AuthenticationModeShift) | |
There was a problem hiding this comment.
| (unsigned(authenticationMode) << AuthenticationModeShift) | | |
| (llvm::to_underlying(authenticationMode) << AuthenticationModeShift) | |
There was a problem hiding this comment.
Fixed, thanks, see 3517033
clang/include/clang/AST/Type.h
Outdated
| PointerAuthQualifier() : Data(0) {} | ||
|
|
||
| static PointerAuthQualifier | ||
| Create(int key, bool isAddressDiscriminated, unsigned extraDiscriminator, |
There was a problem hiding this comment.
Why is key an int here but unsigned in the PointerAuthQualifier constructor?
There was a problem hiding this comment.
It should be one type everywhere, thanks for bringing attention to this. Changed to unsigned to keep things consistent with other parameters.
|
Are there unittests where we could exercise these types? |
clang/include/clang/AST/Type.h
Outdated
| Qualifiers Qs; | ||
| Qs.Mask = opaque; | ||
| Qs.Mask = uint32_t(Opaque); |
There was a problem hiding this comment.
Is this OK or should we prefer smth like Opaque & 0xFFFFFFFF or even Opaque & std::numeric_limits<uint32_t>::max() (or smth else)?
There was a problem hiding this comment.
Either of those look better imo, or could even use maskTrailingOnes from Support/MathExtras.h. Same for the below
There was a problem hiding this comment.
Thanks, I've used numeric limits stuff since it allows constexpr (while maskTrailingOnes is more flexible for arbitrary bit count but does not have constexpr variant as far as I can see). See df81082
@Michael137 Could you clarify a bit, what is the correct place for such a unit test? As for |
|
Although functions in this file use different code styles for function arguments (both I could also submit a subsequent PR fixing the code style of function parameters in clang/include/clang/AST/Type.h and clang/include/clang/AST/ASTContext.h which are not in |
|
It'll be tested in a follow-up commit which actually adds parsing and Sema support for these as qualifiers. That should be sufficient rather than adding a bunch of fairly superficial unit tests. Daniil is just trying to avoid landing the entire feature in a single massive commit. |
|
@AaronBallman Thanks for feedback!
This PR is a prerequisite for #84387. It uses code from this PR and contains tests. There was a discussion regarding adding tests to this PR previously (see comments above), but it seems that the consensus was to rely on tests from #84387. I've not found a proper example of tests for similar functionality - see comment #84384 (comment). I would be glad to see any suggestions on how this could be tested if just relying on a subsequent PR is not enough. If having tests in the subsequent patch is considered to be enough, I'll add a notice for that in the PR description not to confuse anyone more by test absence. |
Oh shoot, I missed that this was discussed earlier in the thread. Doing the testing in a follow-up is reasonable. |
|
I've fixed the build failure https://buildkite.com/llvm-project/github-pull-requests/builds/55986 in clang-tidy by only taking fast qualifiers from the @AaronBallman Please let me know if the approach from e3cb910 is correct or if we should fix the issue somehow differently. |
|
@AaronBallman See test results from compile-time-tracker here: https://llvm-compile-time-tracker.com/compare.php?from=693a458287d019c5c6a66fe3019d099df2978cdb&to=dbb5e29d136a18060ba6759b328ad80fa9cea649. It looks like that there is a statistically meaningful difference, but it's only about 0.05..0.25% depending on the test. Is it considered OK? |
This seems like the correct approach to me. Maybe someday we can use a strong typedef for the fast qualifiers so there's less risk of accidents here, but that's by no means something you need to handle. |
Yeah, this seems to have noticeable impact on compile times for every compilation; out of curiosity, have you tried an approach where this information is stored in |
Not yet, thanks for suggestion. I'll try and let you know about results |
Given all that, I'm not sure what you're asking for. Storing uncommon qualifiers out of line is what we already do with The overhead is probably from additional checks rather than any cost associated with working with a 64-bit |
Ah, okay, thank you! I had two concerns, but I think neither are valid now that I better understand how
Do you think the performance concerns are sufficient to warrant doing this optimization work up front? .25% is big enough to warrant concern, but not big enough to need to ask for onerous efforts. |
AFAIK,
I'm sure we still have 32-bit builds of Clang, and yeah, they're going to be incrementally slower as We're somewhat "saved" here by the fact that many 32-bit ABIs are quite bad. For example, the SysV x86 calling convention always returns structs indirectly, no matter how small they are.
I personally don't think so, no. But this patch upstreams an overhead we've been eating at Apple for about 7 years, so one could argue that it's easy for me to say that. It's not unreasonable for you to ask for an investigation to see if we can identify any localized source of the slowdown. If it's across-the-board (which at .25%, I'd say it probably is, but you never know), maybe we just have to accept it. |
I'm now applying this approach and it looks like that the overhead becomes significantly lower. A new invariant has to be introduced: with |
|
@AaronBallman @rjmccall Please see 53485b1 commit which makes |
After llvm#84384, `Qualifiers::Mask` becomes 64-bit. So, operations like `Mask &= ~U32` where `U32` is `unsigned` produce undesirable results since higher 32 bits of `Mask` become zeroed while they should be preserved. Fix that by explicitly casting `unsigned` values to `uint64_t` in such operations. Signatures of fixed functions are intentionally left intact instead of changing the argument itself to `uint64_t` to keep things consistent with other functions working with the same qualifiers and to emphasize that 64-bit masks should not be used for these types of qualifiers.
…k` (#90329) After #84384, `Qualifiers::Mask` becomes 64-bit. So, operations like `Mask &= ~U32` where `U32` is `unsigned` produce undesirable results since higher 32 bits of `Mask` become zeroed while they should be preserved. Fix that by explicitly casting `unsigned` values to `uint64_t` in such operations. Signatures of fixed functions are intentionally left intact instead of changing the argument itself to `uint64_t` to keep things consistent with other functions working with the same qualifiers and to emphasize that 64-bit masks should not be used for these types of qualifiers.
…sions (#84387) Depends on #84384 and #90329 This adds support for `DW_TAG_LLVM_ptrauth_type` entries corresponding to explicitly signed types (e.g. free function pointers) in lldb user expressions. Applies PR swiftlang#8239 from Apple's downstream and also adds tests and related code. --------- Co-authored-by: Jonas Devlieghere <[email protected]>
This brings declarations of
PointerAuthQualifierclass andPointerAuthenticationModeenum and related functions required for PAuth support in lldb (see #84387) from downstream Apple's code. See #84387 for tests as well.Co-authored-by: Ahmed Bougacha [email protected]
Co-authored-by: John McCall [email protected]