[lld] Refactor storage of PAuth ABI core info #141920
Open
+54
−25
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, the AArch64 PAuth ABI core values were stored as an ArrayRef<uint8_t>, introducing unnecessary indirection. This patch replaces the ArrayRef with two explicit uint64_t fields: aarch64PauthAbiPlatform and aarch64PauthAbiVersion. This simplifies the representation and improves readability. No functional change intended.
sivan-shani
requested review from
stuij,
ostannard,
davemgreen,
nasherm and
smithp35
|
This PR is a preparatory refactor to support a follow-up patch that will add AArch64 Build Attributes to LLD. The intent is to split the work into clean, reviewable steps and avoid repeating past mistakes. Specifically, this change isolates and simplifies the handling of GNU Properties, so that support for Build Attributes can later be added on top of a clearer structure, with minimal duplication and maximum alignment between the two. We would appreciate as many eyes as possible on this PR to ensure it is solid and aligns with project expectations, especially after feedback on the previous attempt (PR #131990). This is the first step in a more structured and minimal series of changes. |
nasherm
reviewed
May 29, 2025
ostannard
reviewed
May 29, 2025
smithp35
reviewed
May 29, 2025
There was a problem hiding this comment.
Main suggestion is to make aarch64PauthAbiPlatform a class as there is at least one invariant, and we can simplify some of the calling code by using the member functions. I'm not sure whether that will make a significant difference. I suggest trying it and then seeing if you prefer it.
I recommend taking out [GNU Properties] from the title as I don't think that has been used anywhere else and is not common enough to add any more information.
Ack, commit message has been updated. |
sivan-shani
changed the title
Ack, commit message has been updated. |
|
Thanks everyone for the feedback. |
- Remove redundant include - Rename fields in AArch64PauthAbiCoreInfo - Simplify assignment using direct initialization
- Convert AArch64PauthAbiCoreInfo to a struct with isValid(), size(), and comparison operators. - Simplify comparison logic in readSecurityNotes() using operator!=.
sivan-shani
force-pushed
the
refactorGNUPLLD_NEW
branch
from
3955931 to
e29b15b
Compare
smithp35
reviewed
May 30, 2025
- Added comments explaining the semantics of AArch64PauthAbiCoreInfo and the reserved (0, 0) value. - Replaced hardcoded sizeof(uint64_t) * 2 with AArch64PauthAbiCoreInfo::size().
Thanks, comments have been addressed. If no further feedback comes in, I plan to move this from 'Draft' to 'Ready for review' early next week. |
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: SivanShani-Arm (sivan-shani) ChangesPreviously, the AArch64 PAuth ABI core values were stored as an ArrayRef<uint8_t>, introducing unnecessary indirection. This patch replaces the ArrayRef with two explicit uint64_t fields: aarch64PauthAbiPlatform and aarch64PauthAbiVersion. This simplifies the representation and improves readability. No functional change intended, aside from improved error messages. Full diff: https://github.com/llvm/llvm-project/pull/141920.diff 7 Files Affected:
diff --git a/lld/ELF/Arch/AArch64.cpp b/lld/ELF/Arch/AArch64.cpp
index 9538dd4a70bae..30ce41e4effa8 100644
--- a/lld/ELF/Arch/AArch64.cpp
+++ b/lld/ELF/Arch/AArch64.cpp
@@ -1044,8 +1044,7 @@ AArch64BtiPac::AArch64BtiPac(Ctx &ctx) : AArch64(ctx) {
// instructions.
if (ctx.arg.zPacPlt) {
- if (llvm::any_of(ctx.aarch64PauthAbiCoreInfo,
- [](uint8_t c) { return c != 0; }))
+ if (ctx.aarch64PauthAbiCoreInfo && ctx.aarch64PauthAbiCoreInfo->isValid())
pacEntryKind = PEK_Auth;
else
pacEntryKind = PEK_AuthHint;
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index f0e9592d85dd6..d8d9e3fd1699e 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -139,6 +139,23 @@ enum class GcsPolicy { Implicit, Never, Always };
// For some options that resemble -z bti-report={none,warning,error}
enum class ReportPolicy { None, Warning, Error };
+// Describes the signing schema for a file using the PAuth ABI extension.
+// Two files are considered compatible when both `platform` and `version` match.
+// The pair (0, 0) is reserved to indicate incompatibility with the PAuth ABI.
+struct AArch64PauthAbiCoreInfo {
+ uint64_t platform;
+ uint64_t version;
+ // Returns true if the core info is not the reserved (0, 0) value.
+ bool isValid() const { return platform || version; }
+ static constexpr size_t size() { return sizeof(platform) + sizeof(version); }
+ bool operator==(const AArch64PauthAbiCoreInfo &other) const {
+ return platform == other.platform && version == other.version;
+ }
+ bool operator!=(const AArch64PauthAbiCoreInfo &other) const {
+ return !(*this == other);
+ }
+};
+
struct SymbolVersion {
llvm::StringRef name;
bool isExternCpp;
@@ -695,7 +712,7 @@ struct Ctx : CommonLinkerContext {
llvm::raw_fd_ostream openAuxiliaryFile(llvm::StringRef, std::error_code &);
- ArrayRef<uint8_t> aarch64PauthAbiCoreInfo;
+ std::optional<AArch64PauthAbiCoreInfo> aarch64PauthAbiCoreInfo;
};
// The first two elements of versionDefinitions represent VER_NDX_LOCAL and
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 76a37b706c5fa..da51d9fab8d07 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -2841,15 +2841,15 @@ static void readSecurityNotes(Ctx &ctx) {
StringRef referenceFileName;
if (ctx.arg.emachine == EM_AARCH64) {
auto it = llvm::find_if(ctx.objectFiles, [](const ELFFileBase *f) {
- return !f->aarch64PauthAbiCoreInfo.empty();
+ return f->aarch64PauthAbiCoreInfo.has_value();
});
if (it != ctx.objectFiles.end()) {
ctx.aarch64PauthAbiCoreInfo = (*it)->aarch64PauthAbiCoreInfo;
referenceFileName = (*it)->getName();
}
}
- bool hasValidPauthAbiCoreInfo = llvm::any_of(
- ctx.aarch64PauthAbiCoreInfo, [](uint8_t c) { return c != 0; });
+ bool hasValidPauthAbiCoreInfo =
+ ctx.aarch64PauthAbiCoreInfo && ctx.aarch64PauthAbiCoreInfo->isValid();
auto report = [&](ReportPolicy policy) -> ELFSyncStream {
return {ctx, toDiagLevel(policy)};
@@ -2909,10 +2909,10 @@ static void readSecurityNotes(Ctx &ctx) {
}
ctx.arg.andFeatures &= features;
- if (ctx.aarch64PauthAbiCoreInfo.empty())
+ if (!ctx.aarch64PauthAbiCoreInfo)
continue;
- if (f->aarch64PauthAbiCoreInfo.empty()) {
+ if (!f->aarch64PauthAbiCoreInfo) {
report(ctx.arg.zPauthReport)
<< f
<< ": -z pauth-report: file does not have AArch64 "
@@ -2922,11 +2922,18 @@ static void readSecurityNotes(Ctx &ctx) {
}
if (ctx.aarch64PauthAbiCoreInfo != f->aarch64PauthAbiCoreInfo)
- Err(ctx) << "incompatible values of AArch64 PAuth core info found\n>>> "
- << referenceFileName << ": 0x"
- << toHex(ctx.aarch64PauthAbiCoreInfo, /*LowerCase=*/true)
- << "\n>>> " << f << ": 0x"
- << toHex(f->aarch64PauthAbiCoreInfo, /*LowerCase=*/true);
+ Err(ctx)
+ << "incompatible values of AArch64 PAuth core info found\n"
+ << "platform:\n"
+ << ">>> " << referenceFileName << ": 0x"
+ << toHex(ctx.aarch64PauthAbiCoreInfo->platform, /*LowerCase=*/true)
+ << "\n>>> " << f << ": 0x"
+ << toHex(f->aarch64PauthAbiCoreInfo->platform, /*LowerCase=*/true)
+ << "\nversion:\n"
+ << ">>> " << referenceFileName << ": 0x"
+ << toHex(ctx.aarch64PauthAbiCoreInfo->version, /*LowerCase=*/true)
+ << "\n>>> " << f << ": 0x"
+ << toHex(f->aarch64PauthAbiCoreInfo->version, /*LowerCase=*/true);
}
// Force enable Shadow Stack.
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 5f6d2b6b647ee..817af15a841ec 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -950,7 +950,7 @@ static void parseGnuPropertyNote(Ctx &ctx, ELFFileBase &f,
} else if (ctx.arg.emachine == EM_AARCH64 &&
type == GNU_PROPERTY_AARCH64_FEATURE_PAUTH) {
ArrayRef<uint8_t> contents = data ? *data : desc;
- if (!f.aarch64PauthAbiCoreInfo.empty()) {
+ if (f.aarch64PauthAbiCoreInfo) {
return void(
err(contents.data())
<< "multiple GNU_PROPERTY_AARCH64_FEATURE_PAUTH entries are "
@@ -961,7 +961,9 @@ static void parseGnuPropertyNote(Ctx &ctx, ELFFileBase &f,
"is invalid: expected 16 bytes, but got "
<< size);
}
- f.aarch64PauthAbiCoreInfo = desc;
+ f.aarch64PauthAbiCoreInfo = {
+ support::endian::read64<ELFT::Endianness>(&desc[0]),
+ support::endian::read64<ELFT::Endianness>(&desc[8])};
}
// Padding is present in the note descriptor, if necessary.
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index 808cb5d24079f..ba844ad18f637 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -241,7 +241,7 @@ class ELFFileBase : public InputFile {
StringRef sourceFile;
uint32_t andFeatures = 0;
bool hasCommonSyms = false;
- ArrayRef<uint8_t> aarch64PauthAbiCoreInfo;
+ std::optional<AArch64PauthAbiCoreInfo> aarch64PauthAbiCoreInfo;
};
// .o file.
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index 46591e909ba4f..eb902767746de 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -344,11 +344,11 @@ void GnuPropertySection::writeTo(uint8_t *buf) {
offset += 16;
}
- if (!ctx.aarch64PauthAbiCoreInfo.empty()) {
+ if (ctx.aarch64PauthAbiCoreInfo) {
write32(ctx, buf + offset + 0, GNU_PROPERTY_AARCH64_FEATURE_PAUTH);
- write32(ctx, buf + offset + 4, ctx.aarch64PauthAbiCoreInfo.size());
- memcpy(buf + offset + 8, ctx.aarch64PauthAbiCoreInfo.data(),
- ctx.aarch64PauthAbiCoreInfo.size());
+ write32(ctx, buf + offset + 4, AArch64PauthAbiCoreInfo::size());
+ write64(ctx, buf + offset + 8, ctx.aarch64PauthAbiCoreInfo->platform);
+ write64(ctx, buf + offset + 16, ctx.aarch64PauthAbiCoreInfo->version);
}
}
@@ -356,8 +356,8 @@ size_t GnuPropertySection::getSize() const {
uint32_t contentSize = 0;
if (ctx.arg.andFeatures != 0)
contentSize += ctx.arg.is64 ? 16 : 12;
- if (!ctx.aarch64PauthAbiCoreInfo.empty())
- contentSize += 4 + 4 + ctx.aarch64PauthAbiCoreInfo.size();
+ if (ctx.aarch64PauthAbiCoreInfo)
+ contentSize += 4 + 4 + AArch64PauthAbiCoreInfo::size();
assert(contentSize != 0);
return contentSize + 16;
}
@@ -4959,7 +4959,7 @@ template <class ELFT> void elf::createSyntheticSections(Ctx &ctx) {
ctx.in.iplt = std::make_unique<IpltSection>(ctx);
add(*ctx.in.iplt);
- if (ctx.arg.andFeatures || !ctx.aarch64PauthAbiCoreInfo.empty()) {
+ if (ctx.arg.andFeatures || ctx.aarch64PauthAbiCoreInfo) {
ctx.in.gnuProperty = std::make_unique<GnuPropertySection>(ctx);
add(*ctx.in.gnuProperty);
}
diff --git a/lld/test/ELF/aarch64-feature-pauth.s b/lld/test/ELF/aarch64-feature-pauth.s
index bc58f69d32f2b..e8c900b9cb134 100644
--- a/lld/test/ELF/aarch64-feature-pauth.s
+++ b/lld/test/ELF/aarch64-feature-pauth.s
@@ -13,8 +13,12 @@
# RUN: not ld.lld tag1.o tag1a.o tag2.o -o /dev/null 2>&1 | FileCheck --check-prefix ERR1 %s
# ERR1: error: incompatible values of AArch64 PAuth core info found
-# ERR1-NEXT: >>> tag1.o: 0x2a000000000000000{{1|2}}00000000000000
-# ERR1-NEXT: >>> tag2.o: 0x2a000000000000000{{1|2}}00000000000000
+# ERR1-NEXT: platform:
+# ERR1-NEXT: >>> tag1.o: 0x2a
+# ERR1-NEXT: >>> tag2.o: 0x2a
+# ERR1-NEXT: version:
+# ERR1-NEXT: >>> tag1.o: 0x01
+# ERR1-NEXT: >>> tag2.o: 0x02
# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu abi-tag-short.s -o short.o
# RUN: not ld.lld short.o -o /dev/null 2>&1 | FileCheck --check-prefix ERR2 %s
|
|
The implementation of AArch64 Build Attributes on top of this change: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the AArch64 PAuth ABI core values were stored as an ArrayRef<uint8_t>, introducing unnecessary indirection.
This patch replaces the ArrayRef with two explicit uint64_t fields: aarch64PauthAbiPlatform and aarch64PauthAbiVersion. This simplifies the representation and improves readability.
No functional change intended, aside from improved error messages.