[PATCH] D136344: [ELF][RISCV] Merge `.riscv.attributes` sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Sorry for my previous belated response. I took a careful look today. I think a very large architectural refactoring is needed.
We probably want a synthetic section created at about `part.armExidx` time and follow its style merging sections.
Merging attributes as we see an input section is probably more convenient than the current approach maintaining two `DenseMap` with `DenseSet` as values (minor size concern) and iterating over all possible values.
I'll try an alternative implementation.
(I'll be out of town for about 1 week starting from Thursday, so sorry for my possible belated response. Hopefully I can get a draft out before that.)



================
Comment at: lld/ELF/Driver.cpp:2815
 
+  // Combine RISCV attributes.
+  if (config->emachine == EM_RISCV)
----------------
Prefer "RISC-V" in comments.


================
Comment at: lld/ELF/InputFiles.cpp:597
+        if (config->eflags & EF_RISCV_RVC) {
+          Optional<StringRef> Attr =
+              attributes.getAttributeString(RISCVAttrs::ARCH);
----------------
C++17 allows `if (Optional<StringRef> Attr = ...)`


================
Comment at: lld/ELF/SyntheticSections.cpp:3684
 
+static void parseInputRISCVAttributesSections(
+    const SmallVector<InputSectionBase *, 0> &attributesSections,
----------------
I'd use `parseRISCVAttributesSections`. "parse" implies `input` (we don't parse output).


================
Comment at: lld/ELF/SyntheticSections.cpp:3695
+    for (const auto &tag : attributesTags) {
+      Optional<unsigned> value = parser.getAttributeValue(tag.attr);
+      if (value) {
----------------
`if (Optional<unsigned> value = ...)`


================
Comment at: lld/ELF/SyntheticSections.cpp:3712
+  unsigned resultXlen;
+  for (const StringRef archValue : arches) {
+    // Parse arch string.
----------------
arch


================
Comment at: lld/ELF/SyntheticSections.cpp:3805
+    SmallVector<InputSectionBase *, 0> &&attributesSections) {
+  assert(!attributesSections.empty() &&
+         "No `.riscv.attributes` sections to merge");
----------------
Delete the assert. The call site has an emptiness check and the next line accesses `[0]`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136344