[PATCH] D131309: [ELF] Add ability to get a symbol by name from the hash table section

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D131309#3751136 <https://reviews.llvm.org/D131309#3751136>, @JonChesterfield wrote:

> Changing the hash table amdgpu uses would be an ABI break needing work in the HSA loader and possibly the windows stack as well. I'm not aware of any inclination to do that. Glibc's deprecation decisions have no direct bearing on amdgpu as it doesn't use glibc.
>
> This patch wants to move code from the openmp runtime into llvm. I'm not convinced that has any value relative to leaving it where it already is, perhaps Joseph would be better off applying these review induced changes to the runtime instead.

The host binaries do not use `DT_HASH` for many years. If AMDGPU wants to stick with DT_HASH, it's fine, but deviating from the host is probably a poor decision.

I do not object to using `DT_HASH` if AMDGPU needs it, but why does the patch expose `getSymbolFromHashTable` as an lib/Object API while it works well in the current setup that it stays as a private API in `openmp/libomptarget/plugins/amdgpu/src/rtl.cpp`?

`getSymbolFromHashTable` is not suitable if it only supports `DT_HASH`, as this is not useful for most Linux binaries:

- most have only `DT_GNU_HASH` and no `DT_HASH`
- for some having both, `DT_GNU_HASH` is used and `DT_HASH` is ignored
- the API does some dynamic loader work which I am not sure is suitable to lib/Object. Reloation resolving and dlsym have more rules than implemented here and the implementation does not handle symbol versioning. How fast do you want? If the object only has a couple of dynamic symbols, just iterate over the whole `DT_SYMTAB`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131309