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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

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

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



================
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) {
----------------
`sys::findProgramByName` tests the filename with an `.exe` suffix appended, no need to duplicate it in application code.


================
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.
----------------
C++ does not require the use sites to prepend `struct` so `typedef struct` is not used by convention.


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

https://reviews.llvm.org/D99360