[PATCH] D59186: [llvm-readobj] Separate `Symbol Version` dumpers into `LLVM style` and `GNU style`

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Are you definitely going to implement a GNU style after this? Otherwise, you've removed any form of support from the GNU dumper, without replacing it with something.



================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3364-3365
+template <class ELFT>
+void GNUStyle<ELFT>::printVersionSymbolSection(const ELFFile<ELFT> *Obj,
+                                               const Elf_Shdr *Sec) {
+  outs() << "dumping .gnu.version section in GNU style is not supported\n";
----------------
I think these unused named parameters might result in warnings on some build systems. Ditto below.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:3366
+                                               const Elf_Shdr *Sec) {
+  outs() << "dumping .gnu.version section in GNU style is not supported\n";
+}
----------------
Here, and below, I don't think you should mention the section names, unless you have actually derived the names from the section passed in.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4488
+  const uint8_t *VersymBuf = (const uint8_t *)Obj->base() + Sec->sh_offset;
+  StringRef StrTable = this->dumper()->getDynamicStringTable();
+
----------------
It may be worth factoring out `this->dumper()` into a local variable, since you use it in a number of places.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:4519
+    if (VerdefBuf + sizeof(Elf_Verdef) > SecEndAddress)
+      report_fatal_error("invalid offset in the section");
+
----------------
I know this was there before, but report_fatal_error is not a good way of reporting errors, since it implies an internal bug in LLVM. It should report a normal parse error or similar in the same way as other errors in llvm-readobj.

Ditto below.

I don't mind if it's in a follow-up patch though.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59186