[lld][AArch64][Build Attributes] Add support for converting AArch64 Build Attributes to GNU Properties #131990
Conversation
…uild Attributes to GNU Properties This patch enables `lld` to read AArch64 Build Attributes and convert them into GNU Properties. Changes: - Parses AArch64 Build Attributes from input object files. - Converts known attributes into corresponding GNU Properties. - Merges attributes when linking multiple objects.
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-llvm-binary-utilities Author: SivanShani-Arm (sivan-shani) Changes…uild Attributes to GNU Properties This patch enables Changes:
Full diff: https://github.com/llvm/llvm-project/pull/131990.diff 8 Files Affected:
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index e07c7dd4ca1b6..9969a9dec5c35 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -24,6 +24,7 @@
#include "llvm/Support/CodeGen.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/Compression.h"
+#include "llvm/Support/ELFAttributes.h"
#include "llvm/Support/Endian.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/GlobPattern.h"
@@ -694,6 +695,10 @@ struct Ctx : CommonLinkerContext {
llvm::raw_fd_ostream openAuxiliaryFile(llvm::StringRef, std::error_code &);
ArrayRef<uint8_t> aarch64PauthAbiCoreInfo;
+
+ // AArch64 Build Attributes data
+ std::optional<llvm::BuildAttributeSubSection> mergedPauthSubSection;
+ std::optional<llvm::BuildAttributeSubSection> mergedFAndBSubSection;
};
// The first two elements of versionDefinitions represent VER_NDX_LOCAL and
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 5f6d2b6b647ee..7ae91729ee4df 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -20,16 +20,21 @@
#include "lld/Common/DWARF.h"
#include "llvm/ADT/CachedHashString.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/LTO/LTO.h"
#include "llvm/Object/IRObjectFile.h"
+#include "llvm/Support/AArch64AttributeParser.h"
#include "llvm/Support/ARMAttributeParser.h"
#include "llvm/Support/ARMBuildAttributes.h"
+#include "llvm/Support/ELFAttributes.h"
#include "llvm/Support/Endian.h"
+#include "llvm/Support/Errc.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/RISCVAttributeParser.h"
#include "llvm/Support/TimeProfiler.h"
#include "llvm/Support/raw_ostream.h"
+#include <cassert>
#include <optional>
using namespace llvm;
@@ -207,6 +212,166 @@ static void updateSupportedARMFeatures(Ctx &ctx,
ctx.arg.armHasThumb2ISA |= thumb && *thumb >= ARMBuildAttrs::AllowThumb32;
}
+// Sanitize pauth values
+static void sanitizePauthSubSection(
+ Ctx &ctx, std::optional<llvm::BuildAttributeSubSection> &pauthSubSection,
+ InputSection isec) {
+ /*
+ Incomplete data: ignore
+ */
+ if (!pauthSubSection)
+ return;
+ // Currently there are 2 known tags defined for the pauth subsection,
+ // however, user is allowed to add other, unknown tag. If such tags exists,
+ // remove them. (no need to check for duplicates, they should not be possible)
+ pauthSubSection->Content.erase(
+ std::remove_if(pauthSubSection->Content.begin(),
+ pauthSubSection->Content.end(),
+ [](const BuildAttributeItem &item) {
+ return item.Tag != 1 && item.Tag != 2;
+ }),
+ pauthSubSection->Content.end());
+
+ if (pauthSubSection->Content.size() < 2) {
+ if (0 == pauthSubSection->Content.size())
+ Warn(ctx) << &isec
+ << ": AArch64 Build Attributes: empty 'aeabi_pauthabi' "
+ "subsection detected; ignoring subsection";
+ if (1 == pauthSubSection->Content.size()) {
+ if (1 == pauthSubSection->Content[0].Tag)
+ Warn(ctx)
+ << &isec
+ << ": AArch64 Build Attributes: 'aeabi_pauthabi' subsection "
+ "contains only an ID (scheme missing); ignoring subsection";
+ if (2 == pauthSubSection->Content[0].Tag)
+ Warn(ctx) << &isec
+ << ": AArch64 Build Attributes: 'aeabi_pauthabi' subsection "
+ "contains only a scheme (ID missing); ignoring subsection";
+ }
+ pauthSubSection = std::nullopt;
+ return;
+ }
+ // printvec(*pauthSubSection);
+ assert(2 == pauthSubSection->Content.size() && "vector size should be 2");
+ std::sort(pauthSubSection->Content.begin(), pauthSubSection->Content.end(),
+ [](const auto &a, const auto &b) { return a.Tag < b.Tag; });
+ assert(1 == pauthSubSection->Content[0].Tag && "first tag should be 1");
+ assert(2 == pauthSubSection->Content[1].Tag && "first tag should be 2");
+}
+
+// Sanitize features bits
+static void sanitizeFAndBSubSection(
+ std::optional<llvm::BuildAttributeSubSection> &fAndBSubSection) {
+ /*
+ Same as gnu properties: treat a missing 'aeabi_feature_and_bits' feature as
+ being set to 0
+ */
+ if (!fAndBSubSection) {
+ fAndBSubSection.emplace("aeabi_feature_and_bits", 1, 0,
+ SmallVector<BuildAttributeItem, 64>());
+ } else {
+ // Currently there are 3 known tags defined for the features and bits
+ // subsection, however, user is allowed to add other, unknown tag. If such
+ // tags exists, remove them. (duplicates are not possible)
+ fAndBSubSection->Content.erase(
+ std::remove_if(fAndBSubSection->Content.begin(),
+ fAndBSubSection->Content.end(),
+ [](const BuildAttributeItem &item) {
+ return item.Tag != 0 && item.Tag != 1 && item.Tag != 2;
+ }),
+ fAndBSubSection->Content.end());
+ }
+
+ constexpr unsigned tagBTI = 0, tagPAC = 1, tagGCS = 2;
+ // Find missing tags
+ std::set<unsigned> requiredTags = {tagBTI, tagPAC, tagGCS};
+ for (const auto &item : fAndBSubSection->Content)
+ requiredTags.erase(item.Tag);
+
+ // Add missing tags
+ for (const auto &tag : requiredTags)
+ fAndBSubSection->Content.push_back(
+ BuildAttributeItem(BuildAttributeItem::NumericAttribute, tag, 0, ""));
+
+ assert(3 == fAndBSubSection->Content.size() && "vector size should be 3");
+ std::sort(fAndBSubSection->Content.begin(), fAndBSubSection->Content.end(),
+ [](const auto &a, const auto &b) { return a.Tag < b.Tag; });
+ assert(0 == fAndBSubSection->Content[0].Tag && "first tag should be 0");
+ assert(1 == fAndBSubSection->Content[1].Tag && "first tag should be 1");
+ assert(2 == fAndBSubSection->Content[2].Tag && "first tag should be 2");
+}
+
+static std::array<std::optional<llvm::BuildAttributeSubSection>, 2>
+extractBuildAttributesSubsection(
+ Ctx &ctx,
+ const SmallVector<llvm::BuildAttributeSubSection, 8>
+ &buildAttributesSubsections,
+ InputSection isec) {
+
+ std::optional<llvm::BuildAttributeSubSection> newPauthSubSection;
+ std::optional<llvm::BuildAttributeSubSection> newFAndBSubSection;
+
+ for (const auto &newSubSection : buildAttributesSubsections) {
+ if (newPauthSubSection && newFAndBSubSection)
+ break;
+ if ("aeabi_pauthabi" == newSubSection.Name) {
+ newPauthSubSection.emplace(newSubSection);
+ continue;
+ }
+ if ("aeabi_feature_and_bits" == newSubSection.Name) {
+ newFAndBSubSection.emplace(newSubSection);
+ }
+ }
+ sanitizePauthSubSection(ctx, newPauthSubSection, isec);
+ sanitizeFAndBSubSection(newFAndBSubSection);
+
+ return {std::move(newPauthSubSection), std::move(newFAndBSubSection)};
+}
+
+// Merge AArch64 Build Attributes subsection
+static void mergeAArch64BuildAttributes(
+ Ctx &ctx,
+ const std::array<std::optional<llvm::BuildAttributeSubSection>, 2>
+ &buildAttributesSubsections,
+ InputSection isec) {
+
+ auto [newPauthSubSection, newFAndBSubSection] = buildAttributesSubsections;
+
+ if (ctx.mergedPauthSubSection == std::nullopt) {
+ ctx.mergedPauthSubSection = newPauthSubSection;
+ }
+
+ if (ctx.mergedFAndBSubSection == std::nullopt)
+ ctx.mergedFAndBSubSection = newFAndBSubSection;
+
+ if (newPauthSubSection) {
+ // Since sanitizePauthSubSection sorts, we know that both vectors align.
+ // Merge pauth (values has to match)
+ if ((ctx.mergedPauthSubSection->Content[0].IntValue !=
+ newPauthSubSection->Content[0].IntValue) ||
+ ctx.mergedPauthSubSection->Content[1].IntValue !=
+ newPauthSubSection->Content[1].IntValue) {
+ ctx.mergedPauthSubSection->Content[0].IntValue =
+ std::numeric_limits<unsigned>::max();
+ ctx.mergedPauthSubSection->Content[1].IntValue =
+ std::numeric_limits<unsigned>::max();
+ Warn(ctx)
+ << &isec
+ << ": AArch64 Build Attributes: mismatch in 'aeabi_pauthabi' values "
+ "detected; marking 'aeabi_pauthabi' as invalid for this project";
+ }
+ }
+
+ // Since sanitizeFAndBSubSection sorts, we know that both vectors align.
+ // Merge Features and Bits
+ ctx.mergedFAndBSubSection->Content[0].IntValue &=
+ newFAndBSubSection->Content[0].IntValue;
+ ctx.mergedFAndBSubSection->Content[1].IntValue &=
+ newFAndBSubSection->Content[1].IntValue;
+ ctx.mergedFAndBSubSection->Content[2].IntValue &=
+ newFAndBSubSection->Content[2].IntValue;
+}
+
InputFile::InputFile(Ctx &ctx, Kind k, MemoryBufferRef m)
: ctx(ctx), mb(m), groupId(ctx.driver.nextGroupId), fileKind(k) {
// All files within the same --{start,end}-group get the same group ID.
@@ -552,6 +717,7 @@ template <class ELFT> void ObjFile<ELFT>::parse(bool ignoreComdats) {
// done in parallel.
ArrayRef<Elf_Shdr> objSections = getELFShdrs<ELFT>();
StringRef shstrtab = CHECK2(obj.getSectionStringTable(objSections), this);
+ bool hasGnuProperties = false;
uint64_t size = objSections.size();
sections.resize(size);
for (size_t i = 0; i != size; ++i) {
@@ -574,9 +740,12 @@ template <class ELFT> void ObjFile<ELFT>::parse(bool ignoreComdats) {
.try_emplace(CachedHashStringRef(signature), this)
.second;
if (keepGroup) {
- if (!ctx.arg.resolveGroups)
- sections[i] = createInputSection(
- i, sec, check(obj.getSectionName(sec, shstrtab)));
+ if (!ctx.arg.resolveGroups) {
+ StringRef name = check(obj.getSectionName(sec, shstrtab));
+ if (name == ".note.gnu.property")
+ hasGnuProperties = true;
+ sections[i] = createInputSection(i, sec, name);
+ }
} else {
// Otherwise, discard group members.
for (uint32_t secIndex : entries.slice(1)) {
@@ -638,27 +807,77 @@ template <class ELFT> void ObjFile<ELFT>::parse(bool ignoreComdats) {
}
}
break;
- case EM_AARCH64:
- // FIXME: BuildAttributes have been implemented in llvm, but not yet in
- // lld. Remove the section so that it does not accumulate in the output
- // file. When support is implemented we expect not to output a build
- // attributes section in files of type ET_EXEC or ET_SHARED, but ld -r
- // ouptut will need a single merged attributes section.
- if (sec.sh_type == SHT_AARCH64_ATTRIBUTES)
+ case EM_AARCH64: {
+ // The specification states that if a file contains both GNU properties
+ // and AArch64 build attributes, they must be identical. Therefore, if a
+ // file contains GNU properties, the AArch64 build attributes are ignored.
+ // If a file does not contain GNU properties, we leverage the existing GNU
+ // properties mechanism by populating the corresponding data structures,
+ // which will later be handled by Driver.cpp::readSecurityNotes. This
+ // ensures that AArch64 build attributes are represented in the linked
+ // object file as GNU properties, which are already supported by the Linux
+ // kernel and the dynamic dispatcher.
+ if (sec.sh_type == SHT_AARCH64_ATTRIBUTES) {
+ StringRef name = check(obj.getSectionName(sec, shstrtab));
+ AArch64AttributeParser attributes;
+ ArrayRef<uint8_t> contents = check(obj.getSectionContents(sec));
+ if (Error e = attributes.parse(contents, ELFT::Endianness)) {
+ InputSection isec(*this, sec, name);
+ Warn(ctx) << &isec << ": " << std::move(e);
+ } else {
+ // for functions that has to warn/err/report
+ InputSection isec(*this, sec, name);
+ const SmallVector<llvm::BuildAttributeSubSection, 8>
+ buildAttributesSubSections =
+ attributes.getBuildAttributesSection();
+ auto subsections = extractBuildAttributesSubsection(
+ ctx, buildAttributesSubSections, isec);
+ mergeAArch64BuildAttributes(ctx, subsections, isec);
+ if (!hasGnuProperties) {
+ ObjFile<ELFT> &f = *this;
+ auto [pauthSubSection, fAndBSubSection] = subsections;
+ if (pauthSubSection) {
+ assert(
+ (pauthSubSection->Content.size() == 2) &&
+ "pauthSubSection must contain exactly two build attributes");
+ // sanitizePauthSubSection already sorts
+ f.aarch64PauthAbiCoreInfoStorage =
+ std::make_unique<std::array<uint8_t, 16>>();
+ uint64_t values[2] = {
+ static_cast<uint64_t>(pauthSubSection->Content[0].IntValue),
+ static_cast<uint64_t>(pauthSubSection->Content[1].IntValue)};
+ std::memcpy(f.aarch64PauthAbiCoreInfoStorage->data(), &values[0],
+ sizeof(values));
+ f.aarch64PauthAbiCoreInfo = *f.aarch64PauthAbiCoreInfoStorage;
+ }
+ if (fAndBSubSection) {
+ assert((fAndBSubSection->Content.size() == 3) &&
+ "fAndBSubSection must contain exactly three build "
+ "attributes");
+ // sanitizeFAndBSubSection already sorts
+ f.andFeatures = 0;
+ f.andFeatures |= (fAndBSubSection->Content[0].IntValue) << 0;
+ f.andFeatures |= (fAndBSubSection->Content[1].IntValue) << 1;
+ f.andFeatures |= (fAndBSubSection->Content[2].IntValue) << 2;
+ }
+ }
+ }
sections[i] = &InputSection::discarded;
// Producing a static binary with MTE globals is not currently supported,
// remove all SHT_AARCH64_MEMTAG_GLOBALS_STATIC sections as they're unused
- // medatada, and we don't want them to end up in the output file for
+ // metadata, and we don't want them to end up in the output file for
// static executables.
if (sec.sh_type == SHT_AARCH64_MEMTAG_GLOBALS_STATIC &&
!canHaveMemtagGlobals(ctx))
sections[i] = &InputSection::discarded;
- break;
+ }
}
+ break;
}
// Read a symbol table.
initializeSymbols(obj);
+ }
}
// Sections with SHT_GROUP and comdat bits define comdat section groups.
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index 808cb5d24079f..8164594e9c41a 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -19,6 +19,7 @@
#include "llvm/Object/ELF.h"
#include "llvm/Support/MemoryBufferRef.h"
#include "llvm/Support/Threading.h"
+#include <memory>
namespace llvm {
struct DILineInfo;
@@ -239,9 +240,11 @@ class ELFFileBase : public InputFile {
public:
// Name of source file obtained from STT_FILE, if present.
StringRef sourceFile;
+ std::unique_ptr<std::string> sourceFileStorage;
uint32_t andFeatures = 0;
bool hasCommonSyms = false;
ArrayRef<uint8_t> aarch64PauthAbiCoreInfo;
+ std::unique_ptr<std::array<uint8_t, 16>> aarch64PauthAbiCoreInfoStorage;
};
// .o file.
diff --git a/llvm/include/llvm/Support/ELFAttrParserExtended.h b/llvm/include/llvm/Support/ELFAttrParserExtended.h
index 68f45fb7f368a..6ae43fb0ed75a 100644
--- a/llvm/include/llvm/Support/ELFAttrParserExtended.h
+++ b/llvm/include/llvm/Support/ELFAttrParserExtended.h
@@ -38,6 +38,11 @@ class ELFExtendedAttrParser : public ELFAttributeParser {
virtual ~ELFExtendedAttrParser() { static_cast<void>(!Cursor.takeError()); }
Error parse(ArrayRef<uint8_t> Section, llvm::endianness Endian) override;
+ const SmallVector<BuildAttributeSubSection, 8> &
+ getBuildAttributesSection() const {
+ return SubSectionVec;
+ }
+
std::optional<unsigned> getAttributeValue(unsigned Tag) const override;
std::optional<unsigned> getAttributeValue(StringRef BuildAttrSubsectionName,
unsigned Tag) const override;
diff --git a/llvm/include/llvm/Support/ELFAttributes.h b/llvm/include/llvm/Support/ELFAttributes.h
index 6782aec6050ad..95e4598b2a2b4 100644
--- a/llvm/include/llvm/Support/ELFAttributes.h
+++ b/llvm/include/llvm/Support/ELFAttributes.h
@@ -32,14 +32,20 @@ struct BuildAttributeItem {
unsigned Tag;
unsigned IntValue;
std::string StringValue;
+ BuildAttributeItem(){};
BuildAttributeItem(Types Ty, unsigned Tg, unsigned IV, std::string SV)
: Type(Ty), Tag(Tg), IntValue(IV), StringValue(std::move(SV)) {}
};
struct BuildAttributeSubSection {
- StringRef Name;
+ std::string Name;
unsigned IsOptional;
unsigned ParameterType;
SmallVector<BuildAttributeItem, 64> Content;
+ BuildAttributeSubSection(){};
+ BuildAttributeSubSection(const std::string &N, unsigned Opt, unsigned Type,
+ SmallVector<BuildAttributeItem, 64> &&Content)
+ : Name(N), IsOptional(Opt), ParameterType(Type),
+ Content(std::move(Content)) {}
};
// Tag to string: ELF extended build attribute section
diff --git a/llvm/test/tools/llvm-readobj/ELF/AArch64/Inputs/build-attributes-to-gnu_properties-2.s b/llvm/test/tools/llvm-readobj/ELF/AArch64/Inputs/build-attributes-to-gnu_properties-2.s
new file mode 100644
index 0000000000000..d3e677eee0360
--- /dev/null
+++ b/llvm/test/tools/llvm-readobj/ELF/AArch64/Inputs/build-attributes-to-gnu_properties-2.s
@@ -0,0 +1,10 @@
+# REQUIRES: aarch64
+
+.section .note.gnu.property, "a"
+.aeabi_subsection aeabi_pauthabi, required, uleb128
+.aeabi_attribute Tag_PAuth_Platform, 49
+.aeabi_attribute Tag_PAuth_Schema, 19
+.aeabi_subsection aeabi_feature_and_bits, optional, uleb128
+.aeabi_attribute Tag_Feature_BTI, 1
+.aeabi_attribute Tag_Feature_PAC, 0
+.aeabi_attribute Tag_Feature_GCS, 1
diff --git a/llvm/test/tools/llvm-readobj/ELF/AArch64/Inputs/build-attributes-to-gnu_properties-3.s b/llvm/test/tools/llvm-readobj/ELF/AArch64/Inputs/build-attributes-to-gnu_properties-3.s
new file mode 100644
index 0000000000000..8763000b786c0
--- /dev/null
+++ b/llvm/test/tools/llvm-readobj/ELF/AArch64/Inputs/build-attributes-to-gnu_properties-3.s
@@ -0,0 +1,10 @@
+# REQUIRES: aarch64
+
+.section .note.gnu.property, "a"
+.aeabi_subsection aeabi_pauthabi, required, uleb128
+.aeabi_attribute Tag_PAuth_Platform, 49
+.aeabi_attribute Tag_PAuth_Schema, 19
+.aeabi_subsection aeabi_feature_and_bits, optional, uleb128
+.aeabi_attribute Tag_Feature_BTI, 1
+.aeabi_attribute Tag_Feature_PAC, 1
+.aeabi_attribute Tag_Feature_GCS, 0
diff --git a/llvm/test/tools/llvm-readobj/ELF/AArch64/build-attributes-to-gnu_properties-1.s b/llvm/test/tools/llvm-readobj/ELF/AArch64/build-attributes-to-gnu_properties-1.s
new file mode 100644
index 0000000000000..5ff5f12269333
--- /dev/null
+++ b/llvm/test/tools/llvm-readobj/ELF/AArch64/build-attributes-to-gnu_properties-1.s
@@ -0,0 +1,21 @@
+# RUN: llvm-mc -triple=aarch64 -filetype=obj %s -o %t1.o
+# RUN: llvm-mc -triple=aarch64 -filetype=obj %p/Inputs/build-attributes-to-gnu_properties-2.s -o %t2.o
+# RUN: llvm-mc -triple=aarch64 -filetype=obj %p/Inputs/build-attributes-to-gnu_properties-3.s -o %t3.o
+# RUN: ld.lld -r %t1.o %t2.o %t3.o -o %t.merged.o
+# RUN: llvm-readelf -n %t.merged.o | FileCheck %s
+
+# CHECK: Displaying notes found in: .note.gnu.property
+# CHECK-NEXT: Owner Data size Description
+# CHECK-NEXT: GNU 0x00000028 NT_GNU_PROPERTY_TYPE_0 (property note)
+# CHECK-NEXT: Properties: aarch64 feature: BTI
+# CHECK-NEXT: AArch64 PAuth ABI core info: platform 0x31 (unknown), version 0x13
+
+
+.section .note.gnu.property, "a"
+.aeabi_subsection aeabi_pauthabi, required, uleb128
+.aeabi_attribute Tag_PAuth_Platform, 49
+.aeabi_attribute Tag_PAuth_Schema, 19
+.aeabi_subsection aeabi_feature_and_bits, optional, uleb128
+.aeabi_attribute Tag_Feature_BTI, 1
+.aeabi_attribute Tag_Feature_PAC, 1
+.aeabi_attribute Tag_Feature_GCS, 1
|
sivan-shani
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
seems as my system clang-format (currently 18) needs updating. |
There was a problem hiding this comment.
This is missing tests for:
- The warnings for invalid attributes
- Mixing GNU properties with build attributes, both within one object file and as different input files
- The effect of build attributes on a non-partial link. There are probably existing tests for PAC/BTI/GCS/pauthabi which you could extend to check that they get enabled by build attributes as well as GNU properties
lld/ELF/InputFiles.cpp
Outdated
| // ouptut will need a single merged attributes section. | ||
| if (sec.sh_type == SHT_AARCH64_ATTRIBUTES) | ||
| case EM_AARCH64: { | ||
| // The specification states that if a file contains both GNU properties |
There was a problem hiding this comment.
The second sentence here doesn't logically follow from the first. The spec allows for this behaviour, but it would be better to check that the two encodings match (if both are present in one object file), and emit a warning if they do not.
There was a problem hiding this comment.
Changing to ...they can be assumed to be identical
Adding check and warning for when both exists and aarch64 build attributes are ignored.
There was a problem hiding this comment.
To be clear, while I'm happy for this error checking to not be included in this PR, it's still very important because silently ignoring a build attribute can result in a security feature being disabled.
There was a problem hiding this comment.
This seems to depend also on your comment below:
What if the input object has a GNU properties section, but it does not have both of GNU_PROPERTY_AARCH64_FEATURE_1_AND and GNU_PROPERTY_AARCH64_FEATURE_PAUTH? In that case, we still need to parse the build attribute subsections which don't have the matching GNU properties subsections.
If it is indeed possible to have any of the subsections in one file but not in the other, the files will have to be read. Which is basically depends on the meaning of The specification states that if a file contains both GNU properties and AArch64 build attributes, they can be assumed to be identical.
There was a problem hiding this comment.
Now GNU properties content is being checked
| @@ -239,9 +240,11 @@ class ELFFileBase : public InputFile { | |||
| public: | |||
| // Name of source file obtained from STT_FILE, if present. | |||
| StringRef sourceFile; | |||
| std::unique_ptr<std::string> sourceFileStorage; | |||
| uint32_t andFeatures = 0; | |||
| bool hasCommonSyms = false; | |||
| ArrayRef<uint8_t> aarch64PauthAbiCoreInfo; | |||
There was a problem hiding this comment.
Will this ever point anywhere other than aarch64PauthAbiCoreInfoStorage? Do we need both?
There was a problem hiding this comment.
Will this ever point anywhere other than aarch64PauthAbiCoreInfoStorage?
No
Do we need both
Yes
In other places aarch64PauthAbiCoreInfo is being pointed at consistent memory that is alive longer then the intended usage of this variable. It has not been designed to point at memory that is going out of scope.
Examples: InputFiles.cpp::parseGnuPropertyNote has f.aarch64PauthAbiCoreInfo = desc and Driver.cpp::readSecurityNotes has ctx.aarch64PauthAbiCoreInfo = (*it)->aarch64PauthAbiCoreInfo.
However, since we have to point aarch64PauthAbiCoreInfo to memory that will no longer be alive beyond scope, aarch64PauthAbiCoreInfoStorage had to be introduced.
There was a problem hiding this comment.
An alternative that involves a bit of refactoring of the existing aarch64PauthABICoreInfo, which is optimal for a note section, but not ideal for build attributes, is to deserialize the aarch64PauthABICoreInfo and store the members as two uint64_t instead of an arrayRef. We'll need to update the comparison code in Driver.cpp and in the .note.gnu.property writing code, but it may be neater overall.
There was a problem hiding this comment.
I agree that the proposed approach could lead to a cleaner design overall, but given the scope and context of the current changes, it feels a bit too involved to refactor at this stage. It might be better to revisit this as a follow-up if needed.
|
Thank you for the review, I believe comments were addressed, if I have missed anything, please let me know. |
|
Note regarding tests files:
In addition, |
| @@ -0,0 +1,23 @@ | |||
|
|
|||
There was a problem hiding this comment.
Don't start with unneeded \n.
yaml Content is difficult to updated. Use assembly test with some comments, e.g.
% cat x.s
.section .ARM.attributes,"",%0x70000003
.long 0 //
.long 0 //
lld/ELF/InputFiles.cpp
Outdated
| auto baPauth = serializeUnsigned(baInfo.Pauth.TagPlatform, | ||
| baInfo.Pauth.TagSchema, isBE); | ||
| if (gpInfo.aarch64PauthAbiCoreInfo != ArrayRef<uint8_t>(baPauth)) | ||
| ErrAlways(ctx) |
There was a problem hiding this comment.
Prefer Err for recoverable, non-fatal errors. --noinhibit-exec will produce an output
| /// llvm-mc should not appear in the output of lld, because | ||
| /// AArch64 build attributes are being transformed into .gnu.properties. | ||
|
|
||
| // Test mc -> big endian, lld -> little endian |
There was a problem hiding this comment.
/// for non-RUN non-CHECK comments to make them stand out. (convention in lld tests; alternatively, use # for RUN/CHECK and ## for comments)
…ldAttributeSubSection::Name (llvm#135625) BuildAttributeSubSection::Name must be a std::string instead of StringRef because it may be assigned from non-persistent memory. StringRef is non-owning and unsafe in this context. This change ensures the subsection name owns its memory, preventing use-after-free or dangling references. Context: Work in progress in PR llvm#131990.
|
All handled, I hope it is good to go. |
| @@ -32,9 +32,11 @@ struct BuildAttributeItem { | |||
| unsigned Tag; | |||
| unsigned IntValue; | |||
| std::string StringValue; | |||
| BuildAttributeItem() {}; | |||
There was a problem hiding this comment.
This is error-prone. Can this constructor be removed instead?
lld/ELF/InputFiles.cpp
Outdated
| @@ -539,6 +540,69 @@ uint32_t ObjFile<ELFT>::getSectionIndex(const Elf_Sym &sym) const { | |||
| this); | |||
| } | |||
|
|
|||
| // Forward declarations: | |||
There was a problem hiding this comment.
nit: This comment its redundant, it is quite clear these are forward declarations.
There was a problem hiding this comment.
it is quite clear these are forward declarations.
That's debatable.
One of the purpose of a comment is to clarify and reduce mental stress involved in understanding code, and considering this bit of code with and without this comment, the comment seems to me to be a net positive here.
There was a problem hiding this comment.
In that case, it might be worth saying why we have the forward declarations. Something like
// Forward Declarations of use when parsing AArch64 Build Attributes
At the moment this comment is describing a what, my understanding is a comment should be explaining a why. The code should easily describe what is happening. If it is felt the comment is useful, then let's explain why the forward declarations are there.
I also think the forward declaration for parseGnuPropertyNote is redundant. It is called twice in this code, both after the function definition.
It is also good to keep the declarations as close as possible to the place they are needed, so readGnuProperty should be moved above ObjFile<ELFT>::parse.
There was a problem hiding this comment.
I also think the forward declaration for
parseGnuPropertyNoteis redundant.
Compiling the code after commenting out the forward declaration for parseGnuPropertyNote results with a linker error:
.../data/sivsha01/Repos/llvm-project/lld/ELF/InputFiles.cpp:1555:5: error: no matching function for call to 'parseGnuPropertyNote' 1555 | parseGnuPropertyNote<ELFT>(ctx, *this, GNU_PROPERTY_AARCH64_FEATURE_1_AND,...
At the moment this comment is describing a what, my understanding is a comment should be explaining a why
I see your point, but I believe the current comment serves a useful purpose by providing a quick mental anchor for readers scanning through the file. While the code does indicate what these lines are, the comment reduces friction in identifying the section's intent without having to interpret syntax or context. I’d prefer to keep it as-is, as I think it aids readability in practice.
Edit: To me the why seems the quite clear part here.
There was a problem hiding this comment.
.../data/sivsha01/Repos/llvm-project/lld/ELF/InputFiles.cpp:1555:5: error: no matching function for call to 'parseGnuPropertyNote' 1555 | parseGnuPropertyNote<ELFT>(ctx, *this, GNU_PROPERTY_AARCH64_FEATURE_1_AND,...
Did you test it with the nullptr default value present in the function definition? The SharedFile call does not use this parameter. If this was not added back, I expect to see this error.
Ok. Lets leave the comment as is, but I still believe the forward definitions can be moved closer to their first use as I indicated and cut down to only include readGnuProperty
There was a problem hiding this comment.
Did you test it with the
nullptr
Yes, seems as you are correct about this! I did forget to put the default parameter back in the original definition.
Will fix this up.
There was a problem hiding this comment.
The // Forward declaration: comment is redundant. It does fit well with the style of LLVM. Don't add it.
|
Comments handled. |
|
@MaskRay In case you are back |
|
@MaskRay ping |
|
I've simplified this branch, though the changes aren't final and may need further adjustments. (I've reviewed thousands of patches-over 1,000 on GitHub alone, and likely more on Phabricator - from hundreds of contributors.) |
| bool isBE) -> std::array<uint8_t, 16> { | ||
| std::array<uint8_t, 16> arr; | ||
| for (size_t i = 0; i < 8; ++i) { | ||
| arr[i] = static_cast<uint8_t>( |
There was a problem hiding this comment.
I've simplified this in my pr131990 branch, but why do we need this serialization at all?
There was a problem hiding this comment.
There are 2 reasons for the serialization of baInfo.Pauth.TagPlatform and baInfo.Pauth.TagSchema into an array:
- In order to compare them to the equivalent Gnu Properties values (
gpInfo.aarch64PauthAbiCoreInfo) - In order to store them in
file->aarch64PauthAbiCoreInfoStorage
While the second seems unavoidable, the first could be replaced with reading the values from the gpInfo.aarch64PauthAbiCoreInfo array.
Reading the values from the array has the advantage of allowing more granular error messages.
On the other hand, it require some more operations.
Will commit a version that reads the values from the array separately so it can be decided which one is preferable.
There was a problem hiding this comment.
In some conversations I've had with the PAuthABI team, they've wanted a property/build-attribute that would be the equivalent of the command-line option -z pac-plt ostensibly to prevent user-error if the linker is called directly (the clang driver could add this automatically).
In that case it would be inconvenient to store the PAuthABICoreInfo as an array_ref only used for comparison.
Would definitely like to see an implementation where both BuildAttributes and GNU properties decode to the same individual properties. Then in https://github.com/llvm/llvm-project/blob/main/lld/ELF/SyntheticSections.cpp#L347 was can use a write32() call for each property rather than memcpy.
|
Thanks for the feedback, will push changes after they will be reviewed downstream. Would it be preferable to to abandon this PR and start a new one, rebased on main? |
This patch enables
lldto read AArch64 Build Attributes and convert them into GNU Properties.Changes: