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

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

> 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.

Ah, yes. GNU uses char* everywhere. We could implement that using the interfaces that take const char* by malloc&memcpy&free, but that is fairly horrible. We could implement the llvm elf interface in terms of gnu by casting away the const and hoping GNU doesn't actually mutate the arguments, but that seems hazardous too. Overall this seems to be digging in the wrong direction - we are taking on a lot of debt in order to preserve the ability to use libelf, which is only of use to toolchains that want to use llvm's libomptarget without compiling llvm.

> 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.

This doesn't seem necessary. Either we dynamically link the LLVMObject and the footprint will hopefully be comparable to using libelf, or we statically link it and let the linker deadstrip.



================
Comment at: openmp/libomptarget/plugins/common/elf_common/elf_common.cpp:54
+        break;
+      case NT_LLVM_OPENMP_OFFLOAD_VERSION:
+      case NT_LLVM_OPENMP_OFFLOAD_PRODUCER:
----------------
Missed this at first glance. This patch is a functional change - it abstracts over the elf interface, and also does some new stuff with said interface. It would probably be easier/safer to review when split down that line.


================
Comment at: openmp/libomptarget/plugins/common/elf_common/elf_constants.h:20
+enum : unsigned {
+  NT_LLVM_OPENMP_OFFLOAD_VERSION = 1,
+  NT_LLVM_OPENMP_OFFLOAD_PRODUCER = 2,
----------------
This enum is presumably in llvm somewhere. We should include it from llvm (note that some other header-only files are already included from llvm, and the cmake ensures that it is available when building libomptarget). We may need to slightly refactor llvm first to make the numbers available without pulling it lots of other stuff.


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

https://reviews.llvm.org/D99553