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

[Vyacheslav Zakharin via Phabricator via cfe-commits]

vzakhari abandoned this revision.
vzakhari added a comment.

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


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

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

[Vyacheslav Zakharin via Phabricator via cfe-commits]

vzakhari added a comment.

In 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vyacheslav Zakharin via Phabricator via cfe-commits]

vzakhari added a comment.

In D99360#2652523 , @jhenderson wrote:

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


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

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

[James Henderson via Phabricator via cfe-commits]

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


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

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

[Vyacheslav Zakharin via Phabricator via cfe-commits]

vzakhari updated this revision to Diff 333484.
vzakhari edited the summary of this revision.
vzakhari added a comment.

Fixed issues detected in Windows testing downstream.


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

https://reviews.llvm.org/D99360

Files:
  clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
  llvm/include/llvm/BinaryFormat/ELF.h
  llvm/tools/llvm-readobj/ELFDumper.cpp
  openmp/libomptarget/plugins/amdgpu/CMakeLists.txt
  openmp/libomptarget/plugins/common/elf_common/CMakeLists.txt
  openmp/libomptarget/plugins/common/elf_common/elf_common.cpp
  openmp/libomptarget/plugins/common/elf_common/elf_common.h
  openmp/libomptarget/plugins/common/elf_common/elf_constants.h
  openmp/libomptarget/plugins/common/elf_common/elf_light.cpp
  openmp/libomptarget/plugins/common/elf_common/elf_light.h
  openmp/libomptarget/plugins/remote/server/CMakeLists.txt

Index: openmp/libomptarget/plugins/remote/server/CMakeLists.txt
===
--- openmp/libomptarget/plugins/remote/server/CMakeLists.txt
+++ openmp/libomptarget/plugins/remote/server/CMakeLists.txt
@@ -10,7 +10,6 @@
 #
 ##===--===##
 
-include_directories(${LIBOMPTARGET_DEP_LIBELF_INCLUDE_DIRS})
 include_directories(${LIBOMPTARGET_SRC_DIR})
 include_directories(${LIBOMPTARGET_INCLUDE_DIR})
 include_directories(${GRPC_INCLUDE_DIR})
@@ -28,4 +27,4 @@
 grpc++
 protobuf
 ${OPENMP_PTHREAD_LIB}
