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

2421

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

if (Config->Pic)
     return ET_DYN;
   if (Config->Relocatable)
     return ET_REL;
   return ET_EXEC;
 }
 
 static uint8_t getAbiVersion() {
   // MIPS non-PIC executable gets ABI version 1.
-  if (Config->EMachine == EM_MIPS && getELFType() == ET_EXEC &&
+  if (Config->EMachine == EM_MIPS) {
+    if (getELFType() == ET_EXEC &&
         (Config->EFlags & (EF_MIPS_PIC | EF_MIPS_CPIC)) == EF_MIPS_CPIC)
       return 1;
     return 0;
   }
 
+  if (Config->EMachine == EM_AMDGPU) {
+    uint8_t Ver = ObjectFiles[0]->ABIVersion;
+    for (InputFile *File : makeArrayRef(ObjectFiles).slice(1))
+      if (File->ABIVersion != Ver)
+        error("incompatible ABI version: " + toString(File));
+    return Ver;
+  }
+
+  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 -o %t.so 2>&1 | FileCheck %s
+
+# CHECK: ld.lld: error: incompatible ABI version: {{.*}}-1.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