[PATCH] D136344: [ELF][RISCV] Merge `riscv.attributes` sections from all input files

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Sorry for the delay. I think this still requires some changes.



================
Comment at: lld/ELF/Driver.cpp:2821
+
+    // Collect all `.riscv.attributes` sections.
+    SmallVector<InputSectionBase *, 0> attributesSections;
----------------
"Combine RISCV attributes." and this comment have the same meaning. Just place the comment before `if`.

This block does not belong to generic code. Move it to Arch/RISCV.cpp


================
Comment at: lld/ELF/Driver.cpp:2826
+    if (!attributesSections.empty()) {
+      // Need to merge different attributes.
+      InputSectionBase *resultAttributesSection =
----------------
This comment does not say beyond what the code explains. Remove it.


================
Comment at: lld/ELF/Driver.cpp:2832
+      // Replace all existing attributes sections with merged one.
+      assert(
+          !ctx.inputSections.empty() &&
----------------
ctx.inputSections[0] being an attribute section is possible, so the code should handle the case, instead of using an assert which would lead to a runtime crash (either assert or attributesSectionPlace).


================
Comment at: lld/ELF/InputFiles.cpp:605
+            if (!parseResult) {
+              error(std::string("Invalid arch name: ") + arch.str());
+            }
----------------
no braces around single line simple statements.


================
Comment at: lld/ELF/InputFiles.cpp:607
+            }
+            auto &isaInfo = *parseResult;
+            if (!isaInfo->hasExtension("c")) {
----------------
avoid the used-once variable


================
Comment at: lld/ELF/InputFiles.cpp:609
+            if (!isaInfo->hasExtension("c")) {
+              error(toString(this) +
+                    " has set set RISCV_RVC flag, but RISCV_ARCH tag from " +
----------------
errorOrWarn so that users can ignore the error with --noinhibit-exec.


================
Comment at: lld/ELF/SyntheticSections.cpp:3684
 
+// Parse `.riscv.attributes` sections from input files.
+static void parseInputRISCVAttributesSections(
----------------
Delete comments when the function name is self-explanatory.

There are many places where a redundant comment is added. Please drop.


================
Comment at: lld/ELF/SyntheticSections.cpp:3705
+      error(toString(s) + ": " + llvm::toString(std::move(e)));
+    }
+    for (const auto &currentTag : attributesTags) {
----------------
MaskRay wrote:
> drop braces
not done


================
Comment at: lld/ELF/SyntheticSections.h:1197
+  llvm::DenseMap<unsigned, std::string> mergedStringAttributes;
+  // Current vendor.
+  llvm::StringRef currentVendor = "riscv";
----------------
The comment is not useful.


================
Comment at: lld/ELF/SyntheticSections.h:1198
+  // Current vendor.
+  llvm::StringRef currentVendor = "riscv";
+
----------------
This variable can be a const. `constexpr`


================
Comment at: lld/ELF/SyntheticSections.h:1308
   std::unique_ptr<InputSection> attributes;
+  std::unique_ptr<RISCVMergedAttributeSection> riscvAttributes;
   std::unique_ptr<BssSection> bss;
----------------
This must be reset in `reset()`


================
Comment at: lld/test/ELF/riscv-merge-attributes.s:40
 .attribute 5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0"
+
----------------
no trailing blank line


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136344/new/

https://reviews.llvm.org/D136344