Skip to content

Commit ccd8005

Browse files
Stylie777Stylie777
committed
Updates after second round of review comments
Summary of changes: * Storing of Phdrs is now done within the SharedFile class as they are not present within a `.o` file, so should only be processed for `.so` files. * Added comment in `getZGcsReport` to explain the inheritance logic that matches GNU ld. * Added comments in the function calls for `getZGcsReport` with the variable name for the boolean value * Updated error message with dependent not depedant * Refactored the parsing of a GNU Property Note. Common logic between `readGnuProperty` and `SharedFiles::parseGnuAndFeatures` have been refactored into their own static function to redude maintainability. * Expanded testing after some feedback from reviewers
1 parent 5f8e25e commit ccd8005

File tree

4 files changed

+94
-94
lines changed

4 files changed

+94
-94
lines changed

‎lld/ELF/Driver.cpp

+7-3
Original file line numberDiff line numberDiff line change
@@ -596,6 +596,10 @@ getZGcsReport(Ctx &ctx, opt::InputArgList &args, bool isReportDynamic,
596596
}
597597
}
598598

599+
// The behaviour of -zgnu-report-dynamic matches that of GNU ld. When -zgcs-report
600+
// is set to `warning` or `error`, -zgcs-report-dynamic will inherit this value if
601+
// there is no user set value. This detects shared libraries without the GCS
602+
// property but does not the shared-libraries to be rebuilt for successful linking
599603
if (isReportDynamic && gcsReportValue.getValue() != GcsReportPolicy::None &&
600604
ret.getValue() == GcsReportPolicy::None)
601605
ret = GcsReportPolicy::Warning;
@@ -1582,9 +1586,9 @@ static void readConfigs(Ctx &ctx, opt::InputArgList &args) {
15821586
ctx.arg.zForceBti = hasZOption(args, "force-bti");
15831587
ctx.arg.zForceIbt = hasZOption(args, "force-ibt");
15841588
ctx.arg.zGcs = getZGcs(ctx, args);
1585-
ctx.arg.zGcsReport = getZGcsReport(ctx, args, false);
1589+
ctx.arg.zGcsReport = getZGcsReport(ctx, args, /* isReportDynamic */ false);
15861590
ctx.arg.zGcsReportDynamic =
1587-
getZGcsReport(ctx, args, true, ctx.arg.zGcsReport);
1591+
getZGcsReport(ctx, args, /* isReportDynamic */ true, ctx.arg.zGcsReport);
15881592
ctx.arg.zGlobal = hasZOption(args, "global");
15891593
ctx.arg.zGnustack = getZGnuStack(args);
15901594
ctx.arg.zHazardplt = hasZOption(args, "hazardplt");
@@ -2956,7 +2960,7 @@ static void readSecurityNotes(Ctx &ctx) {
29562960
"necessary property note. The "
29572961
<< "dynamic loader might not enable GCS or refuse to load the "
29582962
"program unless all shared library "
2959-
<< "dependancies have the GCS marking.";
2963+
<< "dependencies have the GCS marking.";
29602964
}
29612965

29622966
static void initSectionsAndLocalSyms(ELFFileBase *file, bool ignoreComdats) {

‎lld/ELF/InputFiles.cpp

+66-79
Original file line numberDiff line numberDiff line change
@@ -502,7 +502,6 @@ void ELFFileBase::init() {
502502
template <class ELFT> void ELFFileBase::init(InputFile::Kind k) {
503503
using Elf_Shdr = typename ELFT::Shdr;
504504
using Elf_Sym = typename ELFT::Sym;
505-
using Elf_Phdr = typename ELFT::Phdr;
506505

507506
// Initialize trivial attributes.
508507
const ELFFile<ELFT> &obj = getObj<ELFT>();
@@ -514,10 +513,6 @@ template <class ELFT> void ELFFileBase::init(InputFile::Kind k) {
514513
elfShdrs = sections.data();
515514
numELFShdrs = sections.size();
516515

517-
ArrayRef<Elf_Phdr> pHeaders = CHECK2(obj.program_headers(), this);
518-
elfPhdrs = pHeaders.data();
519-
numElfPhdrs = pHeaders.size();
520-
521516
// Find a symbol table.
522517
const Elf_Shdr *symtabSec =
523518
findSection(sections, k == SharedKind ? SHT_DYNSYM : SHT_SYMTAB);
@@ -923,6 +918,56 @@ void ObjFile<ELFT>::initializeSections(bool ignoreComdats,
923918
handleSectionGroup<ELFT>(this->sections, entries);
924919
}
925920

921+
template <typename ELFT>
922+
static void parseGnuPropertyNote(Ctx &ctx, uint32_t &featureAndType, ArrayRef<uint8_t> &desc, ELFFileBase *f, const uint8_t *base, ArrayRef<uint8_t> *data = nullptr) {
923+
auto err = [&](const uint8_t *place) -> ELFSyncStream {
924+
auto diag = Err(ctx);
925+
diag << f->getName() << ":(" << ".note.gnu.properties" << "+0x"
926+
<< Twine::utohexstr(place - base) << "): ";
927+
return diag;
928+
};
929+
930+
while (!desc.empty()) {
931+
const uint8_t *place = desc.data();
932+
if (desc.size() < 8)
933+
return void(err(place) << "program property is too short");
934+
uint32_t type = read32<ELFT::Endianness>(desc.data());
935+
uint32_t size = read32<ELFT::Endianness>(desc.data() + 4);
936+
desc = desc.slice(8);
937+
if (desc.size() < size)
938+
return void(err(place) << "program property is too short");
939+
940+
if (type == featureAndType) {
941+
// We found a FEATURE_1_AND field. There may be more than one of these
942+
// in a .note.gnu.property section, for a relocatable object we
943+
// accumulate the bits set.
944+
if (size < 4)
945+
return void(err(place) << "FEATURE_1_AND entry is too short");
946+
f->andFeatures |= read32<ELFT::Endianness>(desc.data());
947+
} else if (ctx.arg.emachine == EM_AARCH64 &&
948+
type == GNU_PROPERTY_AARCH64_FEATURE_PAUTH) {
949+
// If the file being parsed is a SharedFile, we cannot pass in
950+
// the data variable as there is no InputSection to collect the
951+
// data from. As such, these are ignored. They are needed either
952+
// when loading a shared library oject.
953+
if (!f->aarch64PauthAbiCoreInfo.empty() && data != nullptr) {
954+
return void(
955+
err(data->data())
956+
<< "multiple GNU_PROPERTY_AARCH64_FEATURE_PAUTH entries are "
957+
"not supported");
958+
} else if (size != 16 && data != nullptr) {
959+
return void(err(data->data())
960+
<< "GNU_PROPERTY_AARCH64_FEATURE_PAUTH entry "
961+
"is invalid: expected 16 bytes, but got "
962+
<< size);
963+
}
964+
f->aarch64PauthAbiCoreInfo = desc;
965+
}
966+
967+
// Padding is present in the note descriptor, if necessary.
968+
desc = desc.slice(alignTo<(ELFT::Is64Bits ? 8 : 4)>(size));
969+
}
970+
}
926971
// Read the following info from the .note.gnu.property section and write it to
927972
// the corresponding fields in `ObjFile`:
928973
// - Feature flags (32 bits) representing x86 or AArch64 features for
@@ -960,42 +1005,8 @@ static void readGnuProperty(Ctx &ctx, const InputSection &sec,
9601005

9611006
// Read a body of a NOTE record, which consists of type-length-value fields.
9621007
ArrayRef<uint8_t> desc = note.getDesc(sec.addralign);
963-
while (!desc.empty()) {
964-
const uint8_t *place = desc.data();
965-
if (desc.size() < 8)
966-
return void(err(place) << "program property is too short");
967-
uint32_t type = read32<ELFT::Endianness>(desc.data());
968-
uint32_t size = read32<ELFT::Endianness>(desc.data() + 4);
969-
desc = desc.slice(8);
970-
if (desc.size() < size)
971-
return void(err(place) << "program property is too short");
972-
973-
if (type == featureAndType) {
974-
// We found a FEATURE_1_AND field. There may be more than one of these
975-
// in a .note.gnu.property section, for a relocatable object we
976-
// accumulate the bits set.
977-
if (size < 4)
978-
return void(err(place) << "FEATURE_1_AND entry is too short");
979-
f.andFeatures |= read32<ELFT::Endianness>(desc.data());
980-
} else if (ctx.arg.emachine == EM_AARCH64 &&
981-
type == GNU_PROPERTY_AARCH64_FEATURE_PAUTH) {
982-
if (!f.aarch64PauthAbiCoreInfo.empty()) {
983-
return void(
984-
err(data.data())
985-
<< "multiple GNU_PROPERTY_AARCH64_FEATURE_PAUTH entries are "
986-
"not supported");
987-
} else if (size != 16) {
988-
return void(err(data.data())
989-
<< "GNU_PROPERTY_AARCH64_FEATURE_PAUTH entry "
990-
"is invalid: expected 16 bytes, but got "
991-
<< size);
992-
}
993-
f.aarch64PauthAbiCoreInfo = desc;
994-
}
995-
996-
// Padding is present in the note descriptor, if necessary.
997-
desc = desc.slice(alignTo<(ELFT::Is64Bits ? 8 : 4)>(size));
998-
}
1008+
const uint8_t *base = f.getObj().base();
1009+
parseGnuPropertyNote<ELFT>(ctx, featureAndType, desc, &f, base, &data);
9991010

10001011
// Go to next NOTE record to look for more FEATURE_1_AND descriptions.
10011012
data = data.slice(nhdr->getSize(sec.addralign));
@@ -1424,23 +1435,16 @@ std::vector<uint32_t> SharedFile::parseVerneed(const ELFFile<ELFT> &obj,
14241435
}
14251436

14261437
// To determine if a shared file can support any of the GNU Attributes,
1427-
// the .note.gnu.properties section need to be read. This has to be done
1428-
// differently for SharedFiles as the information available is not as
1429-
// extensive as a normal object input file. This will take the program
1430-
// headers, along with the SHT_NOTE section header to find the relevant
1431-
// information. This uses a similar process to the readGnuProperty
1432-
// function, but designed specifically for SharedFiles.
1438+
// the .note.gnu.properties section need to be read. The appropriate
1439+
// location in memory is located then the GnuPropertyNote can be parsed.
1440+
// This is the same process as is used for readGnuProperty, however we
1441+
// do not pass the data variable as, without an InputSection, its value
1442+
// is unknown in a SharedFile. This is ok as the information that would
1443+
// be collected from this is irrelevant for a dynamic object.
14331444
template <typename ELFT>
1434-
void SharedFile::parseGnuAttributes(const uint8_t *base,
1445+
void SharedFile::parseGnuAndFeatures(const uint8_t *base,
14351446
const typename ELFT::PhdrRange headers,
14361447
const typename ELFT::Shdr *sHeader) {
1437-
auto err = [&](const uint8_t *place) -> ELFSyncStream {
1438-
auto diag = Err(ctx);
1439-
diag << this->getName() << ":(" << ".note.gnu.properties" << "+0x"
1440-
<< Twine::utohexstr(place - base) << "): ";
1441-
return diag;
1442-
};
1443-
14441448
if (numElfPhdrs == 0 || sHeader == nullptr)
14451449
return;
14461450
uint32_t featureAndType = ctx.arg.emachine == EM_AARCH64
@@ -1450,7 +1454,6 @@ void SharedFile::parseGnuAttributes(const uint8_t *base,
14501454
for (unsigned i = 0; i < numElfPhdrs; i++) {
14511455
if (headers[i].p_type != PT_GNU_PROPERTY)
14521456
continue;
1453-
14541457
const typename ELFT::Note note(
14551458
*reinterpret_cast<const typename ELFT::Nhdr *>(base +
14561459
headers[i].p_vaddr));
@@ -1459,28 +1462,7 @@ void SharedFile::parseGnuAttributes(const uint8_t *base,
14591462

14601463
// Read a body of a NOTE record, which consists of type-length-value fields.
14611464
ArrayRef<uint8_t> desc = note.getDesc(sHeader->sh_addralign);
1462-
while (!desc.empty()) {
1463-
const uint8_t *place = desc.data();
1464-
if (desc.size() < 8)
1465-
return void(err(place) << "program property is too short");
1466-
uint32_t type = read32(ctx, desc.data());
1467-
uint32_t size = read32(ctx, desc.data() + 4);
1468-
desc = desc.slice(8);
1469-
if (desc.size() < size)
1470-
return void(err(place) << "program property is too short");
1471-
1472-
// We found a FEATURE_1_AND field. There may be more than one of these
1473-
// in a .note.gnu.property section, for a relocatable object we
1474-
// accumulate the bits set.
1475-
if (type == featureAndType) {
1476-
if (size < 4)
1477-
return void(err(place) << "FEATURE_1_AND entry is too short");
1478-
this->andFeatures |= read32(ctx, desc.data());
1479-
}
1480-
1481-
// Padding is present in the note descriptor, if necessary.
1482-
desc = desc.slice(alignTo<(ELFT::Is64Bits ? 8 : 4)>(size));
1483-
}
1465+
parseGnuPropertyNote<ELFT>(ctx, featureAndType, desc, this, base);
14841466
}
14851467
}
14861468

@@ -1520,11 +1502,16 @@ template <class ELFT> void SharedFile::parse() {
15201502
using Elf_Sym = typename ELFT::Sym;
15211503
using Elf_Verdef = typename ELFT::Verdef;
15221504
using Elf_Versym = typename ELFT::Versym;
1505+
using Elf_Phdr = typename ELFT::Phdr;
15231506

15241507
ArrayRef<Elf_Dyn> dynamicTags;
15251508
const ELFFile<ELFT> obj = this->getObj<ELFT>();
15261509
ArrayRef<Elf_Shdr> sections = getELFShdrs<ELFT>();
15271510

1511+
ArrayRef<Elf_Phdr> pHeaders = CHECK2(obj.program_headers(), this);
1512+
elfPhdrs = pHeaders.data();
1513+
numElfPhdrs = pHeaders.size();
1514+
15281515
const Elf_Shdr *versymSec = nullptr;
15291516
const Elf_Shdr *verdefSec = nullptr;
15301517
const Elf_Shdr *verneedSec = nullptr;
@@ -1598,7 +1585,7 @@ template <class ELFT> void SharedFile::parse() {
15981585

15991586
verdefs = parseVerdefs<ELFT>(obj.base(), verdefSec);
16001587
std::vector<uint32_t> verneeds = parseVerneed<ELFT>(obj, verneedSec);
1601-
parseGnuAttributes<ELFT>(obj.base(), getELFPhdrs<ELFT>(), noteSec);
1588+
parseGnuAndFeatures<ELFT>(obj.base(), getELFPhdrs<ELFT>(), noteSec);
16021589

16031590
// Parse ".gnu.version" section which is a parallel array for the symbol
16041591
// table. If a given file doesn't have a ".gnu.version" section, we use

‎lld/ELF/InputFiles.h

+10-7
Original file line numberDiff line numberDiff line change
@@ -206,10 +206,6 @@ class ELFFileBase : public InputFile {
206206
return typename ELFT::ShdrRange(
207207
reinterpret_cast<const typename ELFT::Shdr *>(elfShdrs), numELFShdrs);
208208
}
209-
template <typename ELFT> typename ELFT::PhdrRange getELFPhdrs() const {
210-
return typename ELFT::PhdrRange(
211-
reinterpret_cast<const typename ELFT::Phdr *>(elfPhdrs), numElfPhdrs);
212-
}
213209
template <typename ELFT> typename ELFT::SymRange getELFSyms() const {
214210
return typename ELFT::SymRange(
215211
reinterpret_cast<const typename ELFT::Sym *>(elfSyms), numSymbols);
@@ -228,10 +224,8 @@ class ELFFileBase : public InputFile {
228224
StringRef stringTable;
229225
const void *elfShdrs = nullptr;
230226
const void *elfSyms = nullptr;
231-
const void *elfPhdrs = nullptr;
232227
uint32_t numELFShdrs = 0;
233228
uint32_t firstGlobal = 0;
234-
uint32_t numElfPhdrs = 0;
235229

236230
// Below are ObjFile specific members.
237231

@@ -366,12 +360,21 @@ class SharedFile : public ELFFileBase {
366360
// parsed. Only filled for `--no-allow-shlib-undefined`.
367361
SmallVector<Symbol *, 0> requiredSymbols;
368362

363+
template <typename ELFT> typename ELFT::PhdrRange getELFPhdrs() const {
364+
return typename ELFT::PhdrRange(
365+
reinterpret_cast<const typename ELFT::Phdr *>(elfPhdrs), numElfPhdrs);
366+
}
367+
368+
protected:
369+
const void *elfPhdrs = nullptr;
370+
uint32_t numElfPhdrs = 0;
371+
369372
private:
370373
template <typename ELFT>
371374
std::vector<uint32_t> parseVerneed(const llvm::object::ELFFile<ELFT> &obj,
372375
const typename ELFT::Shdr *sec);
373376
template <typename ELFT>
374-
void parseGnuAttributes(const uint8_t *base,
377+
void parseGnuAndFeatures(const uint8_t *base,
375378
const typename ELFT::PhdrRange headers,
376379
const typename ELFT::Shdr *sHeader);
377380
};

‎lld/test/ELF/aarch64-feature-gcs.s

+11-5
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,17 @@
5959
# RUN: ld.lld func1-gcs.o func3-gcs.o no-gcs.so force-gcs.so -z gcs-report=error -z gcs-report-dynamic=warning -z gcs=always 2>&1 | FileCheck --check-prefix=REPORT-WARN-DYNAMIC %s
6060
# RUN: not ld.lld func1-gcs.o func3-gcs.o no-gcs.so force-gcs.so -z gcs-report-dynamic=error -z gcs-report=error -z gcs=always 2>&1 | FileCheck --check-prefix=REPORT-ERROR-DYNAMIC %s
6161
# 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
62-
63-
# 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.
64-
# 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.
65-
# 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.
66-
# 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.
62+
# RUN: ld.lld func1-gcs.o func3-gcs.o force-gcs.so -z gcs-report-dynamic=error -z gcs-report=error -z gcs=always 2>&1 | count 0
63+
# RUN: ld.lld func1-gcs.o func3-gcs.o force-gcs.so -z gcs-report-dynamic=warning -z gcs-report=error -z gcs=always 2>&1 | count 0
64+
# RUN: ld.lld func1-gcs.o func3-gcs.o force-gcs.so -z gcs-report=warning -z gcs=always 2>&1 | count 0
65+
# RUN: ld.lld func1-gcs.o func3-gcs.o force-gcs.so -z gcs-report=error -z gcs=always 2>&1 | count 0
66+
# RUN: ld.lld func1-gcs.o func3-gcs.o force-gcs.so -z gcs-report-dynamic=warning -z gcs=always 2>&1 | count 0
67+
# RUN: ld.lld func1-gcs.o func3-gcs.o force-gcs.so -z gcs-report-dynamic=error -z gcs=always 2>&1 | count 0
68+
69+
# 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.
70+
# 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.
71+
# 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.
72+
# 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.
6773

6874
## An invalid gcs option should give an error
6975
# 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

0 commit comments

Comments
 (0)