[PATCH] D74023: [RISCV] ELF attribute section for RISC-V

[Hsiangkai Wang via Phabricator via llvm-commits]

HsiangKai marked 3 inline comments as done.
HsiangKai added inline comments.


================
Comment at: lld/test/ELF/riscv-attributes.s:43-44
+_start:
+  .globl func
+  call func
----------------
jhenderson wrote:
> Do you need these? Is the `call` relevant, or can it just be `nop`?
Yes, this test case will link with another file located in `Inputs` directory. I create this test case to ensure this patch will not broken lld. So, I link two object files together and decode the ELF attributes.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:160-162
+  bool getFeatureBits(uint64_t feature) {
+    return getSTI().getFeatureBits()[feature];
+  }
----------------
jhenderson wrote:
> The rest of this code uses upper-case variable names. We should standardise on one or the other in the file throughout, I think.
I will follow current LLVM coding standards before there is a final decision about variable names.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.test:69
+    Type: SHT_RISCV_ATTRIBUTES
+    Content: 4132000000726973637600012800000004100572763332693270305F6D3270305F613270305F6332703000060008020A000C00
----------------
jhenderson wrote:
> MaskRay wrote:
> > Second to jhenderson's suggestion. This opaque content is not necessarily better than an assembly test.
> Seems like this comment still hasn't been addressed?
I create an assembly version in `test/tools/llvm-readobj/ELF/RISCV/attribute.s`. I just keep the binary format for the same test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74023