[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:259
+
+## Linker might produce an empty no-op SHT_GNU_HASH section when
+## there are no dynamic symbols or when all dynamic symbols are undefined.
----------------
Linker -> either "Linkers" or "A linker"


================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnuhash.test:264-267
+## The index of the first symbol in the dynamic symbol table
+## 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.
----------------
I'm not sure I follow why we shouldn't report this in this specific case? Surely the linker should just set the index to 0, not 1?


================
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"),
----------------
I was going to suggest that this should be `reportUniqueWarning`, but I guess that it can only ever be hit once per file, right?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2507
+    reportWarning(createError("unable to dump 'Values' for the SHT_GNU_HASH "
+                              "section: found no dynamic symbol section"),
+                  ObjF->getFileName());
----------------
I'd move "found" to after "section". I might also change section -> table here, since the data does not technically need a section header, but follow whatever we say elsewhere.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2512
+
+  unsigned NumSyms = dynamic_symbols().size();
+  if (!NumSyms) {
----------------
`unsigned` -> `size_t`?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2521-2524
+    // A normal empty GNU hash table section produced by linker might have
+    // symndx set to the number of dynamic symbols + 1 (for the zero symbol)
+    // and have the dummy null values in the bloom filter and in the buckets
+    // vector. We should not report a warning in this case.
----------------
Similar comment to the test. Why is it okay for linkers to do this? At least explain this.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2530-2531
+    if (!IsEmptyHashTable) {
+      // Use -1 to print the number of dynamic symbols because we do not want to
+      // count the null symbol at the begining of a dynamic symbol table.
+      reportWarning(
----------------
I'm not sure I agree with this statement. The first symbol is the symbol with index 0. Saying we have 0 dynamic symbols is incorrect when there is just that symbol. After all the size of dynamic_symbols() will be 1.


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

https://reviews.llvm.org/D73269