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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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.
----------------
jhenderson wrote:
> 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?
In real world they do not do that.
For example if you have an empty assembly file and do

```
as test.s -o test.o
ld.bfd test.o -o test.so -shared --hash-style=gnu
```

Then you'll see that GNU ld sets index to 1:

```
GnuHashTable {
  Num Buckets: 1
  First Hashed Symbol Index: 1
  Num Mask Words: 1
  Shift Count: 0
  Bloom Filter: [0x0]
  Buckets: [0]
  Values: []
}
```

While we have buckets empty or a Bloom filter empy it is enough for
the dynamic loader to know that there is no symbol it tries to lookup.
So linkers just do not care much about the rest. 

And I am not sure we even can think about this as about a bug:
the index value should point to the first hashed symbol index, but we hash nothing.
I am thinking about it as about unspecified behavior.


================
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"),
----------------
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).



================
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());
----------------
jhenderson wrote:
> 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.
We use "table" usually it seems, so I've switched to it.


================
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(
----------------
jhenderson wrote:
> 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.
OK


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

https://reviews.llvm.org/D73269