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

Mon Oct 31 08:13:06 PDT 2022

eklepilkina added inline comments.

================
Comment at: lld/ELF/SyntheticSections.cpp:3758
+                 resultExtensions[extension.first].MinorVersion ==
+                     extension.second.MinorVersion);
+          continue;
----------------
kito-cheng wrote:
> I think this assertion should be removed or use error instead, but I realized one fact soon is: we'll check the version during RISCVISAInfo::parseArchString and it will error out if the version is unsupported, so the assertion should always true for this moment.
> 
> Anyway I am OK with that for now, but I would prefer that let @MaskRay to make the final call for that.
> 
> Could you add a `TODO:` to the comment, like `TODO: Versions should be the same otherwise parser...`
Yes, it's true I can remove completely.

================
Comment at: lld/ELF/SyntheticSections.cpp:3781
+            *std::max_element(attributes[RISCVAttrs::STACK_ALIGN].begin(),
+                              attributes[RISCVAttrs::STACK_ALIGN].end());
+      }
----------------
kito-cheng wrote:
> This should just error if the stack alignment is different, see psABI for that https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#tag_riscv_stack_align-4-uleb128value
Thank you for link to documentation with merge rules. Fix merging attributes with these rules.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136344