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

[Hsiangkai Wang via Phabricator via llvm-commits]

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


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2670
+  if (!Obj->isLE()) {
+    W.startLine() << "Attributes not implemented.\n";
+    return;
----------------
jhenderson wrote:
> 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.
I add a test, `validate-attr-section.test`,  for it.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2703
+      if (Error E = RISCVAttributeParser(&W).parse(Contents, support::little))
+        consumeError(std::move(E));
+    }
----------------
jhenderson wrote:
> 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.
Print error messages out and add test cases for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75833