-"-ldl" "-lomp" "-fopenmp" "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/../../exports" ${LIBOMPTARGET_DEP_LIBELF_LIBRARIES})
+"-ldl" "-lomp" "-fopenmp" "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/../../exports")
Index: openmp/libomptarget/plugins/common/elf_common/elf_light.h
===
--- /dev/null
+++ openmp/libomptarget/plugins/common/elf_common/elf_light.h
@@ -0,0 +1,132 @@
+//===-- elf_light.h - Basic ELF functionality ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Light ELF implementation provides basic ELF reading functionality.
+// It may be used in systems without libelf support, if the corresponding
+// LLVM ELF implementation is available.
+// The interface declared here must be independent of libelf.h/elf.h.
+//
+// NOTE: we can try to rely on https://github.com/WolfgangSt/libelf
+//   on Windows, if this implementation gets more complex.
+//
+//===--===//
+
+#ifndef LIBOMPTARGET_PLUGINS_ELF_LIGHT_H
+#define LIBOMPTARGET_PLUGINS_ELF_LIGHT_H
+
+#include 
+#include 
+#include 
+
+class ElfL;
+class ElfLSegmentNoteIterator;
+class ElfLSectionNoteIterator;
+class ElfNote;
+
+// Class representing NOTEs from PT_NOTE segments and SHT_NOTE sections.
+class ElfLNote {
+  const void *Impl = nullptr;
+
+  friend class ElfLSegmentNoteIterator;
+  friend class ElfLSectionNoteIterator;
+
+  // Only ElfLSectionNoteIterator is allowed to create notes via its operator*().
+  explicit ElfLNote(const void *I);
+  ElfLNote &operator=(const ElfLNote &) = delete;
+
+public:
+  // FIXME: add move copy constructor and assignment operator.
+  ElfLNote(const ElfLNote &);
+  ~ElfLNote();
+  // Returns the note's name size not including the null terminator.
+  // Note that it may be illegal to access the getName() pointer
+  // beyond the returned size, i.e. the implementation may
+  // not guarantee that there is '\0' after getNameSize()
+  // characters of the name.
+  uint64_t getNameSize() const;
+  // Returns a pointer to the beginning of the note's name.
+  const char *getName() const;
+  // Returns the number of bytes in the descriptor.
+  uint64_t getDescSize() const;
+  // Returns a pointer to the beginning of the note's descriptor.
+  // It is illegal to access more that getDescSize() bytes
+  // via this pointer.
+  const uint8_t *getDesc() const;
+  uint64_t getType() const;
+};
+
+// Iterator over NOTEs in PT_NOTE segments.
+class ElfLSegmentNoteIterator
+: std::iterator {
+
+  void *Impl = nullptr;
+
+  friend class ElfL;
+
+  // Only ElfL is allowed to create iterators to itself.
+  ElfLSegmentNoteIterator(const void *I, bool IsEnd = false);
+  ElfLSectionNoteIterator &operator=(const ElfLSegmentNoteIterator &) = delete;
+
+public:
+  // FIXME: add move copy constructor and assignment operator.
+  ElfLSegmentNoteIterator(const ElfLSegmentNoteIterator &Other);
+  ~ElfLSegmentNoteIterator();
+  ElfLSegmentNoteIterator &operator++();
+  bool operator==(const ElfLSegmentNoteIterator Other) const;
+  bool operator!=(const ElfLSegmentNoteIterator Other)

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

[Vyacheslav Zakharin via Phabricator via cfe-commits]

vzakhari updated this revision to Diff 78.

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

https://reviews.llvm.org/D99360

Files:
  clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
  llvm/include/llvm/BinaryFormat/ELF.h
  llvm/tools/llvm-readobj/ELFDumper.cpp
  openmp/libomptarget/plugins/amdgpu/CMakeLists.txt
  openmp/libomptarget/plugins/common/elf_common/CMakeLists.txt
  openmp/libomptarget/plugins/common/elf_common/elf_common.cpp
  openmp/libomptarget/plugins/common/elf_common/elf_common.h
  openmp/libomptarget/plugins/common/elf_common/elf_constants.h
  openmp/libomptarget/plugins/common/elf_common/elf_light.cpp
  openmp/libomptarget/plugins/common/elf_common/elf_light.h
  openmp/libomptarget/plugins/remote/server/CMakeLists.txt

Index: openmp/libomptarget/plugins/remote/server/CMakeLists.txt
===
--- openmp/libomptarget/plugins/remote/server/CMakeLists.txt
+++ openmp/libomptarget/plugins/remote/server/CMakeLists.txt
@@ -10,7 +10,6 @@
 #
 ##===--===##
 
-include_directories(${LIBOMPTARGET_DEP_LIBELF_INCLUDE_DIRS})
 include_directories(${LIBOMPTARGET_SRC_DIR})
 include_directories(${LIBOMPTARGET_INCLUDE_DIR})
 include_directories(${GRPC_INCLUDE_DIR})
@@ -28,4 +27,4 @@
 grpc++
 protobuf
 ${OPENMP_PTHREAD_LIB}
-"-ldl" "-lomp" "-fopenmp" "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/../../exports" ${LIBOMPTARGET_DEP_LIBELF_LIBRARIES})
+"-ldl" "-lomp" "-fopenmp" "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/../../exports")
Index: openmp/libomptarget/plugins/common/elf_common/elf_light.h
===
--- /dev/null
+++ openmp/libomptarget/plugins/common/elf_common/elf_light.h
@@ -0,0 +1,122 @@
+//===-- elf_light.h - Basic ELF functionality ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Light ELF implementation provides basic ELF reading functionality.
+// It may be used in systems without libelf support, if the corresponding
+// LLVM ELF implementation is available.
+// The interface declared here must be independent of libelf.h/elf.h.
+//
+// NOTE: we can try to rely on https://github.com/WolfgangSt/libelf
+//   on Windows, if this implementation gets more complex.
+//
+//===--===//
+
+#ifndef LIBOMPTARGET_PLUGINS_ELF_LIGHT_H
+#define LIBOMPTARGET_PLUGINS_ELF_LIGHT_H
+
+#include 
+#include 
+#include 
+
+class ElfL;
+class ElfLSegmentNoteIterator;
+class ElfLSectionNoteIterator;
+class ElfNote;
+
+// Class representing NOTEs from PT_NOTE segments and SHT_NOTE sections.
+class ElfLNote {
+  const void *Impl = nullptr;
+
+  friend class ElfLSegmentNoteIterator;
+  friend class ElfLSectionNoteIterator;
+
+  // Only ElfLSectionNoteIterator is allowed to create notes via its operator*().
+  explicit ElfLNote(const void *I);
+  ElfLNote &operator=(const ElfLNote &) = delete;
+
+public:
+  // FIXME: add move copy constructor and assignment operator.
+  ElfLNote(const ElfLNote &);
+  ~ElfLNote();
+  uint64_t getNameSize() const;
+  const char *getName() const;
+  uint64_t getDescSize() const;
+  const uint8_t *getDesc() const;
+  uint64_t getType() const;
+};
+
+// Iterator over NOTEs in PT_NOTE segments.
+class ElfLSegmentNoteIterator
+: std::iterator {
+
+  void *Impl = nullptr;
+
+  friend class ElfL;
+
+  // Only ElfL is allowed to create iterators to itself.
+  ElfLSegmentNoteIterator(const void *I, bool IsEnd = false);
+  ElfLSectionNoteIterator &operator=(const ElfLSegmentNoteIterator &) = delete;
+
+public:
+  // FIXME: add move copy constructor and assignment operator.
+  ElfLSegmentNoteIterator(const ElfLSegmentNoteIterator &Other);
+  ~ElfLSegmentNoteIterator();
+  ElfLSegmentNoteIterator &operator++();
+  bool operator==(const ElfLSegmentNoteIterator Other) const;
+  bool operator!=(const ElfLSegmentNoteIterator Other) const;
+  ElfLNote operator*() const;
+};
+
+// Iterator over NOTEs in SHT_NOTE sections.
+class ElfLSectionNoteIterator
+: std::iterator {
+
+  void *Impl = nullptr;
+
+  friend class ElfL;
+
+  // Only ElfL is allowed to create iterators to itself.
+  ElfLSectionNoteIterator(const void *I, bool IsEnd = false);
+  ElfLSectionNoteIterator &operator=(const ElfLSectionNoteIterator &) = delete;
+
+public:
+  // FIXME: add move copy constructor and assignment operator.
+  ElfLSectionNoteIterator(const ElfLSectionNoteIterator &Other);
+  ~ElfLSectionNoteIterator();
+  ElfLSectionNoteIterator &operator++();
+  bool operator==(const ElfLSectionNoteIterator Other) const;

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

[Vyacheslav Zakharin via Phabricator via cfe-commits]

vzakhari created this revision.
vzakhari added a project: OpenMP.
Herald added subscribers: kerbowa, rupprecht, guansong, yaxunl, mgorny, 
nhaehnle, jvesely.
Herald added a reviewer: alexshap.
Herald added a reviewer: jhenderson.
vzakhari requested review of this revision.
Herald added subscribers: llvm-commits, openmp-commits, cfe-commits, sstefan1, 
MaskRay.
Herald added a reviewer: jdoerfert.
Herald added projects: clang, LLVM.

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 // 
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.
- Perform thorough testing of //LLVM ELF// implementation on Windows downstream.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99360

Files:
  clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
  llvm/include/llvm/BinaryFormat/ELF.h
  llvm/tools/llvm-readobj/ELFDumper.cpp
  openmp/libomptarget/plugins/amdgpu/CMakeLists.txt
  openmp/libomptarget/plugins/common/elf_common/CMakeLists.txt
  openmp/libomptarget/plugins/common/elf_common/elf_common.cpp
  openmp/libomptarget/plugins/common/elf_common/elf_common.h
  openmp/libomptarget/plugins/common/elf_common/elf_constants.h
  openmp/libomptarget/plugins/common/elf_common/elf_light.cpp
  openmp/libomptarget/plugins/common/elf_common/elf_light.h
  openmp/libomptarget/plugins/remote/server/CMakeLists.txt

Index: openmp/libomptarget/plugins/remote/server/CMakeLists.txt
===
--- openmp/libomptarget/plugins/remote/server/CMakeLists.txt
+++ openmp/libomptarget/plugins/remote/server/CMakeLists.txt
@@ -10,7 +10,6 @@
 #
 ##===--===##
 
-include_directories(${LIBOMPTARGET_DEP_LIBELF_INCLUDE_DIRS})
 include_directories(${LIBOMPTARGET_SRC_DIR})
 include_directories(${LIBOMPTARGET_INCLUDE_DIR})
 include_directories(${GRPC_INCLUDE_DIR})
@@ -28,4 +27,4 @@
 grpc++
 protobuf
 ${OPENMP_PTHREAD_LIB}
-"-ldl" "-lomp" "-fopenmp" "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/../../exports" ${LIBOMPTARGET_DEP_LIBELF_LIBRARIES})
+"-ldl" "-lomp" "-fopenmp" "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/../../exports")
Index: openmp/libomptarget/plugins/common/elf_common/elf_light.h
===
--- /dev/null
+++ openmp/libomptarget/plugins/common/elf_common/elf_light.h
@@ -0,0 +1,122 @@
+//===-- elf_light.h - Basic ELF functionality ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Light ELF implementation provides basic ELF reading functionality.
+// It may be used in systems without libelf support, if the corresponding
+// LLVM ELF impleme