Respond to review comments.

Summary of changes
* GcsReportPolicy enum has been changed to a struct. This allows for
the use of a `toString()` function so the values can be passed
to existing error lamdba functions within the Driver.cpp file.
* Removed uneeded additional includes
* `getZGcsReport` now supports the parsing of both `-zgcs-report`
and `-zgcs-report-dynamic`. The GNU inheritance rules are also
included in this function.
* `SharedFile::parseGnuAttributes` now correctly parses the
`.note.gnu.properties` section for the GCS Attributes.
* Release notes updated.
* aarch64-feature-gcs.s updated to be more streamline as per
review comments.

Author: Stylie777

lld/ELF/Config.h

@@ -137,7 +137,24 @@ enum LtoKind : uint8_t {UnifiedThin, UnifiedRegular, Default};
 enum class GcsPolicy { Implicit, Never, Always };
 
 // For -z gcs-report= and -zgcs-report-dynamic
-enum class GcsReportPolicy { None, Warning, Error };
+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; }
+};
 
 struct SymbolVersion {
   llvm::StringRef name;
@@ -396,8 +413,8 @@ struct Config {
   UnresolvedPolicy unresolvedSymbolsInShlib;
   Target2Policy target2;
   GcsPolicy zGcs;
-  GcsReportPolicy zGcsReport;
-  GcsReportPolicy zGcsReportDynamic;
+  GcsReportPolicy zGcsReport = GcsReportPolicy::None;
+  GcsReportPolicy zGcsReportDynamic = GcsReportPolicy::None;
   bool power10Stubs;
   ARMVFPArgKind armVFPArgs = ARMVFPArgKind::Default;
   BuildIdKind buildId = BuildIdKind::None;

lld/ELF/Driver.cpp

@@ -49,15 +49,11 @@
 #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/Compression.h"
@@ -72,7 +68,6 @@
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdlib>
-#include <memory>
 #include <tuple>
 #include <utility>
 
@@ -405,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 != GcsReportPolicy::None)
+    if (ctx.arg.zGcsReport.getValue() != GcsReportPolicy::None)
       ErrAlways(ctx) << "-z gcs-report only supported on AArch64";
-    if (ctx.arg.zGcsReportDynamic != GcsReportPolicy::None)
+    if (ctx.arg.zGcsReportDynamic.getValue() != 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";
@@ -581,54 +576,29 @@ static GcsPolicy getZGcs(Ctx &ctx, opt::InputArgList &args) {
   return ret;
 }
 
-static GcsReportPolicy getZGcsReport(Ctx &ctx, opt::InputArgList &args) {
+static GcsReportPolicy
+getZGcsReport(Ctx &ctx, opt::InputArgList &args, bool isReportDynamic,
+              GcsReportPolicy gcsReportValue = GcsReportPolicy::None) {
   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") {
+    if ((!isReportDynamic && kv.first == "gcs-report") ||
+        (isReportDynamic && 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= value: " << kv.second;
+      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)
+        ErrAlways(ctx) << "unknown -z " << kv.first << "= 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;
-  }
+  if (isReportDynamic && gcsReportValue.getValue() != GcsReportPolicy::None &&
+      ret.getValue() == GcsReportPolicy::None)
+    ret = GcsReportPolicy::Warning;
 
   return ret;
 }
@@ -1612,8 +1582,9 @@ 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.zGcsReport = getZGcsReport(ctx, args, false);
+  ctx.arg.zGcsReportDynamic =
+      getZGcsReport(ctx, args, true, ctx.arg.zGcsReport);
   ctx.arg.zGlobal = hasZOption(args, "global");
   ctx.arg.zGnustack = getZGnuStack(args);
   ctx.arg.zHazardplt = hasZOption(args, "hazardplt");
@@ -2891,17 +2862,6 @@ 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;
 
@@ -2911,8 +2871,8 @@ static void readSecurityNotes(Ctx &ctx) {
         << ": -z bti-report: file does not have "
            "GNU_PROPERTY_AARCH64_FEATURE_1_BTI property";
 
-    reportGcsPolicy(ctx.arg.zGcsReport,
-                    features & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
+    reportUnless(ctx.arg.zGcsReport.toString(),
+                 features & GNU_PROPERTY_AARCH64_FEATURE_1_GCS)
         << f
         << ": -z gcs-report: file does not have "
            "GNU_PROPERTY_AARCH64_FEATURE_1_GCS property";
@@ -2989,8 +2949,8 @@ static void readSecurityNotes(Ctx &ctx) {
   // 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)
+      reportUnless(ctx.arg.zGcsReportDynamic.toString(),
+                   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 "

lld/ELF/InputFiles.cpp

@@ -18,10 +18,8 @@
 #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"
@@ -1425,22 +1423,65 @@ 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.
+// To determine if a shared file can support any of the GNU Attributes,
+// the .note.gnu.properties section need to be read. This has to be done
+// differently for SharedFiles as the information available is not as
+// extensive as a normal object input file. This will take the program
+// headers, along with the SHT_NOTE section header to find the relevant
+// information. This uses a similar process to the readGnuProperty
+// function, but designed specifically for SharedFiles.
 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;
+void SharedFile::parseGnuAttributes(const uint8_t *base,
+                                    const typename ELFT::PhdrRange headers,
+                                    const typename ELFT::Shdr *sHeader) {
+  auto err = [&](const uint8_t *place) -> ELFSyncStream {
+    auto diag = Err(ctx);
+    diag << this->getName() << ":(" << ".note.gnu.properties" << "+0x"
+         << Twine::utohexstr(place - base) << "): ";
+    return diag;
+  };
+
+  if (numElfPhdrs == 0 || sHeader == nullptr)
+    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++) {
+    if (headers[i].p_type != PT_GNU_PROPERTY)
+      continue;
+
+    const typename ELFT::Note note(
+        *reinterpret_cast<const typename ELFT::Nhdr *>(base +
+                                                       headers[i].p_vaddr));
+    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.
+    ArrayRef<uint8_t> desc = note.getDesc(sHeader->sh_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(ctx, desc.data());
+      uint32_t size = read32(ctx, desc.data() + 4);
+      desc = desc.slice(8);
+      if (desc.size() < size)
+        return void(err(place) << "program property is too short");
+
+      // 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 (type == featureAndType) {
+        if (size < 4)
+          return void(err(place) << "FEATURE_1_AND entry is too short");
+        this->andFeatures |= read32(ctx, desc.data());
+      }
+
+      // Padding is present in the note descriptor, if necessary.
+      desc = desc.slice(alignTo<(ELFT::Is64Bits ? 8 : 4)>(size));
+    }
+  }
 }
 
 // We do not usually care about alignments of data in shared object
@@ -1487,6 +1528,7 @@ template <class ELFT> void SharedFile::parse() {
   const Elf_Shdr *versymSec = nullptr;
   const Elf_Shdr *verdefSec = nullptr;
   const Elf_Shdr *verneedSec = nullptr;
+  const Elf_Shdr *noteSec = nullptr;
   symbols = std::make_unique<Symbol *[]>(numSymbols);
 
   // Search for .dynsym, .dynamic, .symtab, .gnu.version and .gnu.version_d.
@@ -1507,6 +1549,9 @@ template <class ELFT> void SharedFile::parse() {
     case SHT_GNU_verneed:
       verneedSec = &sec;
       break;
+    case SHT_NOTE:
+      noteSec = &sec;
+      break;
     }
   }
 
@@ -1553,7 +1598,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>());
+  parseGnuAttributes<ELFT>(obj.base(), getELFPhdrs<ELFT>(), noteSec);
 
   // 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

lld/ELF/InputFiles.h

@@ -371,7 +371,9 @@ class SharedFile : public ELFFileBase {
   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);
+  void parseGnuAttributes(const uint8_t *base,
+                          const typename ELFT::PhdrRange headers,
+                          const typename ELFT::Shdr *sHeader);
 };
 
 class BinaryFile : public InputFile {

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

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

@@ -51,34 +51,26 @@
 
 ## 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
+# 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
+# RUN: ld.lld func1-gcs.o func3-gcs.o no-gcs.so force-gcs.so -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 -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=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 -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 -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 -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 -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
+# 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
-
-## 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
+# 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"