Page MenuHomePhabricator

[OpenMP][WIP] Add standard notes for ELF offload images
AbandonedPublic

Authored by vzakhari on Mar 25 2021, 11:17 AM.

Details

Summary

This patch adds standard ELF notes into SHT_NOTE sections of ELF offload images passed to clang-offload-wrapper. The notes then can be read by the offload plugins to get some extra information about the image.

The new notes use a null-terminated "LLVMOMPOFFLOAD" note name. There are currently three types of notes:

  • VERSION: a string (not null-terminated) representing the ELF offload image structure. The current version '1.0' does not put any restrictions on the structure of the image. If we ever need to come up with a common structure for ELF offload images (e.g. to be able to analyze the images in libomptarget in some standard way), then we will introduce new versions.
  • PRODUCER: a vendor specific name of the producing toolchain. Upstream LLVM uses "LLVM" (not null-terminated).
  • PRODUCER_VERSION: a vendor specific version of the producing toolchain. Upstream LLVM uses LLVM_VERSION_STRING with optional <space> LLVM_REVISION.

All three notes are not mandatory currently.

The second part of the patch implements an ELF light interface for the plugins to be able to ierate ELF notes in SHT/PT_NOTE sections/segments. One implementation is based on libelf and it can be used for platforms, where libelf depdency can be easily satisfied. The second implementation is based on LLVM ELFObjectFile and requires in-tree build - this one can be used on Windows, etc.

Debug output from the plugins would look like this:

TARGET Common ELF --> LLVMOMPOFFLOAD ELF note NT_LLVM_OPENMP_OFFLOAD_VERSION with value: '1.0'
TARGET Common ELF --> LLVMOMPOFFLOAD ELF note NT_LLVM_OPENMP_OFFLOAD_PRODUCER with value: 'LLVM'
TARGET Common ELF --> LLVMOMPOFFLOAD ELF note NT_LLVM_OPENMP_OFFLOAD_PRODUCER_VERSION with value: '13.0.0git 9f8975163c75b1f9f736f9a8e0a60e29ac062754'

TODOs:

  • Find the right place to document clang-offload-wrapper behavior for ELF images.
  • Write LIT tests.
  • Decide how to test the LLVM ELF implementation of ELF light, since I expect the upstream builds will use libelf implementation.

Diff Detail

Event Timeline

vzakhari created this revision.Mar 25 2021, 11:17 AM
vzakhari requested review of this revision.Mar 25 2021, 11:17 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
vzakhari updated this revision to Diff 333378.Mar 25 2021, 11:45 AM
vzakhari updated this revision to Diff 333484.Mar 25 2021, 7:06 PM
vzakhari edited the summary of this revision. (Show Details)

Fixed issues detected in Windows testing downstream.

It might make sense to do the llvm-readobj portions of this patch in a separate review, since they are somewhat independent.

It might make sense to do the llvm-readobj portions of this patch in a separate review, since they are somewhat independent.

I agree. I actually have them in two patches, but I squashed them together for the complete story. If everyone agrees with this in general, I will upload two separate clang-formatted patches.

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.

clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
327

sys::findProgramByName tests the filename with an .exe suffix appended, no need to duplicate it in application code.

openmp/libomptarget/plugins/common/elf_common/elf_light.cpp
47

C++ does not require the use sites to prepend struct so typedef struct is not used by convention.

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.

clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
327

Thanks! Fixed in D99551.

openmp/libomptarget/plugins/common/elf_common/elf_light.cpp
47

Thanks! Fixed in D99553.

vzakhari abandoned this revision.Mar 30 2021, 12:49 PM

The revision was split into: D99612, D99551, D99552, D99553