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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
