[Openmp-commits] [PATCH] D131309: [Libomptarget] Add utility functions for loading an ELF symbol by name

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added inline comments.


================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:1613
 
-int getSymbolInfoWithoutLoading(Elf *Elf, char *Base, const char *Symname,
-                                SymbolInfo *Res) {
-  if (elf_kind(Elf) != ELF_K_ELF) {
-    return 1;
-  }
-
-  Elf64_Shdr *SectionHash = findOnlyShtHash(Elf);
-  if (!SectionHash) {
-    return 1;
-  }
-
-  const Elf64_Sym *Sym = elfLookup(Elf, Base, SectionHash, Symname);
-  if (!Sym) {
+int getSymbolInfoWithoutLoading(const ELFObjectFile<ELF64LE> &ELFObj,
+                                StringRef SymName, SymbolInfo *Res) {
----------------
JonChesterfield wrote:
> Possible follow up is to have this return an Expected<SymbolInfo> but that's not worth doing inline with this patch
Once the plugins are rewritten we'll probably switch over.


================
Comment at: openmp/libomptarget/plugins/common/elf_common/ELFSymbols.cpp:135
+  }
+
+  return nullptr;
----------------
JonChesterfield wrote:
> So one of these is presumably dead, since the only caller is amdgpu. Does cuda use the other sort of hash table? If not, suggest we delete the unused code path for now
I don't think CUDA even uses a hash table as it's some weird executable, for that we will need the exhaustive search below. I mostly kept this because we may have more users in the future and it was easier to just include it all since I already wrote it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131309