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

Fri Nov 22 02:05:52 PST 2019

jhenderson added inline comments.

================
Comment at: llvm/test/tools/llvm-readobj/elf-versioninfo.test:298
+
+## TODO: llvm-readelf also should report a meaningful warning instead of an error.
+# INVALID-LINK-GNU: Version definition
----------------
also should -> should also

is more natural

================
Comment at: llvm/test/tools/llvm-readobj/elf-versioninfo.test:321
+
+# INVALID-STRING-TABLE: warning: '[[FILE]]': SHT_GNU_verdef section: can't use the string table linked: invalid sh_type for string table section [index 0]: expected SHT_STRTAB, but got SHT_NULL
+
----------------
I'd change this to:

sh_link field of SHT_GNU_verdef section at index N is invalid: the section at index 0 is not a string table: expected SHT_STRTAB but got SHT_NULL

================
Comment at: llvm/test/tools/llvm-readobj/elf-versioninfo.test:336
+
+## Check that we report a warning when a SHT_GNU_verdef section is linked with a non-string table section.
+
----------------
This comment doesn't line up with the warning we see below. I'd expect a similar warning to the previous test case.

================
Comment at: llvm/test/tools/llvm-readobj/elf-versioninfo.test:342
+
+# INVALID-DATA: warning: '[[FILE]]': SHT_GNU_verdef section: can't get content: section [index 1] has a sh_offset (0xffffffff) + sh_size (0x0) that cannot be represented
+
----------------
How about "cannot read content of SHT_GNU_verdef section: ..."

I'd ideally put the [index 1] before the colon, i.e. "... of SHT_GNU_verdef section with index 1: the section has a ...", but I'm not sure if that risks losing useful context elsewhere.

================
Comment at: llvm/test/tools/llvm-readobj/elf-versioninfo.test:351-355
+  - Name:    .gnu.version_d
+    Type:    SHT_GNU_verdef
+    Link:    .dynstr
+    Info:    0x0
+    Entries: []
----------------
Nit: this lot all need an extra space of padding to line up with ShOffset.

================
Comment at: llvm/test/tools/llvm-readobj/elf-versioninfo.test:367
+
+# DEF-PAST-END: warning: '[[FILE]]': SHT_GNU_verdef section: version definition 1 goes past the end of the section
+
----------------
invalid SHT_GNU_verdef section with index N: ...

================
Comment at: llvm/test/tools/llvm-readobj/elf-versioninfo.test:398
+
+# AUX-PAST-END: warning: '[[FILE]]': SHT_GNU_verdef section: a version definition 1 refers to an auxiliary entry that goes past the end of the section
+
----------------
invalid SHT_GNU_verdef section with index N: version definition 1 refers ...

================
Comment at: llvm/test/tools/llvm-readobj/elf-versioninfo.test:423
+## Check that we can dump a SHT_GNU_verdef section properly even if it contains version names strings
+## that overruns the string table linked.
+
----------------
overruns -> overrun
the string table linked -> the linked string table

Perhaps worth checking that no warning is printed?

================
Comment at: llvm/test/tools/llvm-readobj/elf-versioninfo.test:473
+
+# UNALIGNED-DEF: warning: '[[FILE]]': SHT_GNU_verdef section: found an unaligned version definition entry
+
----------------
invalid SHT_GNU_verdef section with index N: found a misaligned definition entry at offset X

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:427-428
+
+  Expected<std::vector<VerDef>> getVersionDefinitions(const ELFFile<ELFT> *Obj,
+                                                      const Elf_Shdr *Sec);
+
----------------
I'm not sure getVersionDefinitions belongs in the DumpStyle class, as it isn't a property of the style. I think it should be moved to the ElfDumper class, along with the other get* functions that are common to both dumping styles.

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3954
+
+    // FIXME: this error is untested.
+    if (uintptr_t(VerdauxBuf) % sizeof(uint32_t) != 0)
----------------
grimar wrote:
> MaskRay wrote:
> > 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`.
> > We should probably only use FIXME for real code problems. Here we should "TODO".
> Done.
> 
> > Actually I think the alignment check should be done outside the function. 
> Done.
> 
> >Maybe we should check the alignments of both VerdefBuf and D->vd_aux.
> This is checked already:
> 1) `VerdefBuf` is checked to be 4-bytes aligned. 
> 2) I do `VerdauxBuf = VerdefBuf + D->vd_aux` and check `VerdauxBuf`. This checks `D->vd_aux`.
> 3) I do `VerdefBuf += D->vd_next` and check `VerdefBuf`. This checks `D->vd_next`.
> 
Is there a reason we can't just test this as part of this change?

================
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");
----------------
grimar wrote:
> MaskRay wrote:
> > 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.
> > I guess this should be called "misaligned", not "unaligned", but I am not a native speaker and not very confident about this...
> 
> Perhaps "misaligned" is correct, since "If something is misaligned, then there's a correct alignment that has not been adhered to" and we have one in mind.
> (https://www.reddit.com/r/grammar/comments/6x1sov/difference_between_unaligned_and_misaligned/)
> 
"misaligned" sounds good to me and matches the `LoadVersionNeeds` function.

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4020
+  if (!V) {
+    reportWarning(V.takeError(), this->FileName);
+    return;
----------------
Maybe since we now have it, use `reportUniqueWarning`?

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5813
+  if (!V) {
+    reportWarning(V.takeError(), this->FileName);
+    return;
----------------
`reportUniqueWarning`?

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5828
+    W.printList(
+        "Predecessors", D.AuxV,
+        [](raw_ostream &OS, const VerdAux &Aux) { OS << Aux.Name.c_str(); });
----------------
Do we have any tests for multiple predecessors?

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

https://reviews.llvm.org/D70495