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

Tue Aug 30 18:26:45 PDT 2022

MaskRay added a comment.

In D131309#3759988 <https://reviews.llvm.org/D131309#3759988>, @jhuber6 wrote:

> In D131309#3757002 <https://reviews.llvm.org/D131309#3757002>, @MaskRay wrote:
>
>> 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`?
>
> In the process of transitioning `libomptarget` from `libelf` to `lib/Object` I figured this functionality was tied directly to ELF such that it justified pulling it out and testing directly. If you think it's not a proper fit for `lib/Object` you obviously have seniority here, where would this functionality go if not here? Or is this generally out of LLVM's scope.
>
>> `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
>
> I made a patch in D132743 <https://reviews.llvm.org/D132743> that adds the `DT_GNU_HASH` implementation, AMDGPU uses `DT_HASH` so I figured it was acceptable to support both given that `DT_HASH` isn't officially deprecated as far as I know.
>
>> - the API does some dynamic loader work which I am not sure is suitable to lib/Object. Relocation 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`.
>
> I believe we simply iterated the dynamic symbol table at some point, the number of symbols depends on the user. Large applications have a sufficiently large number of device functions / variables to justify a faster lookup.
>
> Yes, I believe the shortcomings of this implementation as it relates to dynamic loading is that we require section headers and do not handle symbol versions as you said. A better interface just for this could potentially have a `getSymbolFromGnuHashTable` and `getSymbolFromSysVHashTable` that don't use any section headers. Then have a function that attempts to find the relevant sections and call either one of those functions based on the ELF with optional versioning information. But as you mentioned this is pretty much re-implementing `dlsym`, which I don't know if LLVM has any desire to support that kind of functionality outside of our use-case.
>
> if you're against including this functionality we can simply merge it back into `libomptarget`. Personally I feel there is utility in having ELF hash table traversal present in LLVM, as is making the `gnu_hash` function readily available as in D132696 <https://reviews.llvm.org/D132696>. If you feel like this shouldn't be provided by LLVM we can easily move it back into `libomptarget`. Alternatively, if you provide a path forward to provide something that you find useful from this we can move forward with that.

Having the public API may lure users to improperly use it and what it does (DT_HASH) is very different from the practice.
I think the DT_HASH code should reside in `libomptarget`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131309