[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:6
+## This test uses option '-mattr=+e' to specify the "e" extension. However,
+## there is an architecture attribute in the file to specifying rv32i. We will
+## use rv32i to assemble the file instead of rv32e.
----------------
Sorry, missed one bit in my last comment: "to specifying" -> "to specify".


================
Comment at: llvm/test/MC/RISCV/attribute-with-option.s:17
+
+## Check that the architecture attribute is not overrided by the command line
+## option.
----------------
overrided -> overridden


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/invalid-attr-size.test:6
+
+# CHECK: invalid subsection length 0 at offset 0x1
+
----------------
Here and in the other tests, I'd add the full context for this message, i.e. something like `{{.*}}.o: error: invalid subsection length` (or whatever it is).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/invalid-attr-subsection-size.test:1
+# RUN: yaml2obj %s -D BITS=32 -o %t.32.o
+# RUN: not llvm-readobj -A %t.32.o 2>&1 | FileCheck %s
----------------
It's entirely up to you, but you might want to fold all these "invalid-attr" tests into a single test file, with lots of yaml docs. I'm happy either way.

I am a bit confused by "invalid-attr-size.test" and "invalid-attr-subsection-size.test" given that the former appears to emit an error to do with the subsection size, whilst the latter is an error to do with the attribute size itself. Did you switch them around by accident?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/invalid-attr-vendor.test:6
+
+# CHECK: unrecognized vendor-name: qiscv
+
----------------
Probably should be "vendor name" without the hyphen (unless it is called "vendor-name" in the section specification).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/RISCV/validate-attr-section.test:1
+## We only implement attribute section for little-endian encoding.
+
----------------
"attribute section printing"


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2695
 
     // TODO: Print error and delete the redundant FormatVersion check above.
+    if (Machine == EM_ARM) {
----------------
Can we remove this TODO (or at least part of it)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75833