[PATCH] D99360: [OpenMP][WIP] Add standard notes for ELF offload images

[Vyacheslav Zakharin via Phabricator via llvm-commits]

vzakhari added a comment.

In D99360#2654244 <https://reviews.llvm.org/D99360#2654244>, @MaskRay wrote:

>> It might make sense to do the llvm-readobj portions of this patch in a separate review, since they are somewhat independent.
>
> +1. Patches touching BinaryFormat/llvm-readobj/yaml2obj/etc part and others (MC/CodeGen/etc) are often required to do BinaryFormat/llvm-readobj/yaml2obj/etc separately.
> The part is usually very loosely connected with the MC/CodeGen/etc part code.

Thanks. I split the patches.

>> Decide how to test the LLVM ELF implementation of ELF light, since I expect the upstream builds will use libelf implementation.
>
> Does the current code use both llvm/Object and libelf? First, for in-tree components we exclusively use llvm/Object - there should be no dependency on the external libelf.

The current code uses only libelf.  I believe many OpenMP offload plugins have been using libelf for a long time, and they did not switch to LLVM/Object, when libomptarget was merged into the tree.  I am not going to change this now, because this may cause too much disturbance downstream.  The purpose of this patch is to make some ELF reading functionality available in OSes, where it is (historically) unreasonable to require libelf dependency for user environments.

> Having two implementations can make llvm/Object API refactoring difficult. Other contributors will not know that an libelf implementation (a different configuration) exists and can break the libelf implementation.

I think it will be actually vice versa :)  the upstream and many downstream repos will continue to use the libelf path, and the LLVM/Object path may be broken, as you say.  At the same time, we will have QA testing downstream for the LLVM/Object path, so I will be able to catch issues and upstream the fixes.



================
Comment at: clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp:327
+      // Look for llvm-objcopy.exe as well.
+      ObjcopyPathOrErr = sys::findProgramByName("llvm-objcopy.exe");
+      if (!ObjcopyPathOrErr) {
----------------
MaskRay wrote:
> `sys::findProgramByName` tests the filename with an `.exe` suffix appended, no need to duplicate it in application code.
Thanks! Fixed in D99551.


================
Comment at: openmp/libomptarget/plugins/common/elf_common/elf_light.cpp:47
+//        for 64-bit ELFs produced by LLVM.
+typedef struct {
+  uint32_t n_namesz; // Length of note's name.
----------------
MaskRay wrote:
> C++ does not require the use sites to prepend `struct` so `typedef struct` is not used by convention.
Thanks! Fixed in D99553.


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

https://reviews.llvm.org/D99360