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

[Vyacheslav Zakharin via Phabricator via Openmp-commits]

vzakhari added a comment.

Thank you for the comments, Jon!

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

I will rebuild it from scratch to see how big the difference is in the release build.  I guess we do not care about the debug build (the code size hit is much bigger for the debug).

Just to reiterate, I believe we agreed that we should link LLVM ELF library statically, otherwise, it will have to become redistributable.  This may be quite inconvenient for some users.



================
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:
----------------
JonChesterfield wrote:
> 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.
I can split it, but I wanted to have at least something functional in this patch.


================
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,
----------------
JonChesterfield wrote:
> 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.
Yes, these constants are defined in D99612.

elf_constants.h is included for both libelf and LLVM ELF implementations.  If I include the LLVM ELF header file here it will conflict with libelf header files in libeld implementation.  I can get rid of these definitions in two cases:
# System elf.h gets these definitions, so that I can include two different headers defining these constants for two different implementations.
# We get rid of libelf dependency altogether.


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

https://reviews.llvm.org/D99553