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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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 &&
----------------
grimar wrote:
> 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?
> 
> 
The bloom filter saying "true" is a necessary condition. It the first thing to check, but it is not required to return "false" when there is a question "can symbol foo exist". It can say "true", and let the bucket/chain filter to say "false".
So, ``GnuHashTable->maskwords == 1 && BloomFilter.size() == 1 && BloomFilter.front() == 0`;` is indeed one way to make .gnu.hash reject all input, but it is not the only way.

All buckets being zeros (so `buckets[hash % num_buckets] = 0`) is a sufficient condition to say "false". This is another way to make .gnu.hash reject all input.

(Now I think the warning is probably not useful.)


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

https://reviews.llvm.org/D73269