Responding to Review Comments

Stylie777 · Stylie777 · commit 9d522983d546 · 2025-03-10T11:38:08.000Z
Summary of changes:
* Release Notes syntax fix
* GcsReportPolicy has been reverted to an enum, with a function
available for converting the value from an enum to string
* getZGcsReport can now process both `-zgces-report` and
`-zgcs-report-dynamic` at the same time, applying the values where
the user has defined them, or the GNU Inheritance rules where
appropriate
* `elfPhdrs`, `numElfPhdrs` and `getElfPhdrs()` all removed as
the information can be easily access from other locations.
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
@@ -137,24 +137,7 @@ enum LtoKind : uint8_t {UnifiedThin, UnifiedRegular, Default};
 enum class GcsPolicy { Implicit, Never, Always };
 
 // For -z gcs-report= and -zgcs-report-dynamic
-struct GcsReportPolicy {
-  enum Options { None, Warning, Error, Unknown } value;
-  GcsReportPolicy(GcsReportPolicy::Options valueInput) : value(valueInput) {};
-
-  StringRef toString() {
-    StringRef ret;
-    if (value == Warning)
-      ret = "warning";
-    else if (value == Error)
-      ret = "error";
-    else
-      ret = "none";
-
-    return ret;
-  }
-
-  GcsReportPolicy::Options getValue() { return value; }
-};
+enum class GcsReportPolicy { None, Warning, Error, Unknown };
 
 struct SymbolVersion {
   llvm::StringRef name;
@@ -767,6 +750,18 @@ uint64_t errCount(Ctx &ctx);
 
 ELFSyncStream InternalErr(Ctx &ctx, const uint8_t *buf);
 
+inline StringRef gcsReportPolicytoString(GcsReportPolicy value) {
+  StringRef ret;
+  if (value == GcsReportPolicy::Warning)
+    ret = "warning";
+  else if (value == GcsReportPolicy::Error)
+    ret = "error";
+  else
+    ret = "none";
+
+  return ret;
+}
+
 #define CHECK2(E, S) lld::check2((E), [&] { return toStr(ctx, S); })
 
 } // namespace lld::elf
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
@@ -400,9 +400,9 @@ 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.getValue() != GcsReportPolicy::None)
+    if (ctx.arg.zGcsReport != GcsReportPolicy::None)
       ErrAlways(ctx) << "-z gcs-report only supported on AArch64";
-    if (ctx.arg.zGcsReportDynamic.getValue() != GcsReportPolicy::None)
+    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";
@@ -576,23 +576,27 @@ static GcsPolicy getZGcs(Ctx &ctx, opt::InputArgList &args) {
   return ret;
 }
 
-static GcsReportPolicy
-getZGcsReport(Ctx &ctx, opt::InputArgList &args, bool isReportDynamic,
-              GcsReportPolicy gcsReportValue = GcsReportPolicy::None) {
-  GcsReportPolicy ret = GcsReportPolicy::None;
+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 ((!isReportDynamic && kv.first == "gcs-report") ||
-        (isReportDynamic && kv.first == "gcs-report-dynamic")) {
+    if ((kv.first == "gcs-report") || kv.first == "gcs-report-dynamic") {
       arg->claim();
-      ret = StringSwitch<GcsReportPolicy>(kv.second)
-                .Case("none", GcsReportPolicy::None)
-                .Case("warning", GcsReportPolicy::Warning)
-                .Case("error", GcsReportPolicy::Error)
-                .Default(GcsReportPolicy::Unknown);
-      if (ret.getValue() == GcsReportPolicy::Unknown)
+      GcsReportPolicy value = StringSwitch<GcsReportPolicy>(kv.second)
+                                  .Case("none", GcsReportPolicy::None)
+                                  .Case("warning", GcsReportPolicy::Warning)
+                                  .Case("error", GcsReportPolicy::Error)
+                                  .Default(GcsReportPolicy::Unknown);
+      if (value == GcsReportPolicy::Unknown)
         ErrAlways(ctx) << "unknown -z " << kv.first << "= value: " << kv.second;
+
+      if (kv.first == "gcs-report")
+        ctx.arg.zGcsReport = value;
+      else if (kv.first == "gcs-report-dynamic") {
+        ctx.arg.zGcsReportDynamic = value;
+        reportDynamicDefined = true;
+      }
     }
   }
 
@@ -601,11 +605,9 @@ getZGcsReport(Ctx &ctx, opt::InputArgList &args, bool isReportDynamic,
   // inherit this value if there is no user set value. This detects shared
   // libraries without the GCS property but does not the shared-libraries to be
   // rebuilt for successful linking
-  if (isReportDynamic && gcsReportValue.getValue() != GcsReportPolicy::None &&
-      ret.getValue() == GcsReportPolicy::None)
-    ret = GcsReportPolicy::Warning;
-
-  return ret;
+  if (!reportDynamicDefined && ctx.arg.zGcsReport != GcsReportPolicy::None &&
+      ctx.arg.zGcsReportDynamic == GcsReportPolicy::None)
+    ctx.arg.zGcsReportDynamic = GcsReportPolicy::Warning;
 }
 
 // Report a warning for an unknown -z option.
@@ -1587,9 +1589,10 @@ 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, /* isReportDynamic */ false);
-  ctx.arg.zGcsReportDynamic =
-      getZGcsReport(ctx, args, /* isReportDynamic */ true, ctx.arg.zGcsReport);
+  // getZGcsReport assings the values for `ctx.arg.zGcsReport` and
+  // `ctx.arg.zGcsReportDynamic within the function. By doing this, it saves
+  // calling the function twice, as both values can be parsed at once.
+  getZGcsReport(ctx, args);
   ctx.arg.zGlobal = hasZOption(args, "global");
   ctx.arg.zGnustack = getZGnuStack(args);
   ctx.arg.zHazardplt = hasZOption(args, "hazardplt");
