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

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2525
+    // vector. We should not report a warning in this case.
+    bool IsEmptyHashTable =
+        GnuHashTable->symndx == NumSyms && BloomFilter.size() == 1 &&
----------------
MaskRay wrote:
> grimar wrote:
> > MaskRay wrote:
> > > `GnuHashTable->symndx == 1` is probably sufficient.
> > > 
> > > The other conditions are redundant.
> > > GnuHashTable->symndx == 1 is probably sufficient.
> > 
> > It is not, because it can be > 1. For example when we have an object with an
> > undefined dynamic symbol and use lld:
> > 
> > 
> > **1.s:**
> > .global bar
> > 
> > ```
> > as 1.s -o 1.o
> > lld 1.o -o 1.so -shared --hash-style=gnu
> > llvm-readelf --gnu-hash-table 1.so
> > ```
> > 
> > ```
> > GnuHashTable {
> >   Num Buckets: 1
> >   First Hashed Symbol Index: 2
> >   Num Mask Words: 1
> >   Shift Count: 26
> >   Bloom Filter: [0x0]
> >   Buckets: [0]
> >   Values: []
> > }
> > ```
> > 
> > For GNU ld the output is different, btw (but it does not seem to be critical to fix LLD here, I guess):
> > 
> > ```
> > ld.bfd 1.o -o 1.so -shared --hash-style=gnu
> > llvm-readelf --gnu-hash-table 1.so
> > ```
> > 
> > ```
> > GnuHashTable {
> >   Num Buckets: 1
> >   First Hashed Symbol Index: 1
> >   Num Mask Words: 1
> >   Shift Count: 0
> >   Bloom Filter: [0x0]
> >   Buckets: [0]
> >   Values: [0x0]
> > }
> > ```
> > 
> OK, `!Buckets.empty() && Buckets[0] == 0` is sufficient.
> 
> The number of buckets does not matter. The size of the bloom filter does not matter (it can preclude some symbols. Since there is no defined dynamic symbol, it shouldn't matter if the bloom filter precludes anything).
As far I understand, the first thing that any dynamic loader tries to do during a symbol lookup is that it checks agains the Bloom filter,
When we have a zeroed Bloom filter of any size, it means the object can't contain any symbol.

Then it means that what is really important for an empty gnu hash table is that the Bloom filter is zeroed.
And that I believe is what any linker produces and what we might want to check.
Anything else probably is not important.

The code to check against the filter can be:
(https://blogs.oracle.com/solaris/gnu-hash-elf-sections-v2)
```
      /* Test against the Bloom filter */
        c = sizeof (BloomWord) * 8;
        n = (h1 / c) & os->os_maskwords_bm;
        bitmask = (1 << (h1 % c)) | (1 << (h2 % c));
        if ((os->os_bloom[n] & bitmask) != bitmask)
                return (NULL);
```

So it uses `maskwords` and bloom filter words.
So we want to check that maskwords==1 and that the Bloom filter is empty.

Hence, I'd simplify this to:

```
bool IsEmptyHashTable = `GnuHashTable->maskwords == 1 && BloomFilter.size() == 1 && BloomFilter.front() == 0`;
```

Are you OK with this?




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

https://reviews.llvm.org/D73269