[elfabi] Move elfabi related code to InterfaceStub library

[haowei]

Summary

I would like to improve llvm-ifs tool on ELF object handling. Right now it uses yaml2obj which is not robust. This change moves ELFObjHandler and other elfabi code to a new InterfaceStub library so it can be used by both llvm-elfabi and llvm-ifs tool for reading and writing ELF objects. Previously I move it to TextAPI but it causes cyclic dependencies.

[phosek]

LGTM

[leonardchan]

LGTM for LLVM readability.

[haowei]

correct formating.

[MaskRay]

I would like to improve llvm-ifs tool on ELF object handling. Right now it uses yaml2obj which is not robust.

How is it not robust?

[MaskRay]

This change moves ELFObjHandler and other elfabi code to a new IFS library so it can be used by both llvm-elfabi and llvm-ifs tool for reading and writing ELF objects. Previously I move it to TextAPI but it causes cyclic dependencies.

I can't find any explanation why the new file hierarchy works better and why 'IFS' instead of a more specific name (.e.g InterfaceStub) is picked.

[haowei]

In D85678#2211863, @MaskRay wrote:

I would like to improve llvm-ifs tool on ELF object handling. Right now it uses yaml2obj which is not robust.

How is it not robust?

We contacted plotfi and learned that llvm-ifs does not work well in production when it is used for elf stub generation. We suggested to improve llvm-ifs tools and unify it with llvm-elfabi tool as they share many similarities. llvm-elfabi has its own elf handler, most of its code have not yet landed and we will try our best to make it to production grade. This CL is the first step to strip elf handler away from llvm-elfabi and put it into a standalone library so we can continue work on it and make it shared by both tools without breaking anything.

[haowei]

In D85678#2211865, @MaskRay wrote:

This change moves ELFObjHandler and other elfabi code to a new IFS library so it can be used by both llvm-elfabi and llvm-ifs tool for reading and writing ELF objects. Previously I move it to TextAPI but it causes cyclic dependencies.

I can't find any explanation why the new file hierarchy works better and why 'IFS' instead of a more specific name (.e.g InterfaceStub) is picked.

In my first try I put it in TextAPI library and it causes a dependency loop as Object library is depends on TextAPI. Otherwise I think TextAPI is a better place for these files.

I am OK with InterfaceStub name. It explains the purpose clearer.

[MaskRay]

In D85678#2211909, @haowei wrote:

In D85678#2211865, @MaskRay wrote:

This change moves ELFObjHandler and other elfabi code to a new IFS library so it can be used by both llvm-elfabi and llvm-ifs tool for reading and writing ELF objects. Previously I move it to TextAPI but it causes cyclic dependencies.

I can't find any explanation why the new file hierarchy works better and why 'IFS' instead of a more specific name (.e.g InterfaceStub) is picked.

In my first try I put it in TextAPI library and it causes a dependency loop as Object library is depends on TextAPI. Otherwise I think TextAPI is a better place for these files.

I think the sole reason that Object depends on TextAPI is due to D66159. Have you tried contacting the author about thinking whether we should continue organizing the two libraries this way?

I am OK with InterfaceStub name. It explains the purpose clearer.

If you look at existing library names. Abbreviation is not common, so IFS should probably be expanded.

[phosek]

I'd be fine with InterfaceStub as a name for the library.

llvm/lib/CMakeLists.txt

34 ↗

