LLD: Preserve ABI version during linking ELF

[ruiu]

With this patch, lld simply selects the last object file passed via the command line and sets the ABI version field to the value read from the object file. But is this what you actually want? What if all input files except the last one have ABI version 1, and the last one has ABI version 0?

[ruiu]

I quickly hacked up this patch: https://gist.github.com/rui314/f3cd508b6cbbde763fc13bc6190d7505 Does this look good?

ELF/Writer.cpp

2407

getAbiVersion is intended to be the single place to compute an ABI version number to write to to an output file. You should always call it under any circumstances.

[kzhuravl]

Make sure ABI versions across all object files are the same.

[kzhuravl]

In D58026#1393777, @ruiu wrote:

I quickly hacked up this patch: https://gist.github.com/rui314/f3cd508b6cbbde763fc13bc6190d7505 Does this look good?

The patch you linked will work. Alternatively, can you take a look at the updated patch I just posted?

Is there any reason to not implement this for all targets? In your link you specifically check for EM_AMDGPU...

Also it seems like OSABI is also suffering from the problem you described in your first comment. Is this intentional?

Thanks

[ruiu]

The interpretation of ABI version of an ELF object file depends on the target machine type. For example, we can think of a target that allows a mix of version X and version Y objects let a linker to create a shim between cross-version function call. At least most ABIs don't define anything about the ABI version, so it is perhaps most natural to ignore the field at all, unless you are linking for a specific target in which the ABI version has a special meaning. Therefore, I'd think code that works only for AMDGPU target is better.

[kzhuravl]

In D58026#1393837, @ruiu wrote:

The interpretation of ABI version of an ELF object file depends on the target machine type. For example, we can think of a target that allows a mix of version X and version Y objects let a linker to create a shim between cross-version function call. At least most ABIs don't define anything about the ABI version, so it is perhaps most natural to ignore the field at all, unless you are linking for a specific target in which the ABI version has a special meaning. Therefore, I'd think code that works only for AMDGPU target is better.

Ok, sounds fair. Do you want me to incorporate your patch into this review?

[ruiu]

Ok, sounds fair. Do you want me to incorporate your patch into this review?

If you find that patch OK, yes.

[kzhuravl]

Address review feedback.

[ruiu]

LGTM

ELF/Config.h

 };
 
 // This struct contains the global configuration for the linker.
 // Most fields are direct mapping from the command line options
 // and such fields have the same name as the corresponding options.
 // Most fields are initialized by the driver.
 struct Configuration {
   uint8_t OSABI = 0;
+  uint8_t ABIVersion = 0;
   llvm::CachePruningPolicy ThinLTOCachePolicy;
   llvm::StringMap<uint64_t> SectionStartMap;
   llvm::StringRef Chroot;
   llvm::StringRef DynamicLinker;
   llvm::StringRef DwoDir;
   llvm::StringRef Entry;
   llvm::StringRef Emulation;
   llvm::StringRef Fini;

ELF/Driver.cpp

if (Config->EKind != ELFNoneKind)
     return;
 
   for (InputFile *F : Files) {
     if (F->EKind == ELFNoneKind)
       continue;
     Config->EKind = F->EKind;
     Config->EMachine = F->EMachine;
     Config->OSABI = F->OSABI;
+    Config->ABIVersion = F->ABIVersion;
     Config->MipsN32Abi = Config->EMachine == EM_MIPS && isMipsN32Abi(F);
     return;
   }
   error("target emulation unknown: -m or at least one .o file required");
 }
 
 // Parse -z max-page-size=<value>. The default value is defined by
 // each target.

ELF/InputFiles.h

public:
   // string for creating error messages.
   std::string ArchiveName;
 
   // If this is an architecture-specific file, the following members
   // have ELF type (i.e. ELF{32,64}{LE,BE}) and target machine type.
   ELFKind EKind = ELFNoneKind;
   uint16_t EMachine = llvm::ELF::EM_NONE;
   uint8_t OSABI = 0;
+  uint8_t ABIVersion = 0;
 
   // Cache for toString(). Only toString() should use this member.
   mutable std::string ToStringCache;
 
   std::string getSrcMsg(const Symbol &Sym, InputSectionBase &Sec,
                         uint64_t Offset);
 
   // True if this is an argument for --just-symbols. Usually false.

ELF/InputFiles.cpp

 ELFFileBase<ELFT>::ELFFileBase(Kind K, MemoryBufferRef MB) : InputFile(K, MB) {
   if (ELFT::TargetEndianness == support::little)
     EKind = ELFT::Is64Bits ? ELF64LEKind : ELF32LEKind;
   else
     EKind = ELFT::Is64Bits ? ELF64BEKind : ELF32BEKind;
 
   EMachine = getObj().getHeader()->e_machine;
   OSABI = getObj().getHeader()->e_ident[llvm::ELF::EI_OSABI];
+  ABIVersion = getObj().getHeader()->e_ident[llvm::ELF::EI_ABIVERSION];
 }
 
 template <class ELFT>
 typename ELFT::SymRange ELFFileBase<ELFT>::getGlobalELFSyms() {
   return makeArrayRef(ELFSyms.begin() + FirstGlobal, ELFSyms.end());
 }
 
 template <class ELFT>

ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::writeHeader() {
   memcpy(Buf, "\177ELF", 4);
 
   // Write the ELF header.
   auto *EHdr = reinterpret_cast<Elf_Ehdr *>(Buf);
   EHdr->e_ident[EI_CLASS] = Config->Is64 ? ELFCLASS64 : ELFCLASS32;
   EHdr->e_ident[EI_DATA] = Config->IsLE ? ELFDATA2LSB : ELFDATA2MSB;
   EHdr->e_ident[EI_VERSION] = EV_CURRENT;
   EHdr->e_ident[EI_OSABI] = Config->OSABI;
-  EHdr->e_ident[EI_ABIVERSION] = getAbiVersion();
+  EHdr->e_ident[EI_ABIVERSION] =
+      Config->ABIVersion == 0 ? getAbiVersion() : Config->ABIVersion;
   EHdr->e_type = getELFType();
   EHdr->e_machine = Config->EMachine;
   EHdr->e_version = EV_CURRENT;
   EHdr->e_entry = getEntryAddr();
   EHdr->e_shoff = SectionHeaderOff;
   EHdr->e_flags = Config->EFlags;
   EHdr->e_ehsize = sizeof(Elf_Ehdr);
   EHdr->e_phnum = Phdrs.size();

test/ELF/amdgpu-abi-version.s

+# REQUIRES: amdgpu
+# RUN: llvm-mc -triple amdgcn-amd-amdhsa -mcpu=gfx900 -filetype=obj %s -o %t.o
+# RUN: ld.lld -shared %t.o -o %t.so
+# RUN: llvm-readobj -file-headers %t.so | FileCheck %s
+
+# CHECK: OS/ABI: AMDGPU_HSA (0x40)
+# CHECK: ABIVersion: 1
+
+.text
+  s_nop 0x0
+  s_endpgm
 No newline at end of file