[PATCH] D70495: [llvm-readobj/llvm-readelf] - Reimplement dumping of the SHT_GNU_verdef section.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

The logic looks good to me now. I've only got a few nits.



================
Comment at: llvm/test/tools/llvm-readobj/elf-versioninfo.test:315
+
+## Check that we report a warning when a SHT_GNU_verdef section is linked with a non-string table section.
+
----------------
Probably ".. the sh_link field of a SHT_GNU_verdef section references a non-string table section"

I think the error/warning messages are a bit unnatural but I am not confident enough to suggest alternative messages. Thankfully we've got native speakers to suggest the wording :)


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3954
+
+    // FIXME: this error is untested.
+    if (uintptr_t(VerdauxBuf) % sizeof(uint32_t) != 0)
----------------
We should probably only use FIXME for real code problems. Here we should "TODO".

Actually I think the alignment check should be done outside the function. Maybe we should check the alignments of both `VerdefBuf` and `D->vd_aux`.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3979
+    if (uintptr_t(VerdefBuf) % sizeof(uint32_t) != 0)
+      return createError("SHT_GNU_verdef section: found an unaligned version "
+                         "definition entry");
----------------
I guess this should be called "misaligned", not "unaligned", but I am not a native speaker and not very confident about this...

Not checking "misaligned auxiliary entry" should be fine.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5825
+
+    if (D.AuxV.empty())
+      continue;
----------------
For consistency, shall we simply drop this `if` statement to print an empty list?


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

https://reviews.llvm.org/D70495