(On Diff #284851)

This should be moved up to preserve the alphabetical sorting.

[haowei]

In D85678#2211912, @MaskRay wrote:

In D85678#2211909, @haowei wrote:

In D85678#2211865, @MaskRay wrote:

This change moves ELFObjHandler and other elfabi code to a new IFS library so it can be used by both llvm-elfabi and llvm-ifs tool for reading and writing ELF objects. Previously I move it to TextAPI but it causes cyclic dependencies.

I can't find any explanation why the new file hierarchy works better and why 'IFS' instead of a more specific name (.e.g InterfaceStub) is picked.

In my first try I put it in TextAPI library and it causes a dependency loop as Object library is depends on TextAPI. Otherwise I think TextAPI is a better place for these files.

I think the sole reason that Object depends on TextAPI is due to D66159. Have you tried contacting the author about thinking whether we should continue organizing the two libraries this way?

I am OK with InterfaceStub name. It explains the purpose clearer.

If you look at existing library names. Abbreviation is not common, so IFS should probably be expanded.

I think the reason to put TapiFile.cpp to Object library is legit. I would probably do the same thing if I were the original author.

[plotfi]

In D85678#2211904, @haowei wrote:

In D85678#2211863, @MaskRay wrote:

I would like to improve llvm-ifs tool on ELF object handling. Right now it uses yaml2obj which is not robust.

How is it not robust?

We contacted plotfi and learned that llvm-ifs does not work well in production when it is used for elf stub generation. We suggested to improve llvm-ifs tools and unify it with llvm-elfabi tool as they share many similarities. llvm-elfabi has its own elf handler, most of its code have not yet landed and we will try our best to make it to production grade. This CL is the first step to strip elf handler away from llvm-elfabi and put it into a standalone library so we can continue work on it and make it shared by both tools without breaking anything.

I've spoken briefly with @phosek on this. I think I only mentioned that we used llvm-ifs for a production project internally, but that it has since been EOL'ed: this was unrelated to any of the technical decisions to use the yaml2obj lib at the time. For elf-stub generation I didn't have much trouble due to use of the yaml2obj lib though: generating the sections for ELF stubs is pretty straight forward, so yaml2obj worked fine for us.

The more tedious bits with Interface Stubs that I've run into are in things like clang driver and build system corner cases where llvm-ifs might expect some archive libFoo.a.ifs to be somewhere where it wasn't placed by the build system or the clang driver, also the clang stub file generation seems to sometimes not get the sizes of globals for a given symbol uniformly the same across the -cc1 invocations and its likely that the clang ASTMatcher side of this needs hardening. I ran into said issues when trying to build llvm with Clang Interface Stubs enabled.

Overall I am really happy to see an effort to unify elftapi, Darwin Tapi, and Interface Stubs though. My original plan was to use elftapi for the ELF generation, but it wasn't ready a year ago when I did this work but the yaml2obj stuff had been turned into a reusable library so thats really the reason I used yaml2obj back in Aug 2019.

[plotfi]

In D85678#2211933, @phosek wrote:

I'd be fine with InterfaceStub as a name for the library.

@phosek @haowei Are there any plans in the near term to unify any of the TBD work with InterfaceStubs/elftapi?

@ributzka @cishida

[phosek]

In D85678#2212326, @plotfi wrote:

In D85678#2211933, @phosek wrote:

I'd be fine with InterfaceStub as a name for the library.

@phosek @haowei Are there any plans in the near term to unify any of the TBD work with InterfaceStubs/elftapi?

@ributzka @cishida

We've talked about this some time ago and we can to the conclusion that it probably doesn't make sense to unify these for now because the overlap isn't as big as we originally thought it'd be. Specifically, .tbd defines a new object format that's understood by tools like the linker, but this is explicitly a non-goal for us. We see the textual representation only as an intermediate format that's used to generate the library stubs which from the perspective of other tools are just a regular ELF files.

[phosek]

LGTM

llvm/lib/InterfaceStub/LLVMBuild.txt

1 ↗

(On Diff #284948)

InterfaceStub

[haowei]

Are there any more comments?

llvm/lib/InterfaceStub/LLVMBuild.txt

1 ↗

(On Diff #284948)

Thanks for pointing out.

[thakis]

llvm/lib/InterfaceStub/CMakeLists.txt

7 ↗

(On Diff #285451)

Do you need this? Doesn't look like it?

[haowei]

llvm/lib/InterfaceStub/CMakeLists.txt

7 ↗

(On Diff #285451)

Thanks for pointing out. Addressed in D85936

llvm/include/llvm/TextAPI/ELF/ELFObjHandler.h

 //===- ELFObjHandler.h ------------------------------------------*- 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
 //
 //===-----------------------------------------------------------------------===/
 ///
 /// This supports reading and writing of elf dynamic shared objects.
 ///
 //===-----------------------------------------------------------------------===/
 
-#ifndef LLVM_TOOLS_ELFABI_ELFOBJHANDLER_H
-#define LLVM_TOOLS_ELFABI_ELFOBJHANDLER_H
+#ifndef LLVM_TEXTAPI_ELFOBJHANDLER_H
+#define LLVM_TEXTAPI_ELFOBJHANDLER_H
 
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/TextAPI/ELF/ELFStub.h"
 
 namespace llvm {
 
 class MemoryBuffer;
 
 namespace elfabi {
 
 /// Attempt to read a binary ELF file from a MemoryBuffer.
 Expected<std::unique_ptr<ELFStub>> readELFFile(MemoryBufferRef Buf);
 
 } // end namespace elfabi
 } // end namespace llvm
 
-#endif // LLVM_TOOLS_ELFABI_ELFOBJHANDLER_H
+#endif // LLVM_TEXTAPI_ELFOBJHANDLER_H

llvm/lib/TextAPI/CMakeLists.txt

 add_llvm_component_library(LLVMTextAPI
+  ELF/ELFObjHandler.cpp
   ELF/ELFStub.cpp
   ELF/TBEHandler.cpp
   MachO/Architecture.cpp
   MachO/ArchitectureSet.cpp
   MachO/InterfaceFile.cpp
   MachO/PackedVersion.cpp
   MachO/Platform.cpp
   MachO/Symbol.cpp
   MachO/Target.cpp
   MachO/TextStub.cpp
   MachO/TextStubCommon.cpp
 
   ADDITIONAL_HEADER_DIRS
   "${LLVM_MAIN_INCLUDE_DIR}/llvm/TextAPI"
-  "${LLVM_MAIN_INCLUDE_DIR}/llvm/TextAPI/Elf"
+  "${LLVM_MAIN_INCLUDE_DIR}/llvm/TextAPI/ELF"
   "${LLVM_MAIN_INCLUDE_DIR}/llvm/TextAPI/MachO"
 )

llvm/lib/TextAPI/ELF/ELFObjHandler.cpp

 //===- ELFObjHandler.cpp --------------------------------------------------===//
 //
 // 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
 //
 //===-----------------------------------------------------------------------===/
 
-#include "ELFObjHandler.h"
+#include "llvm/TextAPI/ELF/ELFObjHandler.h"
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/TextAPI/ELF/ELFStub.h"
 
if (!FoundDynStrSz) {
     return createError(
         "Couldn't determine dynamic string table size (no DT_STRSZ entry)");
   }
   if (!FoundDynSym) {
     return createError(
         "Couldn't locate dynamic symbol table (no DT_SYMTAB entry)");
   }
   if (Dyn.SONameOffset.hasValue() && *Dyn.SONameOffset >= Dyn.StrSize) {
-    return createStringError(
-        object_error::parse_failed,
+    return createStringError(object_error::parse_failed,
                              "DT_SONAME string offset (0x%016" PRIx64
                              ") outside of dynamic string table",
                              *Dyn.SONameOffset);
   }
   for (uint64_t Offset : Dyn.NeededLibNames) {
     if (Offset >= Dyn.StrSize) {
-      return createStringError(
-          object_error::parse_failed,
+      return createStringError(object_error::parse_failed,
                                "DT_NEEDED string offset (0x%016" PRIx64
                                ") outside of dynamic string table",
                                Offset);
     }
   }
 
   return Error::success();
 }
 
 /// This function finds the number of dynamic symbols using a GNU hash table.
 ///

 /// maps it to an ELFSymbolType enum.
 /// Currently, STT_NOTYPE, STT_OBJECT, STT_FUNC, and STT_TLS are supported.
 /// Other symbol types are mapped to ELFSymbolType::Unknown.
 ///
 /// @param Info Binary symbol st_info to extract symbol type from.
 static ELFSymbolType convertInfoToType(uint8_t Info) {
   Info = Info & 0xf;
   switch (Info) {
-    case ELF::STT_NOTYPE:
-      return ELFSymbolType::NoType;
-    case ELF::STT_OBJECT:
-      return ELFSymbolType::Object;
-    case ELF::STT_FUNC:
-      return ELFSymbolType::Func;
-    case ELF::STT_TLS:
-      return ELFSymbolType::TLS;
-    default:
-      return ELFSymbolType::Unknown;
   }
 }
 
 /// This function creates an ELFSymbol and populates all members using
 /// information from a binary ELFT::Sym.
 ///
 /// @param SymName The desired name of the ELFSymbol.
 /// @param RawSym ELFT::Sym to extract symbol information from.

 /// This function populates an ELFStub with symbols using information read
 /// from an ELF binary.
 ///
 /// @param TargetStub ELFStub to add symbols to.
 /// @param DynSym Range of dynamic symbols to add to TargetStub.
 /// @param DynStr StringRef to the dynamic string table.
 template <class ELFT>
 static Error populateSymbols(ELFStub &TargetStub,
                              const typename ELFT::SymRange DynSym,
                              StringRef DynStr) {
   // Skips the first symbol since it's the NULL symbol.
   for (auto RawSym : DynSym.drop_front(1)) {
     // If a symbol does not have global or weak binding, ignore it.
     uint8_t Binding = RawSym.getBinding();
     if (!(Binding == STB_GLOBAL || Binding == STB_WEAK))
       continue;
     // If a symbol doesn't have default or protected visibility, ignore it.
     uint8_t Visibility = RawSym.getVisibility();
if (!PHdrs) {
     return PHdrs.takeError();
   }
 
   // Collect relevant .dynamic entries.
   DynamicEntries DynEnt;
   if (Error Err = populateDynamic<ELFT>(DynEnt, *DynTable))
     return std::move(Err);
 
-    // Get pointer to in-memory location of .dynstr section.
   Expected<const uint8_t *> DynStrPtr =
       ElfFile->toMappedAddr(DynEnt.StrTabAddr);
   if (!DynStrPtr)
     return appendToError(DynStrPtr.takeError(),
                          "when locating .dynstr section contents");
 
   StringRef DynStr(reinterpret_cast<const char *>(DynStrPtr.get()),
                    DynEnt.StrSize);
if (!SymCount)
     return SymCount.takeError();
   if (*SymCount > 0) {
     // Get pointer to in-memory location of .dynsym section.
     Expected<const uint8_t *> DynSymPtr =
         ElfFile->toMappedAddr(DynEnt.DynSymAddr);
     if (!DynSymPtr)
       return appendToError(DynSymPtr.takeError(),
                            "when locating .dynsym section contents");
-    Elf_Sym_Range DynSyms =
-        ArrayRef<Elf_Sym>(reinterpret_cast<const Elf_Sym *>(*DynSymPtr),
-                          *SymCount);
+    Elf_Sym_Range DynSyms = ArrayRef<Elf_Sym>(
+        reinterpret_cast<const Elf_Sym *>(*DynSymPtr), *SymCount);
     Error SymReadError = populateSymbols<ELFT>(*DestStub, DynSyms, DynStr);
     if (SymReadError)
       return appendToError(std::move(SymReadError),
                            "when reading dynamic symbols");
   }
 
   return std::move(DestStub);
 }

llvm/tools/llvm-elfabi/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Object
   Support
   TextAPI
   )
 
 add_llvm_tool(llvm-elfabi
-  ELFObjHandler.cpp
   ErrorCollector.cpp
   llvm-elfabi.cpp
   )

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

 //===- llvm-elfabi.cpp ----------------------------------------------------===//
 //
 // 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
 //
 //===-----------------------------------------------------------------------===/
 
-#include "ELFObjHandler.h"
 #include "ErrorCollector.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
-#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/TextAPI/ELF/ELFObjHandler.h"
 #include "llvm/TextAPI/ELF/TBEHandler.h"
 #include <string>
 
 namespace llvm {
 namespace elfabi {
 
 enum class FileFormat {
   TBE,