[PATCH] D73269: [llvm-readobj] - Add a few warnings for --gnu-hash-table.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnuhash.test:266-267
+## included in the hash table can be set to the number of dynamic symbols,
+## what is one larger that the index of the last dynamic symbol and we
+## normally should report it, but not in this case.
+
----------------
what -> which

What is "it" that we should normally report? I think that needs clarifying a little. I'd split the whole part from "and we..." into a new sentence, which more explicitly explains what we normally report, and say something like "For empty tables however, this value is unimportant and can be ignored."


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2506
+  if (!DynSymRegion.Addr) {
+    reportWarning(createError("unable to dump 'Values' for the SHT_GNU_HASH "
+                              "section: found no dynamic symbol section"),
----------------
grimar wrote:
> jhenderson wrote:
> > I was going to suggest that this should be `reportUniqueWarning`, but I guess that it can only ever be hit once per file, right?
> Yes. We take the `GnuHashTable` from the `DT_GNU_HASH` entry:
> 
> ```
>     case ELF::DT_GNU_HASH:
>       GnuHashTable = reinterpret_cast<const Elf_GnuHash *>(
>           toMappedAddr(Dyn.getTag(), Dyn.getPtr()));
> ```
> 
> and even if there are more than one of them (a possible broken case), we still will print only the last one.
> 
> Perhaps we want to change all of `reportWarning` to `reportUniqueWarning`. The benefit I see is that people
> who will look the code around and use copy-paste will copy-paste the right piece. But I am not sure
> it should be done in this patch. Perhaps there should be one separate patch that changes all warnings and
> adds missing test cases. I have no strong feeling about it, but I'd prefer a comment for each untested
> `reportUniqueWarning` call then (like one we could have here).
> 
All sounds reasonable to me. I'd actually make `reportWarning` report unique warnings if possible, with a possible extra argument (defaulted to true) that says whether to make them unique or not.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2525
+    // vector. It happens because the value of symndx is not important for
+    // dynamic loaders when they finds that the GNU hash table is empty. They
+    // just skips the whole object during symbol lookup then, and so linkers do
----------------
when they finds that the -> when the


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2526
+    // dynamic loaders when they finds that the GNU hash table is empty. They
+    // just skips the whole object during symbol lookup then, and so linkers do
+    // not care much too. We should not report a warning in this case.
----------------
skips -> skip
Delete "then"
And I'd change "and so linkers do not care much too" and the next sentence to say something like, in a new sentence, "In such cases, the symndx value is irrelevant and we should not report a warning."


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

https://reviews.llvm.org/D73269