[PATCH] D75833: [RISCV] Support RISC-V ELF attribute section in llvm-readobj

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/MC/RISCV/attribute-with-option.s:1
+## Different/conflict arch between option and attribute.
+
----------------
It's not clear what this comment is trying to say. Could you turn it into a complete sentence please, describing what the test is testing.


================
Comment at: llvm/test/MC/RISCV/attribute-with-option.s:7
+.attribute arch, "rv32i2p0"
+# Invalid operand for RV32E.
+lui x16, 1
----------------
'##'


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/attribute.s:1
+## Test llvm-readobj & llvm-readelf to decode RISCV attributes correctly.
+
----------------
to decode -> can decode

I think the canoncial name is "RISC-V", so you should use that in comments instead of "RISCV".


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/unknown-arch-attr.test:7
+## The object file has the "rv32i2p0_x1p0_m2p0_a2p0" arch feature. "x1p0" is an
+## unrecognized architecture extension. llvm-objdump will ignore it and decode
+## "mul" instruction correctly according to "m2p0" in the arch feature.
----------------
Is this test an llvm-objdump or llvm-readobj test? It looks to me like this test is testing llvm-objdump primarily, in which case it should be in the llvm-objdump directory, and maybe belongs in a separate change?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/unknown-arch-attr.test:38-39
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+# The content is the encoding of "mul a0, a1, a2".
+# The encoding could be decoded only when M-extension is enabled.
+    Content: 3385C502
----------------
'##'

Same below.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2670
+  if (!Obj->isLE()) {
+    W.startLine() << "Attributes not implemented.\n";
+    return;
----------------
Is there a test case for this line?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2676
+      Obj->getHeader()->e_machine != EM_RISCV) {
     W.startLine() << "Attributes not implemented.\n";
     return;
----------------
Is there a test case for this line?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2703
+      if (Error E = RISCVAttributeParser(&W).parse(Contents, support::little))
+        consumeError(std::move(E));
+    }
----------------
This is an existing mistake in the ARM version, but we shouldn't replicate it (and perhaps you could fix it separately). Unless `E` is going to be a duplicate of an already-reported error, this should report the error, probably as a warning, as there's no particular reason to terminate the program.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75833