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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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
+
----------------
jhenderson wrote:
> 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
It comes from:

```
Expected<StringRef> StrTabOrErr = Obj->getStringTable(*StrTabSecOrErr);
  if (!StrTabOrErr)
    return createError(
        "SHT_GNU_verdef section: can't use the string table linked: " +
        toString(StrTabOrErr.takeError()));
```

i.e. we know that `getStringTable` failed, but can't do any additional assumptions about details. `getStringTable()` reports the reason and it's text is
out of the area of this patch. I've changed the prefix part.


================
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
+
----------------
jhenderson wrote:
> 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.
> How about "cannot read content of SHT_GNU_verdef section: ..."

Done.

> 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.

I can only change the prefix, the rest part of the error comes from elsewhere.

```
 Expected<ArrayRef<uint8_t>> ContentsOrErr = Obj->getSectionContents(Sec);
  if (!ContentsOrErr)
    return createError("cannot read content of SHT_GNU_verdef section: " +
                       toString(ContentsOrErr.takeError()));
```


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:427-428
+
+  Expected<std::vector<VerDef>> getVersionDefinitions(const ELFFile<ELFT> *Obj,
+                                                      const Elf_Shdr *Sec);
+
----------------
jhenderson wrote:
> 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.
Yeah, looks true. Done.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3954
+
+    // FIXME: this error is untested.
+    if (uintptr_t(VerdauxBuf) % sizeof(uint32_t) != 0)
----------------
jhenderson wrote:
> 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?
> Is there a reason we can't just test this as part of this change?
I believe it is impossible to use yaml2obj here. So the only way to do that is probably to craft and checking a binary. Do you want me to do that?


================
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(); });
----------------
jhenderson wrote:
> Do we have any tests for multiple predecessors?
I've added one.


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

https://reviews.llvm.org/D70495