-
Notifications
You must be signed in to change notification settings - Fork 13k
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.
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
[AArch64][GCS][LLD] Introduce -zgcs-report-dynamic Command Line Option #127787
base: main
Are you sure you want to change the base?
Changes from all commits
84b665c
79fd58d
b37c82e
5660484
bdea2f7
c2af177
64e6544
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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"; | ||
} | ||
|
@@ -574,6 +576,41 @@ static GcsPolicy getZGcs(Ctx &ctx, opt::InputArgList &args) { | |
return ret; | ||
} | ||
|
||
static void getZGcsReport(Ctx &ctx, opt::InputArgList &args) { | ||
bool reportDynamicDefined = false; | ||
for (auto *arg : args.filtered(OPT_z)) { | ||
std::pair<StringRef, StringRef> kv = StringRef(arg->getValue()).split('='); | ||
if (kv.first != "gcs-report" && kv.first != "gcs-report-dynamic") | ||
continue; | ||
arg->claim(); | ||
ReportPolicy value; | ||
if (kv.second == "none") | ||
value = ReportPolicy::None; | ||
else if (kv.second == "warning") | ||
value = ReportPolicy::Warning; | ||
else if (kv.second == "error") | ||
value = ReportPolicy::Error; | ||
else { | ||
ErrAlways(ctx) << "unknown -z " << kv.first << "= value: " << kv.second; | ||
continue; | ||
} | ||
if (kv.first == "gcs-report") { | ||
ctx.arg.zGcsReport = value; | ||
} else if (kv.first == "gcs-report-dynamic") { | ||
ctx.arg.zGcsReportDynamic = value; | ||
reportDynamicDefined = true; | ||
} | ||
} | ||
|
||
// When -zgcs-report is set to `warning` or `error`, -zgcs-report-dynamic will | ||
// inherit this value if unspecified, matching GNU ld. This detects shared | ||
// libraries without the GCS property but does not the shared-libraries to be | ||
// rebuilt for successful linking | ||
if (!reportDynamicDefined && ctx.arg.zGcsReport != ReportPolicy::None && | ||
ctx.arg.zGcsReportDynamic == ReportPolicy::None) | ||
ctx.arg.zGcsReportDynamic = ReportPolicy::Warning; | ||
} | ||
|
||
// 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 +1585,7 @@ 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); | ||
getZGcsReport(ctx, args); | ||
ctx.arg.zGlobal = hasZOption(args, "global"); | ||
ctx.arg.zGnustack = getZGnuStack(args); | ||
ctx.arg.zHazardplt = hasZOption(args, "hazardplt"); | ||
|
@@ -1624,7 +1662,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. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 If it makes it clearer for the user, I can add a comment explaining that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
then after the for loop,
|
||
for (opt::Arg *arg : args.filtered(OPT_z)) { | ||
std::pair<StringRef, StringRef> option = | ||
|
@@ -2907,6 +2944,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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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)); | ||
|
@@ -1418,6 +1434,36 @@ std::vector<uint32_t> SharedFile::parseVerneed(const ELFFile<ELFT> &obj, | |
return verneeds; | ||
} | ||
|
||
// To determine if a shared file can support any of the GNU Attributes, | ||
// the .note.gnu.properties section need to be read. The appropriate | ||
// location in memory is located then the GnuPropertyNote can be parsed. | ||
// This is the same process as is used for readGnuProperty, however we | ||
// do not pass the data variable as, without an InputSection, its value | ||
// is unknown in a SharedFile. This is ok as the information that would | ||
// be collected from this is irrelevant for a dynamic object. | ||
template <typename ELFT> | ||
void SharedFile::parseGnuAndFeatures(const uint8_t *base, | ||
const typename ELFT::PhdrRange headers) { | ||
if (headers.size() == 0 || ctx.arg.emachine != EM_AARCH64) | ||
return; | ||
|
||
for (unsigned i = 0; i < headers.size(); i++) { | ||
if (headers[i].p_type != PT_GNU_PROPERTY) | ||
continue; | ||
const typename ELFT::Note note( | ||
*reinterpret_cast<const typename ELFT::Nhdr *>(base + | ||
headers[i].p_offset)); | ||
if (note.getType() != NT_GNU_PROPERTY_TYPE_0 || note.getName() != "GNU") | ||
continue; | ||
|
||
// Read a body of a NOTE record, which consists of type-length-value fields. | ||
Stylie777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove If we need to name the argument, the canonical form is |
||
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 | ||
|
@@ -1454,10 +1500,12 @@ template <class ELFT> void SharedFile::parse() { | |
using Elf_Sym = typename ELFT::Sym; | ||
using Elf_Verdef = typename ELFT::Verdef; | ||
using Elf_Versym = typename ELFT::Versym; | ||
using Elf_Phdr = typename ELFT::Phdr; | ||
|
||
ArrayRef<Elf_Dyn> dynamicTags; | ||
const ELFFile<ELFT> obj = this->getObj<ELFT>(); | ||
ArrayRef<Elf_Shdr> sections = getELFShdrs<ELFT>(); | ||
ArrayRef<Elf_Phdr> pHeaders = CHECK2(obj.program_headers(), this); | ||
|
||
const Elf_Shdr *versymSec = nullptr; | ||
const Elf_Shdr *verdefSec = nullptr; | ||
|
@@ -1528,6 +1576,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.base(), pHeaders); | ||
|
||
// 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
Stylie777 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## 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. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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 dependencies 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 dependencies 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 dependencies 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 | ||
# 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" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #130715 to introduce an
enum class
for none/warning/error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once this is merged, I will update this so we can utilise that
enum class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the function to use the new
enum class
.