[PATCH] D70826: [llvm-readobj/llvm-readelf] - Reimplement dumping of the SHT_GNU_verneed section.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/elf-verneed-invalid.test:92
 # GNU-NOLINK-NEXT:  Addr: 0000000000000000  Offset: 0x000044  Link: 0 ()
-# GNU-NOLINK-NEXT:   0x0000: Version: 1  File: <invalid>  Cnt: 1
-# GNU-NOLINK-NEXT:   0x0010:   Name: <invalid>  Flags: none Version: 2
+# GNU-NOLINK-EMPTY:
+# GNU-NOLINK-NEXT:  warning: '[[FILE]]': invalid string table linked to SHT_GNU_verneed section with index 2: invalid sh_type for string table section [index 0]: expected SHT_STRTAB, but got SHT_NULL
----------------
jhenderson wrote:
> No need for this EMPTY line.
It comes from `reportWarning`:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/llvm-readobj.cpp#L393

And we also print an additional '\n' when reporting errors:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/llvm-readobj.cpp#L369

I guess removing might cause issues like "error/warning is printed on the same like with the
normal output",  but haven't tried yet.

What do you think if I'll try to remove it in a follow-up?
(I had plan to investigate this issue to address the similar comment for D70665 anyways).


================
Comment at: llvm/test/tools/llvm-readobj/elf-verneed-invalid.test:340
+    Link:  0xFF
+    Dependencies:
+      - Version: 1
----------------
jhenderson wrote:
> Can you delete this Dependencies section to simplify the YAML?
I could, but then we'll have one more warning reported:
"invalid SHT_GNU_verneed section with index 1: version dependency 1 goes past the end of the section"

Here I tried to check that we emit only a single warning. I've extended this test to show that we continue printing the output.



================
Comment at: llvm/test/tools/llvm-readobj/elf-verneed-invalid.test:372
+    ShOffset: 0xFFFFFFFF
+    Dependencies:
+      - Version: 1
----------------
jhenderson wrote:
> Ditto
Done.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:468-483
+  unsigned SecNdx = Sec - &cantFail(Obj->sections()).front();
+
+  Expected<const Elf_Shdr *> StrTabSecOrErr = Obj->getSection(Sec->sh_link);
+  if (!StrTabSecOrErr)
+    return createError(
+        "invalid section linked to SHT_GNU_verneed section with index " +
+        Twine(SecNdx) + ": " + toString(StrTabSecOrErr.takeError()));
----------------
jhenderson wrote:
> I wonder if this block, which is common at least with the verdef section, could be pulled out into a function `getLinkAsStrtab` or something similar?
Done.


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

https://reviews.llvm.org/D70826