[AArch64][GCS][LLD] Introduce -zgcs-report-dynamic Command Line Option #127787
Conversation
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-lld-elf Author: Jack Styles (Stylie777) ChangesWhen GCS was introduced to LLD, the gcs-report option allowed for a user to gain information relating to if their relocatable objects supported the feature. For an executable or shared-library to support GCS, all relocatable objects must declare that they support GCS. The gcs-report checks were only done on relocatable object files, however for a program to enable GCS, the executable and all shared libraries that it loads must enable GCS. gcs-report-dynamic enables checks to be performed on all shared objects loaded by LLD, and in cases where GCS is not supported, a warning or error will be emitted. It should be noted that only shared files directly passed to LLD are checked for GCS support. Files that are noted in the The behaviour of the -zgcs-dynamic-report option matches that of GNU ld. The behaviour is as follows unless the user explicitly sets the value:
This approach avoids inheriting an error level if the user wishes to continue building a module without rebuilding all the shared libraries. The same approach was taken for the GNU ld linker, so behaviour is identical across the toolchains. This implementation matches the error message and command line interface used within the GNU ld Linker. See here: To support this option being introduced, two other changes are included as part of this PR. The first converts the -zgcs-report option to utilise an Enum, opposed to StringRef values. This enables easier tracking of the value the user defines when inheriting the value for the gas-report-dynamic option. The second is to parse the Dynamic Objects program headers to locate the GNU Attribute flag that shows GCS is supported. This is needed so, when using the gcs-report-dynamic option, LLD can correctly determine if a dynamic object supports GCS. Full diff: https://github.com/llvm/llvm-project/pull/127787.diff 6 Files Affected:
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index f132b11b20c63..3221c83d499e1 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -136,6 +136,9 @@ enum LtoKind : uint8_t {UnifiedThin, UnifiedRegular, Default};
// For -z gcs=
enum class GcsPolicy { Implicit, Never, Always };
+// For -z gcs-report= and -zgcs-report-dynamic
+enum class GcsReportPolicy { None, Warning, Error };
+
struct SymbolVersion {
llvm::StringRef name;
bool isExternCpp;
@@ -228,7 +231,6 @@ struct Config {
StringRef zBtiReport = "none";
StringRef zCetReport = "none";
StringRef zPauthReport = "none";
- StringRef zGcsReport = "none";
bool ltoBBAddrMap;
llvm::StringRef ltoBasicBlockSections;
std::pair<llvm::StringRef, llvm::StringRef> thinLTOObjectSuffixReplace;
@@ -393,6 +395,8 @@ struct Config {
UnresolvedPolicy unresolvedSymbolsInShlib;
Target2Policy target2;
GcsPolicy zGcs;
+ GcsReportPolicy zGcsReport;
+ GcsReportPolicy zGcsReportDynamic;
bool power10Stubs;
ARMVFPArgKind armVFPArgs = ARMVFPArgKind::Default;
BuildIdKind buildId = BuildIdKind::None;
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index 70a293875f27b..e9098ff94f1a4 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -49,11 +49,15 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
+#include "llvm/BinaryFormat/ELF.h"
#include "llvm/Config/llvm-config.h"
#include "llvm/LTO/LTO.h"
#include "llvm/Object/Archive.h"
+#include "llvm/Object/ELFTypes.h"
#include "llvm/Object/IRObjectFile.h"
+#include "llvm/Option/ArgList.h"
#include "llvm/Remarks/HotnessThresholdParser.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/SaveAndRestore.h"
@@ -68,6 +72,7 @@
#include "llvm/Support/TimeProfiler.h"
#include "llvm/Support/raw_ostream.h"
#include <cstdlib>
+#include <memory>
#include <tuple>
#include <utility>
@@ -400,8 +405,10 @@ static void checkOptions(Ctx &ctx) {
ErrAlways(ctx) << "-z bti-report only supported on AArch64";
if (ctx.arg.zPauthReport != "none")
ErrAlways(ctx) << "-z pauth-report only supported on AArch64";
- if (ctx.arg.zGcsReport != "none")
+ if (ctx.arg.zGcsReport != GcsReportPolicy::None)
ErrAlways(ctx) << "-z gcs-report only supported on AArch64";
+ if(ctx.arg.zGcsReportDynamic != GcsReportPolicy::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";
}
@@ -569,6 +576,57 @@ static GcsPolicy getZGcs(Ctx &ctx, opt::InputArgList &args) {
return ret;
}
+static GcsReportPolicy getZGcsReport(Ctx &ctx, opt::InputArgList &args) {
+ GcsReportPolicy ret = GcsReportPolicy::None;
+
+ for (auto *arg : args.filtered(OPT_z)) {
+ std::pair<StringRef, StringRef> kv = StringRef(arg->getValue()).split('=');
+ if (kv.first == "gcs-report") {
+ arg->claim();
+ if (kv.second == "none")
+ ret = GcsReportPolicy::None;
+ else if (kv.second == "warning")
+ ret = GcsReportPolicy::Warning;
+ else if (kv.second == "error")
+ ret = GcsReportPolicy::Error;
+ else
+ ErrAlways(ctx) << "unknown -z gcs-report= value: " << kv.second;
+ }
+ }
+
+ return ret;
+}
+
+static GcsReportPolicy getZGcsReportDynamic(Ctx &ctx, opt::InputArgList &args) {
+ GcsReportPolicy ret = GcsReportPolicy::None;
+ for (auto *arg : args.filtered(OPT_z)) {
+ std::pair<StringRef, StringRef> kv = StringRef(arg->getValue()).split('=');
+ if (kv.first == "gcs-report-dynamic") {
+ arg->claim();
+ if (kv.second == "none")
+ ret = GcsReportPolicy::None;
+ else if (kv.second == "warning")
+ ret = GcsReportPolicy::Warning;
+ else if (kv.second == "error")
+ ret = GcsReportPolicy::Error;
+ else
+ ErrAlways(ctx) << "unknown -z gcs-report-dynamic= value: " << kv.second;
+ // once the gcs-report-dynamic option has been processed, we want to break
+ // from the loop to ensure we do not overwrite the return value if the
+ // user has also passed a value for the gcs-report option.
+ break;
+ }
+ // If the user has not defined a value for gcs-report-dynamic, but has for
+ // gcs-report, we want to inherit that value for gcs-report-dynamic. This is
+ // capped at a warning to ensure a users module can still build, while providing
+ // information relating to if a dynamic object supports GCS.
+ if (kv.first == "gcs-report" && (kv.second == "warning" || kv.second == "error"))
+ ret = GcsReportPolicy::Warning;
+ }
+
+ return ret;
+}
+
// Report a warning for an unknown -z option.
static void checkZOptions(Ctx &ctx, opt::InputArgList &args) {
// This function is called before getTarget(), when certain options are not
@@ -1548,6 +1606,8 @@ static void readConfigs(Ctx &ctx, opt::InputArgList &args) {
ctx.arg.zForceBti = hasZOption(args, "force-bti");
ctx.arg.zForceIbt = hasZOption(args, "force-ibt");
ctx.arg.zGcs = getZGcs(ctx, args);
+ ctx.arg.zGcsReport = getZGcsReport(ctx, args);
+ ctx.arg.zGcsReportDynamic = getZGcsReportDynamic(ctx, args);
ctx.arg.zGlobal = hasZOption(args, "global");
ctx.arg.zGnustack = getZGnuStack(args);
ctx.arg.zHazardplt = hasZOption(args, "hazardplt");
@@ -1622,7 +1682,6 @@ static void readConfigs(Ctx &ctx, opt::InputArgList &args) {
auto reports = {std::make_pair("bti-report", &ctx.arg.zBtiReport),
std::make_pair("cet-report", &ctx.arg.zCetReport),
- std::make_pair("gcs-report", &ctx.arg.zGcsReport),
std::make_pair("pauth-report", &ctx.arg.zPauthReport)};
for (opt::Arg *arg : args.filtered(OPT_z)) {
std::pair<StringRef, StringRef> option =
@@ -2825,6 +2884,16 @@ static void readSecurityNotes(Ctx &ctx) {
return {ctx, DiagLevel::None};
return report(config);
};
+ auto reportGcsPolicy = [&](GcsReportPolicy config, bool cond) -> ELFSyncStream {
+ if (cond)
+ return {ctx, DiagLevel::None};
+ StringRef configString = "none";
+ if(config == GcsReportPolicy::Warning)
+ configString = "warning";
+ else if (config == GcsReportPolicy::Error)
+ configString = "error";
+ return report(configString);
+ };
for (ELFFileBase *f : ctx.objectFiles) {
uint32_t features = f->andFeatures;
@@ -2834,7 +2903,7 @@ static void readSecurityNotes(Ctx &ctx) {
<< ": -z bti-report: file does not have "
"GNU_PROPERTY_AARCH64_FEATURE_1_BTI property";
- reportUnless(ctx.arg.zGcsReport,
+ reportGcsPolicy(ctx.arg.zGcsReport,
features & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
<< f
<< ": -z gcs-report: file does not have "
@@ -2904,6 +2973,16 @@ 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)
+ reportGcsPolicy(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 "
+ << "dependancies have the GCS marking.";
}
static void initSectionsAndLocalSyms(ELFFileBase *file, bool ignoreComdats) {
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index d43de8ce6dfef..055dd5ae0a668 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -18,8 +18,10 @@
#include "Target.h"
#include "lld/Common/CommonLinkerContext.h"
#include "lld/Common/DWARF.h"
+#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/CachedHashString.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/BinaryFormat/ELF.h"
#include "llvm/LTO/LTO.h"
#include "llvm/Object/IRObjectFile.h"
#include "llvm/Support/ARMAttributeParser.h"
@@ -502,6 +504,7 @@ void ELFFileBase::init() {
template <class ELFT> void ELFFileBase::init(InputFile::Kind k) {
using Elf_Shdr = typename ELFT::Shdr;
using Elf_Sym = typename ELFT::Sym;
+ using Elf_Phdr = typename ELFT::Phdr;
// Initialize trivial attributes.
const ELFFile<ELFT> &obj = getObj<ELFT>();
@@ -513,6 +516,10 @@ template <class ELFT> void ELFFileBase::init(InputFile::Kind k) {
elfShdrs = sections.data();
numELFShdrs = sections.size();
+ ArrayRef<Elf_Phdr> pHeaders = CHECK2(obj.program_headers(), this);
+ elfPhdrs = pHeaders.data();
+ numElfPhdrs = pHeaders.size();
+
// Find a symbol table.
const Elf_Shdr *symtabSec =
findSection(sections, k == SharedKind ? SHT_DYNSYM : SHT_SYMTAB);
@@ -1418,6 +1425,21 @@ std::vector<uint32_t> SharedFile::parseVerneed(const ELFFile<ELFT> &obj,
return verneeds;
}
+// To determine if a shared file can support the AArch64 GCS extension, the program headers for the object
+// need to be read. This ensures when input options are read, appropriate warning/error messages can be
+// emitted depending on the user's command line options.
+template <typename ELFT>
+uint64_t SharedFile::parseGnuAttributes(const typename ELFT::PhdrRange headers) {
+ if(numElfPhdrs == 0)
+ return 0;
+ uint64_t attributes = 0;
+ for (unsigned i = 0; i < numElfPhdrs; i++)
+ if(headers[i].p_type == PT_GNU_PROPERTY && headers[i].p_flags & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
+ attributes |= GNU_PROPERTY_AARCH64_FEATURE_1_GCS;
+
+ return attributes;
+}
+
// 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
@@ -1528,6 +1550,7 @@ template <class ELFT> void SharedFile::parse() {
verdefs = parseVerdefs<ELFT>(obj.base(), verdefSec);
std::vector<uint32_t> verneeds = parseVerneed<ELFT>(obj, verneedSec);
+ this->andFeatures = parseGnuAttributes<ELFT>(getELFPhdrs<ELFT>());
// 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
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index 0b186db1ba0d1..99b1842326622 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -206,6 +206,10 @@ class ELFFileBase : public InputFile {
return typename ELFT::ShdrRange(
reinterpret_cast<const typename ELFT::Shdr *>(elfShdrs), numELFShdrs);
}
+ template <typename ELFT> typename ELFT::PhdrRange getELFPhdrs() const {
+ return typename ELFT::PhdrRange(
+ reinterpret_cast<const typename ELFT::Phdr *>(elfPhdrs), numElfPhdrs);
+ }
template <typename ELFT> typename ELFT::SymRange getELFSyms() const {
return typename ELFT::SymRange(
reinterpret_cast<const typename ELFT::Sym *>(elfSyms), numSymbols);
@@ -224,8 +228,10 @@ class ELFFileBase : public InputFile {
StringRef stringTable;
const void *elfShdrs = nullptr;
const void *elfSyms = nullptr;
+ const void *elfPhdrs = nullptr;
uint32_t numELFShdrs = 0;
uint32_t firstGlobal = 0;
+ uint32_t numElfPhdrs = 0;
// Below are ObjFile specific members.
@@ -364,6 +370,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>
+ uint64_t parseGnuAttributes(const typename ELFT::PhdrRange headers);
};
class BinaryFile : public InputFile {
diff --git a/lld/docs/ReleaseNotes.rst b/lld/docs/ReleaseNotes.rst
index 6f60efd87c975..a321b5234ee79 100644
--- a/lld/docs/ReleaseNotes.rst
+++ b/lld/docs/ReleaseNotes.rst
@@ -25,6 +25,11 @@ Non-comprehensive list of changes in this release
ELF Improvements
----------------
+* For AArch64, support for the -zgcs-report-dynamic option has been added. This will provide users with
+the ability to check any Dynamic Objects explicitly passed to LLD for the GNU GCS Attribute Flag. This is
+required for all files when linking with GCS enabled. Unless defined by the user, -zgcs-report-dynamic
+inherits its value from the -zgcs-report option, capped at the `warning` level to ensure that a users
+module can still compile. This behaviour is designed to match the GNU ld Linker.
Breaking changes
----------------
diff --git a/lld/test/ELF/aarch64-feature-gcs.s b/lld/test/ELF/aarch64-feature-gcs.s
index 7a08673dbb7e6..e90ead3be23ff 100644
--- a/lld/test/ELF/aarch64-feature-gcs.s
+++ b/lld/test/ELF/aarch64-feature-gcs.s
@@ -49,11 +49,37 @@
# 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 -o /dev/null -z gcs-report=warning -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 -o /dev/null -z gcs-report-dynamic=warning -z gcs-report=warning -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 -o /dev/null -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 -o /dev/null -z gcs-report=error -z gcs-report-dynamic=warning -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 -o /dev/null -z gcs-report-dynamic=warning -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 -o /dev/null -z gcs-report=error -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 -o /dev/null -z gcs-report-dynamic=error -z gcs-report=error -z gcs=always 2>&1 | FileCheck --check-prefix=REPORT-ERROR-DYNAMIC %s
+# RUN: not ld.lld func1-gcs.o func3-gcs.o no-gcs.so force-gcs.so -o /dev/null -z gcs-report-dynamic=error -z gcs=always 2>&1 | FileCheck --check-prefix=REPORT-ERROR-DYNAMIC %s
+
+# 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 dependancies have the GCS marking.
+# REPORT-WARN-DYNAMIC-NOT: warning: force-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 dependancies have the GCS marking.
+# 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 dependancies have the GCS marking.
+# REPORT-ERROR-DYNAMIC-NOT: error: force-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 dependancies have the GCS marking.
+
## 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
# INVALID: error: unknown -z gcs= value: nonsense
+## An invalid gcs option should give an error
+# RUN: not ld.lld func1-gcs.o func2-gcs.o func3-gcs.o -z gcs-report=nonsense 2>&1 | FileCheck --check-prefix=INVALID-GCS-REPORT %s
+
+# INVALID-GCS-REPORT: error: unknown -z gcs-report= value: nonsense
+
+## An invalid gcs-report-dynamic option should give an error
+# RUN: not ld.lld func1-gcs.o func2-gcs.o func3-gcs.o -z gcs-report-dynamic=nonsense 2>&1 | FileCheck --check-prefix=INVALID-GCS-REPORT-DYNAMIC %s
+
+# INVALID-GCS-REPORT-DYNAMIC: error: unknown -z gcs-report-dynamic= value: nonsense
+
#--- func1-gcs.s
.section ".note.gnu.property", "a"
.long 4
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
lld/docs/ReleaseNotes.rst
Outdated
| @@ -25,6 +25,11 @@ Non-comprehensive list of changes in this release | |||
|
|
|||
| ELF Improvements | |||
| ---------------- | |||
| * For AArch64, support for the -zgcs-report-dynamic option has been added. This will provide users with | |||
There was a problem hiding this comment.
``-zgcs-report-dynamic``
ditto below for -zgcs-report
lld/docs/ReleaseNotes.rst
Outdated
| @@ -25,6 +25,11 @@ Non-comprehensive list of changes in this release | |||
|
|
|||
| ELF Improvements | |||
| ---------------- | |||
| * For AArch64, support for the -zgcs-report-dynamic option has been added. This will provide users with | |||
| @@ -49,10 +49,28 @@ | |||
| # 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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done. Added 6 tests to cover all use cases where this new option could trigger a warning/error to ensure it does not.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Note: "Resolve conversion" is reserved for reviewers per recommendation on https://discourse.llvm.org/t/rfc-github-pr-resolve-conversation-button/73178
There was a problem hiding this comment.
Apologies, will make sure I follow that (sorry I had not seen this comment and have resolved some conversations this morning)
There was a problem hiding this comment.
I have reduced it to 6 total test cases, this should still cover alot of cases to ensure the option is working as intended.
Stylie777
force-pushed
the
gcs_lld_improvements
branch
from
b21cebc to
ccd8005
Compare
|
Thanks @smithp35! I will fix the code formatting issues after the next round of reviews too. EDIT: I have fixed the formatting so the review is as it will be on the main branch. |
lld/ELF/InputFiles.cpp
Outdated
| // If the file being parsed is a SharedFile, we cannot pass in | ||
| // the data variable as there is no InputSection to collect the | ||
| // data from. As such, these are ignored. They are needed either | ||
| // when loading a shared library oject. |
There was a problem hiding this comment.
IIUC the data->data() is the full contents of the notes section. Whereas desc->data() is the contents of just one desc. Instead of skipping both the errors you could do something like
ArrayRef<uint8_t> contents = data ? *data : desc;
Then use contents.data() below.
There was a problem hiding this comment.
Done. Thanks for the suggestion.
There was a problem hiding this comment.
The comment is a bit too verbose regarding the lld code convention.
Actually, I am not sure shared objects should parse GNU_PROPERTY_AARCH64_FEATURE_PAUTH. It's orthogonal to this PR and this PR should not introduce the behavior change
There was a problem hiding this comment.
This comment is now actually irrelevant so I have removed it.
There was a problem hiding this comment.
LGTM on my side. Please leave some time for @MaskRay to comment.
For reference, the use of p_vaddr in Bionic's dynamic loader makes sense. At that point Bionic has already mmapped the ELF file into memory at p_vaddr (+ some displacement as shared libraries are position independent) so it is operating on the in-memory representation of the ELF file. LLD is reading directly from the file.
|
Thanks @smithp35. And that makes sense as to why we need to use |
MaskRay
force-pushed
the
gcs_lld_improvements
branch
from
9d52298 to
bfc23cb
Compare
lld/ELF/InputFiles.cpp
Outdated
| return; | ||
| uint32_t featureAndType = ctx.arg.emachine == EM_AARCH64 | ||
| ? GNU_PROPERTY_AARCH64_FEATURE_1_AND | ||
| : GNU_PROPERTY_X86_FEATURE_1_AND; |
There was a problem hiding this comment.
This PR should not change the behavior of x86. DSOs are ignored for GNU_PROPERTY_X86_FEATURE_1_AND and I think we should continue ignoring it.
There was a problem hiding this comment.
I have made it so the SharedFile will only be parsed if its an EM_AARCH64 machine time, and the featureAndType value is passed directly to the parseGnuPropertyNote function.
lld/docs/ReleaseNotes.rst
Outdated
| @@ -25,6 +25,11 @@ Non-comprehensive list of changes in this release | |||
|
|
|||
| ELF Improvements | |||
| ---------------- | |||
| * For AArch64, support for the ``-zgcs-report-dynamic`` option has been added. This will provide users with | |||
There was a problem hiding this comment.
This is too verbose. Consider
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.
MaskRay
force-pushed
the
gcs_lld_improvements
branch
2 times, most recently
from
34668ed to
70ab9ec
Compare
| @@ -574,6 +576,41 @@ static GcsPolicy getZGcs(Ctx &ctx, opt::InputArgList &args) { | |||
| return ret; | |||
| } | |||
|
|
|||
| static void getZGcsReport(Ctx &ctx, opt::InputArgList &args) { | |||
There was a problem hiding this comment.
Created #130715 to introduce an enum class for none/warning/error.
There was a problem hiding this comment.
Once this is merged, I will update this so we can utilise that enum class
There was a problem hiding this comment.
I have updated the function to use the new enum class.
| @@ -1629,7 +1667,6 @@ static void readConfigs(Ctx &ctx, opt::InputArgList &args) { | |||
| std::make_pair("bti-report", &ctx.arg.zBtiReport), | |||
| 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("pauth-report", &ctx.arg.zPauthReport)}; | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
lld/ELF/Driver.cpp
Outdated
| @@ -2788,6 +2825,17 @@ static void redirectSymbols(Ctx &ctx, ArrayRef<WrappedSymbol> wrapped) { | |||
| ctx.symtab->wrap(w.sym, w.real, w.wrap); | |||
| } | |||
|
|
|||
| static StringRef gcsReportPolicytoString(GcsReportPolicy value) { | |||
To enable easier processing of inheritance for the -zgcs-report-dynamic option, the -zgcs-report option has been converted to use an Enum value to define its setting rather than a StringRef value. This adds the Enum class for defining the three options, None, Warning and Error, along with the appropirate methods for processing the users input. To enable the error messages, a new lambda function has been added. This processes the Enum value and passes the appropriate string into the `report` lambda function for the error message. Error messages have been updated where required.
When GCS was introduced to LLD, the gcs-report option allowed for a user to gain information relating to if their relocatable objects supported the feature. For an executable or shared-library to support GCS, all relocatable objects must declare that they support GCS. The gcs-report checks were only done on relocatable object files, however for a program to enable GCS, the executable and all shared libraries that it loads must enable GCS. gcs-report-dynamic enables checks to be performed on all shared objects loaded by LLD, and in cases where GCS is not supported, a warning or error will be emitted. It should be noted that only shared files directly passed to LLD are checked for GCS support. Files that are noted in the `DT_NEEDED` tags are assumed to have had their GCS support checked when they were created. The behaviour of the -zgcs-dynamic-report option matches that of GNU ld. The behaviour is as follows unless the user explicitly sets the value: * -zgcs-report=warning or -zgcs-report=error implies -zgcs-report-dynamic=warning. This approach avoids inheriting an error level if the user wishes to continue building a module without rebuilding all the shared libraries. The same approach was taken for the GNU ld linker, so behaviour is identical across the toolchains. This implementation matches the error message and command line interface used within the GNU ld Linker. See here: https://inbox.sourceware.org/binutils/[email protected]/ To enable the checking of the GNU GCS Attributes when linking together an application, LLD needs to be able to parse a Shared Files program headers. This will enable a new option, allowing the user to output diagnostic information relating to if there are shared files being used that do not support the GCS extension. To define if a shared file support GCS, the GNU GCS Attribute will be stored within the Program Header, so this is parsed and, if found, the `andFeatures` value updated to reflect the support level.
This is no longer needed now this PR is using the new `ReportPolicy` enum.
Stylie777
force-pushed
the
gcs_lld_improvements
branch
from
70ab9ec to
bdea2f7
Compare
| // Read a body of a NOTE record, which consists of type-length-value fields. | ||
| ArrayRef<uint8_t> desc = note.getDesc(headers[i].p_align); | ||
| parseGnuPropertyNote<ELFT>( | ||
| ctx, *this, /*featureAndType*/ GNU_PROPERTY_AARCH64_FEATURE_1_AND, desc, |
There was a problem hiding this comment.
remove /*featureAndType*/. it's clear from the argument value (GNU_PROPERTY...)
If we need to name the argument, the canonical form is /*xxx=*/value (no space)
When GCS was introduced to LLD, the gcs-report option allowed for a user to gain information relating to if their relocatable objects supported the feature. For an executable or shared-library to support GCS, all relocatable objects must declare that they support GCS.
The gcs-report checks were only done on relocatable object files, however for a program to enable GCS, the executable and all shared libraries that it loads must enable GCS. gcs-report-dynamic enables checks to be performed on all shared objects loaded by LLD, and in cases where GCS is not supported, a warning or error will be emitted.
It should be noted that only shared files directly passed to LLD are checked for GCS support. Files that are noted in the
DT_NEEDEDtags are assumed to have had their GCS support checked when they were created.The behaviour of the -zgcs-dynamic-report option matches that of GNU ld. The behaviour is as follows unless the user explicitly sets the value:
-zgcs-report-dynamic=warning.
This approach avoids inheriting an error level if the user wishes to continue building a module without rebuilding all the shared libraries. The same approach was taken for the GNU ld linker, so behaviour is identical across the toolchains.
This implementation matches the error message and command line interface used within the GNU ld Linker. See here:
bminor/binutils-gdb@724a834
To support this option being introduced, two other changes are included as part of this PR. The first converts the -zgcs-report option to utilise an Enum, opposed to StringRef values. This enables easier tracking of the value the user defines when inheriting the value for the gas-report-dynamic option. The second is to parse the Dynamic Objects program headers to locate the GNU Attribute flag that shows GCS is supported. This is needed so, when using the gcs-report-dynamic option, LLD can correctly determine if a dynamic object supports GCS.