Page MenuHomePhabricator

[ELF] - Never create .gnu_hash with NBuckets == 0.
ClosedPublic

Authored by grimar on Mar 13 2018, 5:39 AM.

Details

Summary

I suggest setting NBuckets so that it is always >= 0.

Currently, we can end up with NBuckets==0 and android loader
does not like it (see PR36537).

The gold linker as we know use predefined table with values for NBuckets
and it never produces output where NBuckets == 0.

In theory, we could drop the whole hash table in this case probably,
as the only use case I can imagine now where it might harm is when
binary contains only undefined symbols, so that without hash section,
the loader will have to lookup for some symbol instead of instantly getting
the info that there are no symbols from the empty hash table. It's unrealistic I think.

But I also think that we probably can go with a minimal amount of changes here for simplicity
and be consistent with gold and so just always use >= 1 value for NBuckets.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

grimar created this revision.Mar 13 2018, 5:39 AM

I think that this is the simplest change that will pass through Bionic loaders check. The other possibility I thought of which might be a bit more honest is to add one of the symbols to the table even if it were a reference, that would minimise the risk of some other tool from complaining about hash table structure problems.

Unfortunately the bionic loader doesn't look like it will accept an executable without at least one of the ELF hash or GNU hash sections being present (and if present having nbuckets being non 0). I've got it on my list of things to do to raise a defect on the loader as a hash table should be an optimisation.

George Rimar via Phabricator via llvm-commits
<llvm-commits@lists.llvm.org> writes:

Index: ELF/SyntheticSections.cpp
===================================================================

    • ELF/SyntheticSections.cpp +++ ELF/SyntheticSections.cpp @@ -1799,15 +1799,16 @@ return SS->CopyRelSec == nullptr && !SS->NeedsPltAddr; return !S.Sym->isDefined(); });
  • if (Mid == V.end())
  • return;

    We chose load factor 4 for the on-disk hash table. For each hash collision, the dynamic linker will compare a uint32_t hash value. Since the integer comparison is quite fast, we believe we can make the load factor even larger. 4 is just a conservative choice. NBuckets = std::max<size_t>((V.end() - Mid) / 4, 1);

Please update the comment to say:

  • Why we want to keep this section: It indicates to a loader that the file exports no symbols.
  • Why we can't use 0: The android loader doesn't support that.

With that, LGTM.

Thanks,
Rafael

This revision was not accepted when it landed; it landed in state Needs Review.Mar 14 2018, 1:56 AM
This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.Mar 14 2018, 11:47 AM
ELF/SyntheticSections.cpp
1805

Please separate sentences with blank lines. The sentence about an Android loader workaround and "We chose ..." are talking about completely different stuff, so this is confusing.