@@ -2877,7 +2880,7 @@ static void readSecurityNotes(Ctx &ctx) {
         << ": -z bti-report: file does not have "
            "GNU_PROPERTY_AARCH64_FEATURE_1_BTI property";
 
-    reportUnless(ctx.arg.zGcsReport.toString(),
+    reportUnless(gcsReportPolicytoString(ctx.arg.zGcsReport),
                  features & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
         << f
         << ": -z gcs-report: file does not have "
@@ -2955,7 +2958,7 @@ static void readSecurityNotes(Ctx &ctx) {
   // either `warning` or `error`.
   if (ctx.arg.andFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
     for (SharedFile *f : ctx.sharedFiles)
-      reportUnless(ctx.arg.zGcsReportDynamic.toString(),
+      reportUnless(gcsReportPolicytoString(ctx.arg.zGcsReportDynamic),
                    f->andFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
           << f
           << ": GCS is required by -z gcs, but this shared library lacks the "
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
@@ -1448,13 +1448,13 @@ std::vector<uint32_t> SharedFile::parseVerneed(const ELFFile<ELFT> &obj,
 template <typename ELFT>
 void SharedFile::parseGnuAndFeatures(const uint8_t *base,
                                      const typename ELFT::PhdrRange headers) {
-  if (numElfPhdrs == 0)
+  if (headers.size() == 0)
     return;
   uint32_t featureAndType = ctx.arg.emachine == EM_AARCH64
                                 ? GNU_PROPERTY_AARCH64_FEATURE_1_AND
                                 : GNU_PROPERTY_X86_FEATURE_1_AND;
 
-  for (unsigned i = 0; i < numElfPhdrs; i++) {
+  for (unsigned i = 0; i < headers.size(); i++) {
     if (headers[i].p_type != PT_GNU_PROPERTY)
       continue;
     const typename ELFT::Note note(
@@ -1510,10 +1510,7 @@ template <class ELFT> void SharedFile::parse() {
   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);
-  elfPhdrs = pHeaders.data();
-  numElfPhdrs = pHeaders.size();
 
   const Elf_Shdr *versymSec = nullptr;
   const Elf_Shdr *verdefSec = nullptr;
@@ -1584,7 +1581,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(), getELFPhdrs<ELFT>());
+  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
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
@@ -360,15 +360,6 @@ class SharedFile : public ELFFileBase {
   // parsed. Only filled for `--no-allow-shlib-undefined`.
   SmallVector<Symbol *, 0> requiredSymbols;
 
-  template <typename ELFT> typename ELFT::PhdrRange getELFPhdrs() const {
-    return typename ELFT::PhdrRange(
-        reinterpret_cast<const typename ELFT::Phdr *>(elfPhdrs), numElfPhdrs);
-  }
-
-protected:
-  const void *elfPhdrs = nullptr;
-  uint32_t numElfPhdrs = 0;
-
 private:
   template <typename ELFT>
   std::vector<uint32_t> parseVerneed(const llvm::object::ELFFile<ELFT> &obj,
diff --git a/lld/docs/ReleaseNotes.rst b/lld/docs/ReleaseNotes.rst
@@ -25,10 +25,10 @@ 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
+* 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
+  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
