[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
+## When user specifies conflicting architecture extension with arch attributes,
+## we use the arch attributes instead of command line options.
----------------
When a user specifies an architecture extension which conflicts with an arhitecture attribute, we use the architecture attribute instead of the  command line option.


================
Comment at: llvm/test/MC/RISCV/attribute-with-option.s:4
+##
+## This test uses option '-mattr=+e' to specify e-extension. However, there is
+## an arch attribute in the file to specify rv32i. So, we will use rv32i to
----------------
specify the "e" extension


================
Comment at: llvm/test/MC/RISCV/attribute-with-option.s:5
+## This test uses option '-mattr=+e' to specify e-extension. However, there is
+## an arch attribute in the file to specify rv32i. So, we will use rv32i to
+## assemble the file instead of rv32e.
----------------
arch -> architecture
specify -> specifying
So, we -> This means we


================
Comment at: llvm/test/MC/RISCV/attribute-with-option.s:12
+.attribute arch, "rv32i2p0"
+## Invalid operand for RV32E. x16 is an invalid register for RV32E.
+## Use RV32I to assemble. So, it will not trigger an assembly error.
----------------
RV32E. x16 -> RV32E, because x16


================
Comment at: llvm/test/MC/RISCV/attribute-with-option.s:13
+## Invalid operand for RV32E. x16 is an invalid register for RV32E.
+## Use RV32I to assemble. So, it will not trigger an assembly error.
+lui x16, 1
----------------
assemble. So, it -> assemble, since it


================
Comment at: llvm/test/MC/RISCV/attribute-with-option.s:16
+
+## Check the arch attribute is not overrided by command line options.
+# CHECK:      Tag: 5
----------------
Check that the architecture attribute is not overridden by the command line option.



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2670
+  if (!Obj->isLE()) {
+    W.startLine() << "Attributes not implemented.\n";
+    return;
----------------
HsiangKai wrote:
> jhenderson wrote:
> > Is there a test case for this line?
> No, it is not related to this patch. Originally, printAttributes() is only implemented for `ELFDumper<ELF32LE>`. So, I check `Obj->isLE()` to keep original logic. In this way, it does not change the functionality of original version. So, I did not add a test case for it.
The `!Obj->isLE()` line is new, so I think we need a test case that exercises this line as part of this change. You may be adding it to preserve existing behaviour, in which case, consider adding a test for it that in a prerequisite patch, to show that there is no behaviour change.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2676
+      Obj->getHeader()->e_machine != EM_RISCV) {
     W.startLine() << "Attributes not implemented.\n";
     return;
----------------
HsiangKai wrote:
> jhenderson wrote:
> > Is there a test case for this line?
> No, it is not related to this patch.
Yes it is. The behaviour has gone from "not EM_ARM" to "not EM_ARM and not EM_RISCV", which means the code here is now exercised differently. Again, you could add a prerequisite test that shows that non-ARM architectures hit this. Then, in this patch, you would remove the EM_RISCV case.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2703
+      if (Error E = RISCVAttributeParser(&W).parse(Contents, support::little))
+        consumeError(std::move(E));
+    }
----------------
HsiangKai wrote:
> jhenderson wrote:
> > 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.
> We could fix it separately.
If you want to fix the ARM case separately, that's fine, but we shouldn't introduce the same mistake with new code for RISC-V.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75833