[ELF] Set EF_ARM_EABI_VER5 for ARM

[mstorsjo]

Summary

Without this flag set, an aarch64 linux kernel won't try to load the executable (even if a 32 bit arm kernel will run the binary just fine).

[ruiu]

I don't know if this is the right thing to do, so I'd defer this to ARM developers.

[emaste]

Can you add a test case?

Without this flag set, an aarch64 kernel won't try to load the executable

Do you mean this is part of some ABI spec and so any aarch64 kernel should not load the executable, or that this is due to an implementation detail of (presumably) the Linux kernel?

ELF/Writer.cpp

1265–1268 ↗

(On Diff #71051)

All else being equal I'd keep these in alphabetical order.

[mstorsjo]

In D24471#540599, @emaste wrote:

Can you add a test case?

Ok, I'll have a look at it

Without this flag set, an aarch64 kernel won't try to load the executable

Do you mean this is part of some ABI spec and so any aarch64 kernel should not load the executable, or that this is due to an implementation detail of (presumably) the Linux kernel?

I think it's just an implementation detail. Since GNU ld has been setting it since forever (I assume), I guess the aarch64 kernel just hasn't bothered to add support for "legacy" binaries, only the ones commonly available.

[mstorsjo]

In D24471#540754, @mstorsjo wrote:

In D24471#540599, @emaste wrote:

Without this flag set, an aarch64 kernel won't try to load the executable

Do you mean this is part of some ABI spec and so any aarch64 kernel should not load the executable, or that this is due to an implementation detail of (presumably) the Linux kernel?

I think it's just an implementation detail. Since GNU ld has been setting it since forever (I assume), I guess the aarch64 kernel just hasn't bothered to add support for "legacy" binaries, only the ones commonly available.

http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/arch/arm64/include/asm/elf.h?h=v4.4.9&id=1a1a512b983108015ced1e7a7c7775cfeec42d8c#n176

So it seems like it doesn't matter much which EABI version is set, as long as any flag within EF_ARM_EABI_MASK is set.

[peter.smith]

Strictly speaking EF_ARM_EABI version It is a bit more than an implementation detail. The current value of 0 is more honest, however it may be less useful in practice.

The EF_ARM_EABI version is defined in http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf

EF_ARM_ABIMASK (0xFF000000) (current version is 0x05000000)

This masks an 8-bit version number, the version of the ABI to which this
ELF file conforms. This ABI is version 5. A value of 0 denotes unknown
conformance. |

I think the AArch64 kernel isn't being very helpful here as 0 does not necessarily mean < 1, it just means not known. However that isn't lld's problem.

To the question of what should the ABI version be set to:
The ABI version that is put in the executable is a combination of the ABI version of the input files and the changes made by the linker for the output file. For example some aspects of the ABI version are determined by the compiler/assembler, the linker can also use and will set certain flags such as EF_ARM_ABI_FLOAT_HARD that are only defined in a certain ABI version.

In an ideal world we would read the ABI version from the input objects, fault any incompatibilities that we can't handle, use no ARM ELF specific features from the highest ABI version read, and write this version out into the executable. We are quite a long way from doing this in the ARM port so far.

I think the current value of EF_ARM_ABI_VERSION = 0 is honest, as the ARM port is incomplete and we can't make any firm statements of conformance right now. In practice all input objects given to lld will be EF_ARM_ABI_VERSION 5 and we don't make use of any features that are incompatible with EF_ARM_ABI_VERSION 5 so I think it is acceptable to set the default to 5 as long as we leave an appropriate comment.

We aren't in a position to report hard or soft float. We don't support BuildAttributes to get this information from object files and we don't hard-code at build time like GNU ld (arm-linux-gnueabi-hf distribution). In any case EF_ARM_VFP_FLOAT and EF_ARM_SOFT_FLOAT are one of the few changes from ABI version 4 to version 5. The correct terms are EF_ARM_ABI_FLOAT_HARD and EF_ARM_ABI_FLOAT_SOFT in version 5.

[mstorsjo]

In D24471#540946, @peter.smith wrote:

Strictly speaking EF_ARM_EABI version It is a bit more than an implementation detail. The current value of 0 is more honest, however it may be less useful in practice.

What I meant was that the fact that the aarch64 kernel refuses to run binaries with a zero version is an implementation detail.

I think the current value of EF_ARM_ABI_VERSION = 0 is honest, as the ARM port is incomplete and we can't make any firm statements of conformance right now. In practice all input objects given to lld will be EF_ARM_ABI_VERSION 5 and we don't make use of any features that are incompatible with EF_ARM_ABI_VERSION 5 so I think it is acceptable to set the default to 5 as long as we leave an appropriate comment.

Thanks for the good writeup!

What would be a useful but concise enough comment here? "// We don't currently use any features incompatible with EF_ARM_EABI_VER5, but we don't have any firm guarantees of conformance"

[peter.smith]

Yes I think your suggested comment sums up the situation well.

[mstorsjo]

In D24471#540599, @emaste wrote:

Can you add a test case?

It seems to me that llvm-objdump doesn't print e_flags at all anywhere. Any suggestions on how to test it without that?

Also, is something similar to "Subtarget->isTargetAEABI() || Subtarget->isTargetGNUAEABI() || Subtarget->isTargetMuslAEABI()" available in ELF/Writer.cpp? In that case, we could selectively set the flag in those cases.

[rengolin]

In D24471#540946, @peter.smith wrote:

I think the current value of EF_ARM_ABI_VERSION = 0 is honest, as the ARM port is incomplete and we can't make any firm statements of conformance right now. In practice all input objects given to lld will be EF_ARM_ABI_VERSION 5 and we don't make use of any features that are incompatible with EF_ARM_ABI_VERSION 5 so I think it is acceptable to set the default to 5 as long as we leave an appropriate comment.

Well, I take it the kernel interprets this flag as being "what I intend to implement", not "what I honestly can implement".

In that case, 5 is the *correct* value, given the users' interpretation, not the ABI. We've been into that situation before with regards to architecture support and the approach was similar: state what we intend to do and fix the bugs as we find them.

Since we don't have a full conformance test-suite, exaggerating the statement and complete later is the only thing we can reliably do.

I agree the comment expresses it well and is necessary.

cheers,
--renato

[peter.smith]

I think llvm-readobj -file-headers is a good starting point. This should dump the ELF header, and if the flag isn't shown textually you should be able to find the binary value.

ELF/Writer.cpp doesn't have any similar for subtargets at the moment unfortunately.

[mstorsjo]

Updated the order of the if statements as requested (alphabetically) and added the requested comment. No tests added yet.

[mstorsjo]

Added a test using llvm-readobj.

[peter.smith]

The test looks ok despite llvm-readobj generic header output separating 0x5000000 into 0x1000000 and 0x4000000 which is confusing. The actual binary field is correct.

This looks good to me. I've accepted the revision although you might want to wait to see if Rui has any further comments.

[emaste]

This seems fine to me also.

One request if you're using the summary above as the commit message, please be explicit that you mean "an AArch64 Linux kernel." FreeBSD's kernel is "an AArch64 kernel" and does not have this issue (we don't currently run 32-bit arm binaries at all).

[ruiu]

LGTM.

Thanks a lot Peter, your explanation was extremely useful to understand the situation.

ELF/Writer.cpp

1267 ↗

(On Diff #71142)

Please also add a comment to describe why we need to set this value and can't leave as zero. Linux/AArch64 as of 2016 doesn't accept e_flags with zero.

[mstorsjo]

Amended the comment as requested.

[ruiu]

LGTM

[mstorsjo]

In D24471#541688, @ruiu wrote:

LGTM

Thanks - can you commit it?

[ruiu]

Submitted as r281394.

lld/trunk/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::writeHeader() {
   EHdr->e_entry = getEntryAddr<ELFT>();
   EHdr->e_shoff = SectionHeaderOff;
   EHdr->e_ehsize = sizeof(Elf_Ehdr);
   EHdr->e_phnum = Phdrs.size();
   EHdr->e_shentsize = sizeof(Elf_Shdr);
   EHdr->e_shnum = OutputSections.size() + 1;
   EHdr->e_shstrndx = Out<ELFT>::ShStrTab->SectionIndex;
 
-  if (Config->EMachine == EM_MIPS)
+  if (Config->EMachine == EM_ARM)
+    // We don't currently use any features incompatible with EF_ARM_EABI_VER5,
+    // but we don't have any firm guarantees of conformance. Linux AArch64
+    // kernels (as of 2016) require an EABI version to be set.
+    EHdr->e_flags = EF_ARM_EABI_VER5;
+  else if (Config->EMachine == EM_MIPS)
     EHdr->e_flags = getMipsEFlags<ELFT>();
 
   if (!Config->Relocatable) {
     EHdr->e_phoff = sizeof(Elf_Ehdr);
     EHdr->e_phentsize = sizeof(Elf_Phdr);
   }
 
   // Write the program header table.

lld/trunk/test/ELF/arm-eabi-version.s

+// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t.o
+// RUN: ld.lld -static %t.o -o %tout
+// RUN: llvm-readobj -file-headers %tout | FileCheck %s
+// REQUIRES: arm
+ .syntax unified
+ .text
+ .globl _start
+_start:
+ bx lr
+
+// CHECK:  Flags [
+// CHECK-NEXT:    0x1000000
+// CHECK-NEXT:    0x4000000
+// CHECK-NEXT:  ]