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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

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.

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.

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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99553