Skip to content

[AArch64][GCS][LLD] Introduce -zgcs-report-dynamic Command Line Option #127787

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

Merged
merged 9 commits into from
Mar 16, 2025
Merged

[AArch64][GCS][LLD] Introduce -zgcs-report-dynamic Command Line Option #127787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
84b665c
[AArch64][GCS] Convert -zgcs-report processing to use an Enum
Stylie777 Feb 18, 2025
79fd58d
[AArch64][GCS][LLD] Introduce -z gcs-report-dynamic option
Stylie777 Feb 17, 2025
b37c82e
Simplify code
MaskRay Mar 11, 2025
5660484
Respond to Review Comments
Stylie777 Mar 11, 2025
bdea2f7
Remove `gcsReportPolicytoString` function
Stylie777 Mar 12, 2025
c2af177
Fix release notes issue
Stylie777 Mar 12, 2025
64e6544
Fix formatting in InputFiles.cpp
Stylie777 Mar 12, 2025
1d2db0b
Update parsing of gcs-report and gcs-report-dynamic options
Stylie777 Mar 13, 2025
d85481c
make code more conventional; simplify code; use just diag prefix in C…
MaskRay Mar 16, 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
1 change: 1 addition & 0 deletions lld/ELF/Config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ struct Config {
ReportPolicy zCetReport = ReportPolicy::None;
ReportPolicy zPauthReport = ReportPolicy::None;
ReportPolicy zGcsReport = ReportPolicy::None;
ReportPolicy zGcsReportDynamic = ReportPolicy::None;
ReportPolicy zExecuteOnlyReport = ReportPolicy::None;
bool ltoBBAddrMap;
llvm::StringRef ltoBasicBlockSections;
Expand Down
29 changes: 28 additions & 1 deletion lld/ELF/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,13 @@
#include "llvm/Object/IRObjectFile.h"
#include "llvm/Remarks/HotnessThresholdParser.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/SaveAndRestore.h"
#include "llvm/Support/Compression.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/GlobPattern.h"
#include "llvm/Support/LEB128.h"
#include "llvm/Support/Parallel.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/SaveAndRestore.h"
#include "llvm/Support/TarWriter.h"
#include "llvm/Support/TargetSelect.h"
#include "llvm/Support/TimeProfiler.h"
Expand Down Expand Up @@ -402,6 +402,8 @@ static void checkOptions(Ctx &ctx) {
ErrAlways(ctx) << "-z pauth-report only supported on AArch64";
if (ctx.arg.zGcsReport != ReportPolicy::None)
ErrAlways(ctx) << "-z gcs-report only supported on AArch64";
if (ctx.arg.zGcsReportDynamic != ReportPolicy::None)
ErrAlways(ctx) << "-z gcs-report-dynamic only supported on AArch64";
if (ctx.arg.zGcs != GcsPolicy::Implicit)
ErrAlways(ctx) << "-z gcs only supported on AArch64";
}
Expand Down Expand Up @@ -1625,7 +1627,9 @@ static void readConfigs(Ctx &ctx, opt::InputArgList &args) {
std::make_pair("cet-report", &ctx.arg.zCetReport),
std::make_pair("execute-only-report", &ctx.arg.zExecuteOnlyReport),
std::make_pair("gcs-report", &ctx.arg.zGcsReport),
std::make_pair("gcs-report-dynamic", &ctx.arg.zGcsReportDynamic),
std::make_pair("pauth-report", &ctx.arg.zPauthReport)};
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to handle gcs-report and gcs-report-dynamic here. We can add a special case to track whether gcs-report-dynamic has been specified

Copy link
Contributor Author

@Stylie777 Stylie777 Mar 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree, I think it is better to handle the options in the location currently being used by this change. I have implemented the inheritance so it tries to find both gcs-report and gcs-report-dynamic at the same time. The inheritance is then implemented and it will all be contained in its own function. So I prefer to use the function that has been created and utilise this. Implementing the inheritance at this location I feel would not be a good solution, and may be harder to maintain in the future than the current getZGcsReport function.

If it makes it clearer for the user, I can add a comment explaining that -zgcs-report and -zgcs-report-dynamic are both handled inside that function.

Copy link
Member

@MaskRay MaskRay Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree. There is significant duplication and code bloat due to none/warning/error parsing.
You could add

if (option.first == "gcs-report-dynamic")
  reportDynamicSpecified = true

then after the for loop,

  if (!reportDynamicDefined && ctx.arg.zGcsReport != ReportPolicy::None &&
      ctx.arg.zGcsReportDynamic == ReportPolicy::None)
    ctx.arg.zGcsReportDynamic = ReportPolicy::Warning;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have done this. Thanks

bool hasGcsReportDynamic = false;
for (opt::Arg *arg : args.filtered(OPT_z)) {
std::pair<StringRef, StringRef> option =
StringRef(arg->getValue()).split('=');
Expand All @@ -1644,9 +1648,16 @@ static void readConfigs(Ctx &ctx, opt::InputArgList &args) {
<< "= value: " << option.second;
continue;
}
hasGcsReportDynamic |= option.first == "gcs-report-dynamic";
}
}

