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

2410

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/SymbolTable.cpp

 
 // All input object files must be for the same architecture
 // (e.g. it does not make sense to link x86 object files with
 // MIPS object files.) This function checks for that error.
 static bool isCompatible(InputFile *F) {
   if (!F->isElf() && !isa<BitcodeFile>(F))
     return true;
 
-  if (F->EKind == Config->EKind && F->EMachine == Config->EMachine) {
+  if (F->EKind == Config->EKind && F->EMachine == Config->EMachine &&
+      F->ABIVersion == Config->ABIVersion) {
     if (Config->EMachine != EM_MIPS)
       return true;
     if (isMipsN32Abi(F) == Config->MipsN32Abi)
       return true;
   }
 
   if (!Config->Emulation.empty())
     error(toString(F) + " is incompatible with " + Config->Emulation);

ELF/Writer.cpp

static uint16_t getELFType() {
   if (Config->Pic)
     return ET_DYN;
   if (Config->Relocatable)
     return ET_REL;
   return ET_EXEC;
 }
 
 static uint8_t getAbiVersion() {
+  if (Config->ABIVersion)
+    return Config->ABIVersion;
+
   // MIPS non-PIC executable gets ABI version 1.
   if (Config->EMachine == EM_MIPS && getELFType() == ET_EXEC &&
       (Config->EFlags & (EF_MIPS_PIC | EF_MIPS_CPIC)) == EF_MIPS_CPIC)
     return 1;
   return 0;
 }
 
 template <class ELFT> void Writer<ELFT>::writeHeader() {
   uint8_t *Buf = Buffer->getBufferStart();
 
   // For executable segments, the trap instructions are written before writing
   // the header. Setting Elf header bytes to zero ensures that any unused bytes
   // in header are zero-cleared, instead of having trap instructions.
   memset(Buf, 0, sizeof(Elf_Ehdr));
   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_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();
   EHdr->e_shentsize = sizeof(Elf_Shdr);

test/ELF/amdgpu-abi-version-err.s

+# REQUIRES: amdgpu
+# RUN: llvm-mc -triple amdgcn-amd-amdhsa -mcpu=gfx900 -filetype=obj %s -o %t-0.o
+# RUN: llvm-mc -triple amdgcn-amd-amdhsa -mcpu=gfx900 -mattr=-code-object-v3 -filetype=obj %s -o %t-1.o
+# RUN: not ld.lld -shared %t-0.o %t-1 -o %t.so
+
+# CHECK: ld.lld: error: {{.*}}-1.o is incompatible with {{.*}}-0.o
+
+.text
+  s_nop 0x0
+  s_endpgm

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