[Openmp-commits] [PATCH] D99553: [libomptarget] Read standard notes for ELF offload images

Tue Mar 30 11:55:16 PDT 2021

vzakhari added a comment.

In D99553#2658055 <https://reviews.llvm.org/D99553#2658055>, @JonChesterfield wrote:

> That's a lot of code. Mostly straightforward, though the raw new/delete makes me slightly nervous. I think it's almost all here to abstract over libelf or llvm's elf library with a higher level interface. I haven't read through it carefully yet.

This is correct.  This is all here just to abstract ELF reading with a new interface.

> There is a profiling build that depends on llvm. Enabling that did break some builds. It might be worth reviewing whether to treat llvm binaries as a hard requirement for libomptarget.

The LLVM ELF implementation is disabled by default for upstream, so it should not cause any issues with LLVM component libraries.  I have tested the LLVM ELF implementation downstream, and I believe it is working now (I will upload my latest patch shortly).

> Aside from that, if we want to continue on this path. I suspect we don't need a third interface. In particular, we can probably implement llvm's libelf interface in terms of elf.h or vice versa. I'd lean towards converting the plugins to use llvm libelf's interface and providing a fallback implementation of the (few) non-header pieces that are used by the plugins. That gives zero bug surface for builds using llvm's libelf and a clear path to dropping the gnu libelf dependency.

Implementing GNU libelf interface as-is via LLVM ELF is problematic because this will require const casting of constant references/pointers exposed by LLVM ELF.  Another side effect of linking LLVM component libraries to the plugins is the binary size.  The plugins grow in size quite significantly, and I do not know whether this is acceptable for everyone.

Can you please clarify which non-header pieces you are referring to?

> The other way round, implementing the subset of libelf that we use in terms of llvm (or from scratch, elf isn't very complicated), is simpler today as the plugins don't have to change, but kind of ties us into an abstraction layer that looks like debt in the medium term.
>
> (tagging myself as reviewer because I'd like to drop the libelf dependency from the amdgpu plugin, and this is a path by which that could be done)

FWIW, generic-elf-64bit code includes link.h (for link_map declaration), which, in turn, includes system elf.h.  This will conflict with LLVM ELF, if we try to use it directly in generic-elf-64bit code.

I assume getting rid of GNU libelf dependency in all plugins will take some time.  Moreover, not all plugin developers may be willing to switch to LLVM ELF implementaion.  I wonder if we can start using the new interface, and then gradually switch to LLVM ELF and deprecate the new interface, if it is convenient for all.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99553