// When -zgcs-report-dynamic is unspecified, it inherits -zgcs-report
// but is capped at warning to avoid needing to rebuild the shared library
// with GCS enabled.
if (!hasGcsReportDynamic && ctx.arg.zGcsReport != ReportPolicy::None)
ctx.arg.zGcsReportDynamic = ReportPolicy::Warning;

for (opt::Arg *arg : args.filtered(OPT_compress_sections)) {
SmallVector<StringRef, 0> fields;
StringRef(arg->getValue()).split(fields, '=');
Expand Down Expand Up @@ -2907,6 +2918,22 @@ static void readSecurityNotes(Ctx &ctx) {
ctx.arg.andFeatures |= GNU_PROPERTY_AARCH64_FEATURE_1_GCS;
else if (ctx.arg.zGcs == GcsPolicy::Never)
ctx.arg.andFeatures &= ~GNU_PROPERTY_AARCH64_FEATURE_1_GCS;

// If we are utilising GCS at any stage, the sharedFiles should be checked to
// ensure they also support this feature. The gcs-report-dynamic option is
// used to indicate if the user wants information relating to this, and will
// be set depending on the user's input, or warning if gcs-report is set to
// either `warning` or `error`.
if (ctx.arg.andFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
for (SharedFile *f : ctx.sharedFiles)
reportUnless(ctx.arg.zGcsReportDynamic,
f->andFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
<< f
<< ": GCS is required by -z gcs, but this shared library lacks the "
"necessary property note. The "
<< "dynamic loader might not enable GCS or refuse to load the "
"program unless all shared library "
<< "dependencies have the GCS marking.";
}

static void initSectionsAndLocalSyms(ELFFileBase *file, bool ignoreComdats) {
Expand Down
111 changes: 75 additions & 36 deletions lld/ELF/InputFiles.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,56 @@ void ObjFile<ELFT>::initializeSections(bool ignoreComdats,
handleSectionGroup<ELFT>(this->sections, entries);
}

template <typename ELFT>
static void parseGnuPropertyNote(Ctx &ctx, ELFFileBase &f,
uint32_t featureAndType,
ArrayRef<uint8_t> &desc, const uint8_t *base,
ArrayRef<uint8_t> *data = nullptr) {
auto err = [&](const uint8_t *place) -> ELFSyncStream {
auto diag = Err(ctx);
diag << &f << ":(" << ".note.gnu.property+0x"
<< Twine::utohexstr(place - base) << "): ";
return diag;
};

while (!desc.empty()) {
const uint8_t *place = desc.data();
if (desc.size() < 8)
return void(err(place) << "program property is too short");
uint32_t type = read32<ELFT::Endianness>(desc.data());
uint32_t size = read32<ELFT::Endianness>(desc.data() + 4);
desc = desc.slice(8);
if (desc.size() < size)
return void(err(place) << "program property is too short");

if (type == featureAndType) {
// We found a FEATURE_1_AND field. There may be more than one of these
// in a .note.gnu.property section, for a relocatable object we
// accumulate the bits set.
if (size < 4)
return void(err(place) << "FEATURE_1_AND entry is too short");
f.andFeatures |= read32<ELFT::Endianness>(desc.data());
} else if (ctx.arg.emachine == EM_AARCH64 &&
type == GNU_PROPERTY_AARCH64_FEATURE_PAUTH) {
ArrayRef<uint8_t> contents = data ? *data : desc;
if (!f.aarch64PauthAbiCoreInfo.empty()) {
return void(
err(contents.data())
<< "multiple GNU_PROPERTY_AARCH64_FEATURE_PAUTH entries are "
"not supported");
} else if (size != 16) {
return void(err(contents.data())
<< "GNU_PROPERTY_AARCH64_FEATURE_PAUTH entry "
"is invalid: expected 16 bytes, but got "
<< size);
}
f.aarch64PauthAbiCoreInfo = desc;
}

// Padding is present in the note descriptor, if necessary.
desc = desc.slice(alignTo<(ELFT::Is64Bits ? 8 : 4)>(size));
}
}
// Read the following info from the .note.gnu.property section and write it to
// the corresponding fields in `ObjFile`:
// - Feature flags (32 bits) representing x86 or AArch64 features for
Expand Down Expand Up @@ -955,42 +1005,8 @@ static void readGnuProperty(Ctx &ctx, const InputSection &sec,

// Read a body of a NOTE record, which consists of type-length-value fields.
ArrayRef<uint8_t> desc = note.getDesc(sec.addralign);
while (!desc.empty()) {
const uint8_t *place = desc.data();
if (desc.size() < 8)
return void(err(place) << "program property is too short");
uint32_t type = read32<ELFT::Endianness>(desc.data());
uint32_t size = read32<ELFT::Endianness>(desc.data() + 4);
desc = desc.slice(8);
if (desc.size() < size)
return void(err(place) << "program property is too short");

if (type == featureAndType) {
// We found a FEATURE_1_AND field. There may be more than one of these
// in a .note.gnu.property section, for a relocatable object we
// accumulate the bits set.
if (size < 4)
return void(err(place) << "FEATURE_1_AND entry is too short");
f.andFeatures |= read32<ELFT::Endianness>(desc.data());
} else if (ctx.arg.emachine == EM_AARCH64 &&
type == GNU_PROPERTY_AARCH64_FEATURE_PAUTH) {
if (!f.aarch64PauthAbiCoreInfo.empty()) {
return void(
err(data.data())
<< "multiple GNU_PROPERTY_AARCH64_FEATURE_PAUTH entries are "
"not supported");
} else if (size != 16) {
return void(err(data.data())
<< "GNU_PROPERTY_AARCH64_FEATURE_PAUTH entry "
"is invalid: expected 16 bytes, but got "
<< size);
}
f.aarch64PauthAbiCoreInfo = desc;
}

// Padding is present in the note descriptor, if necessary.
desc = desc.slice(alignTo<(ELFT::Is64Bits ? 8 : 4)>(size));
}
const uint8_t *base = sec.content().data();
parseGnuPropertyNote<ELFT>(ctx, f, featureAndType, desc, base, &data);

// Go to next NOTE record to look for more FEATURE_1_AND descriptions.
data = data.slice(nhdr->getSize(sec.addralign));
Expand Down Expand Up @@ -1418,6 +1434,28 @@ std::vector<uint32_t> SharedFile::parseVerneed(const ELFFile<ELFT> &obj,
return verneeds;
}

// Parse PT_GNU_PROPERTY segments in DSO. The process is similar to
// readGnuProperty, but we don't have the InputSection information.
template <typename ELFT>
void SharedFile::parseGnuAndFeatures(const ELFFile<ELFT> &obj) {
if (ctx.arg.emachine != EM_AARCH64)
return;
const uint8_t *base = obj.base();
auto phdrs = CHECK2(obj.program_headers(), this);
for (auto phdr : phdrs) {
if (phdr.p_type != PT_GNU_PROPERTY)
continue;
typename ELFT::Note note(
*reinterpret_cast<const typename ELFT::Nhdr *>(base + phdr.p_offset));
if (note.getType() != NT_GNU_PROPERTY_TYPE_0 || note.getName() != "GNU")
continue;

ArrayRef<uint8_t> desc = note.getDesc(phdr.p_align);
parseGnuPropertyNote<ELFT>(ctx, *this, GNU_PROPERTY_AARCH64_FEATURE_1_AND,
desc, base);
}
}

// We do not usually care about alignments of data in shared object
// files because the loader takes care of it. However, if we promote a
// DSO symbol to point to .bss due to copy relocation, we need to keep
Expand Down Expand Up @@ -1528,6 +1566,7 @@ template <class ELFT> void SharedFile::parse() {

verdefs = parseVerdefs<ELFT>(obj.base(), verdefSec);
std::vector<uint32_t> verneeds = parseVerneed<ELFT>(obj, verneedSec);
parseGnuAndFeatures<ELFT>(obj);

// Parse ".gnu.version" section which is a parallel array for the symbol
// table. If a given file doesn't have a ".gnu.version" section, we use
Expand Down
2 changes: 2 additions & 0 deletions lld/ELF/InputFiles.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,8 @@ class SharedFile : public ELFFileBase {
template <typename ELFT>
std::vector<uint32_t> parseVerneed(const llvm::object::ELFFile<ELFT> &obj,
const typename ELFT::Shdr *sec);
template <typename ELFT>
void parseGnuAndFeatures(const llvm::object::ELFFile<ELFT> &obj);
};

class BinaryFile : public InputFile {
Expand Down
4 changes: 4 additions & 0 deletions lld/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ Non-comprehensive list of changes in this release

ELF Improvements
----------------
* For AArch64, added support for ``-zgcs-report-dynamic``, enabling checks for
GNU GCS Attribute Flags in Dynamic Objects when GCS is enabled. Inherits value
from ``-zgcs-report`` (capped at ``warning`` level) unless user-defined,
ensuring compatibility with GNU ld linker.

Breaking changes
----------------
Expand Down
18 changes: 17 additions & 1 deletion lld/test/ELF/aarch64-feature-gcs.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,26 @@
# REPORT-WARN: warning: func2.o: -z gcs-report: file does not have GNU_PROPERTY_AARCH64_FEATURE_1_GCS property
# REPORT-ERROR: error: func3.o: -z gcs-report: file does not have GNU_PROPERTY_AARCH64_FEATURE_1_GCS property

## gcs-report-dynamic should report any dynamic objects that does not have the gcs property. This also ensures the inhertance from gcs-report is working correctly.

# RUN: ld.lld func1-gcs.o func3-gcs.o no-gcs.so force-gcs.so -z gcs-report=warning -z gcs=always 2>&1 | FileCheck --check-prefix=REPORT-WARN-DYNAMIC %s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test case with only inputs that have the marking force-gcs.so and -zgcs-report=error and -zgcs-report-dynamic=error? We expect no error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added 6 tests to cover all use cases where this new option could trigger a warning/error to ensure it does not.

Copy link
Member

@MaskRay MaskRay Mar 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

lld runs quickly, but 10+ invocations are too much for a relatively minor feature. Can the tests be simplified while covering all the interesting cases?

Copy link
Member

Choose a reason for hiding this comment

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

Note: "Resolve conversion" is reserved for reviewers per recommendation on https://discourse.llvm.org/t/rfc-github-pr-resolve-conversation-button/73178

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, will make sure I follow that (sorry I had not seen this comment and have resolved some conversations this morning)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have reduced it to 6 total test cases, this should still cover alot of cases to ensure the option is working as intended.

# RUN: ld.lld func1-gcs.o func3-gcs.o no-gcs.so force-gcs.so -z gcs-report=error -z gcs=always 2>&1 | FileCheck --check-prefix=REPORT-WARN-DYNAMIC %s
# RUN: ld.lld func1-gcs.o func3-gcs.o no-gcs.so force-gcs.so -z gcs-report-dynamic=warning -z gcs=always 2>&1 | FileCheck --check-prefix=REPORT-WARN-DYNAMIC %s
# RUN: not ld.lld func1-gcs.o func3-gcs.o no-gcs.so force-gcs.so -z gcs-report-dynamic=error -z gcs=always 2>&1 | FileCheck --check-prefix=REPORT-ERROR-DYNAMIC %s
# RUN: ld.lld func1-gcs.o func3-gcs.o force-gcs.so -z gcs-report-dynamic=warning -z gcs=always 2>&1 | count 0
# RUN: ld.lld func1-gcs.o func3-gcs.o force-gcs.so -z gcs-report-dynamic=error -z gcs=always 2>&1 | count 0

# REPORT-WARN-DYNAMIC: warning: no-gcs.so: GCS is required by -z gcs, but this shared library lacks the necessary property note. The dynamic loader might not enable GCS or refuse to load the program unless all shared library dependencies have the GCS marking.
# REPORT-WARN-DYNAMIC-NOT: warning:
# REPORT-ERROR-DYNAMIC: error: no-gcs.so: GCS is required by -z gcs, but this shared library lacks the necessary property note. The dynamic loader might not enable GCS or refuse to load the program unless all shared library dependencies have the GCS marking.
# REPORT-ERROR-DYNAMIC-NOT: error:

## An invalid gcs option should give an error
# RUN: not ld.lld func1-gcs.o func2-gcs.o func3-gcs.o -z gcs=nonsense 2>&1 | FileCheck --check-prefix=INVALID %s
# RUN: not ld.lld func1-gcs.o func2-gcs.o func3-gcs.o -z gcs=nonsense -z gcs-report=nonsense -z gcs-report-dynamic=nonsense 2>&1 | FileCheck --check-prefix=INVALID %s

# INVALID: error: unknown -z gcs= value: nonsense
# INVALID: error: unknown -z gcs-report= value: nonsense
# INVALID: error: unknown -z gcs-report-dynamic= value: nonsense

#--- func1-gcs.s
.section ".note.gnu.property", "a"
Expand Down