[libomptarget] Support BE ELF files in plugins-nextgen #85246
Conversation
Code in plugins-nextgen reading ELF files is currently hard-coded to assume a 64-bit little-endian ELF format. Unfortunately, this assumption is even embedded in the interface between GlobalHandler and Utils/ELF routines, which use ELF64LE types. To fix this, I've refactored the interface to use generic types, in particular by using (a unique_ptr to) ObjectFile instead of ELF64LEObjectFile, and ELFSymbolRef instead of ELF64LE::Sym. This allows properly templating over multiple ELF format variants inside Utils/ELF; specifically, this patch adds support for 64-bit big-endian ELF files in addition to 64-bit little-endian files.
| static Expected<std::optional<ELFSymbolRef>> | ||
| getHashTableSymbol(const ELFObjectFile<ELFT> &ELFObj, | ||
| const typename ELFT::Shdr &Sec, StringRef Name) { | ||
| const ELFFile<ELFT> &Elf = ELFObj.getELFFile(); |
There was a problem hiding this comment.
Why is this part changed? It's internal so it should be using the "actual" ELF format and not the Object file wrapper I would assume.
There was a problem hiding this comment.
We have to at some point convert a ELFT::Sym pointer into a ELFSymbolRef. This requires access to both the symbol table (section header) containing the symbol, and the Object file wrapper (which provides the ELFObj.toSymbolRef routine that actually creates the ELFSymbolRef. We could move the call to toSymbolRef to the caller, but there we don't have ready access to the symbol table, so we'd have to re-compute it again. Doing it inside getHashTableSymbol seems the lesser change to me.
| if (const ELF64LEObjectFile *ELFObj = dyn_cast<ELF64LEObjectFile>(&Obj)) | ||
| return getSymbolImpl(*ELFObj, Name); | ||
| if (const ELF64BEObjectFile *ELFObj = dyn_cast<ELF64BEObjectFile>(&Obj)) | ||
| return getSymbolImpl(*ELFObj, Name); |
There was a problem hiding this comment.
Can you not just do getELFFile here like before?
There was a problem hiding this comment.
We need the ObjectFile wrapper so we can call toSymbolRef, as discussed above.
|
Huh. Unfortunately, this still causes failures in the AMD GPU bots: |
Code in plugins-nextgen reading ELF files is currently hard-coded to assume a 64-bit little-endian ELF format. Unfortunately, this assumption is even embedded in the interface between GlobalHandler and Utils/ELF routines, which use ELF64LE types. To fix this, I've refactored the interface to use generic types, in particular by using (a unique_ptr to) ObjectFile instead of ELF64LEObjectFile, and ELFSymbolRef instead of ELF64LE::Sym. This allows properly templating over multiple ELF format variants inside Utils/ELF; specifically, this patch adds support for 64-bit big-endian ELF files in addition to 64-bit little-endian files.
|
OK, found the bug. We need to translate a NULL ELFT::Sym pointer into a std::nullopt to indicate a symbol is not found. I've reapplied the PR with this bug fixed, and now all build bots finally remain green! Thanks for your patience in getting this resolved. |
The plugin was not getting built as the build_generic_elf64 macro assumes the LLVM triple processor name matches the CMake processor name, which is unfortunately not the case for SystemZ. Fix this by providing two separate arguments instead. Actually building the plugin exposed a number of other issues causing various test failures. Specifically, I've had to add the SystemZ target to - CompilerInvocation::ParseLangArgs - linkDevice in ClangLinuxWrapper.cpp - OMPContext::OMPContext (to set the device_kind_cpu trait) - LIBOMPTARGET_ALL_TARGETS in libomptarget/CMakeLists.txt - a check_plugin_target call in libomptarget/src/CMakeLists.txt Finally, I've had to set a number of test cases to UNSUPPORTED on s390x-ibm-linux-gnu; all these tests were already marked as UNSUPPORTED for x86_64-pc-linux-gnu and aarch64-unknown-linux-gnu and are failing on s390x for what seem to be the same reason. In addition, this also requires support for BE ELF files in plugins-nextgen: #85246
Code in plugins-nextgen reading ELF files is currently hard-coded to assume a 64-bit little-endian ELF format. Unfortunately, this assumption is even embedded in the interface between GlobalHandler and Utils/ELF routines, which use ELF64LE types.
To fix this, I've refactored the interface to use generic types, in particular by using (a unique_ptr to) ObjectFile instead of ELF64LEObjectFile, and ELFSymbolRef instead of ELF64LE::Sym.
This allows properly templating over multiple ELF format variants inside Utils/ELF; specifically, this patch adds support for 64-bit big-endian ELF files in addition to 64-bit little-endian files.