Support ARM_V4BX relocation

[yinma]

Summary

This patch implementation the handler for ARM_V4BX. This relocation is used by GNU runtime files and other armv4 applications.

[ruiu]

ELF/Arch/ARM.cpp

522 ↗

(On Diff #170224)

This is little weird that you can just ignore the relocation. Is this correct?

test/ELF/Inputs/v4bx.yaml

1 ↗

(On Diff #170224)

Assembly is preferred over YAML. I'm not sure if you can express this file in .s, but if you can, please do. If you can't, YAML is fine though.

test/ELF/v4bx.s

10 ↗

(On Diff #170224)

Remove the trailing blank line.

[efriedma]

ELF/Arch/ARM.cpp

522 ↗

(On Diff #170224)

Yes, it's correct to ignore it; it's just a marker to indicate there's a "bx rN" instruction at the given address. (It can be used to implement a special linker mode which rewrites ARMv4T inputs to ARMv4.)

test/ELF/Inputs/v4bx.yaml

1 ↗

(On Diff #170224)

LLVM currently never generates R_ARM_V4BX relocations.

[peter.smith]

This change looks fine to me. Just to check what CPU you are intending to run the output on. The LLD interworking, Thunk and PLT support assumes the presence of the BLX instruction which is only present on ARMv5 CPUs and above (LLD will warn if it doesn't detect at least one object compiled for ARMv5 or above).

Do you actually need ARMv4t support? I can work on supporting it but it probably wouldn't be optimal. At a minimum we'd need to insert Thunks for every interworking BL, including all Thumb calls to PLT sequences, prevent ARM and Thumb callers from sharing the a Thunk that starts in a different state, and have a new set of Thunks that avoid v5t instructions.

From memory, when the ABI was created the number of ARM only ARMv4 CPUs was tiny compared to those supporting ARM and Thumb, and all future CPUs would support Thumb so to simplify code generation it was assumed that BX reg could always be generated and a linker could translate it if needed.

From the ABI for the ARM Architecture:
R_ARM_V4BX records the location of an ARMv4t BX instruction. This enables a static linker to generate ARMv4 compatible images from ARMv4t objects containing only ARM code by converting the instruction to MOV PC, r, where r is the register used in the BX instruction. See [AAPCS] for details. The symbol is unused and may even be the NULL symbol (index 0).

[efriedma]

Do you actually need ARMv4t support?

No, we don't target anything older than ARMv5. This issue showed up because some of our tests use an ARMv4 sysroot.

[ruiu]

This may be a silly question, but what is ARMv4 sysroot, and why do you
need this patch for "ARMv4 sysroot"?

[efriedma]

By "ARMv4 sysroot", I mean a Linux sysroot that supports ARMv4T. So libc etc. are built with gcc, targeting ARMv4T, which generates these relocations.

[ruiu]

I'm confused. You wrote that you don't want to target anything older than ARMv5, and this patch fixes an issue for ARMv4T, which look inconsistent as long as ARMv4T < ARMv5.

[efriedma]

Some of the input object files are built for ARMv4T. The output executable is not; it's something like ARMv7, because some of the input object files are built for ARMv7.

[ruiu]

ELF/Arch/ARM.cpp

520–521 ↗

(On Diff #170224)

Please elaborate a bit more based on Eli's comment. I'd write

V4BX is just a marker to indicate there's a "bx rN" instruction at the given address. It can be used to implement a special linker mode which rewrites ARMv4T inputs to ARMv4. Since we support only ARMv4 input and not ARMv4 output, we can just ignore it.

[ruiu]

test/ELF/v4bx.s

1 ↗

(On Diff #172187)

I think you don't need two input files. This main test file can be written in YAML which contains _start symbol.

[ruiu]

test/ELF/v4bx.s

1 ↗

(On Diff #172187)

Hmm, do you still need this empty object file?

[yinma]

test/ELF/v4bx.s

1 ↗

(On Diff #172187)

I need lld to link an object containing a V4BX relocation.

[MaskRay]

test/ELF/v4bx.s

1 ↗

(On Diff #172187)

I think @ruiu means: as you don't use test/ELF/v4bx.s to produce an object file. You could mv test/ELF/Inputs/v4bx.yaml test/ELF/v4bx.yaml and put check lines in v4bx.yaml. v4bx.s is not necessary.

[ruiu]

test/ELF/v4bx.s

1 ↗

(On Diff #172187)

Yeah, right. That's what I meant.

[ruiu]

test/ELF/v4bx.yaml

2 ↗

(On Diff #174142)

nit: since now you have only one object file, %tv4x.o -> %t.o.

4–6 ↗

(On Diff #174142)

Does this really check anything? My understanding is that if ld.lld succeeds, then the test succeeded. So you don't really need readelf, no?

[yinma]

test/ELF/v4bx.yaml

4–6 ↗

(On Diff #174142)

We cannot assume ld.lld succeeds without checking the output. Without readelf checking, if lld just simply returns 0 without producing a correct .o, the test will still pass. Actually it is good practice to check something in the output for all unit tests.

[ruiu]

LGTM

test/ELF/v4bx.yaml

4–6 ↗

(On Diff #174142)

Please rename this file arm-v4bx.test to follow the local convention.

[MaskRay]

Contacted by the author to commit on behalf of him.

lld/trunk/ELF/Arch/ARM.cpp

write16le(Loc,
               0xf240 |                     // opcode
                   ((Val >> 1) & 0x0400) |  // i
                   ((Val >> 12) & 0x000f)); // imm4
     write16le(Loc + 2,
               (read16le(Loc + 2) & 0x8f00) | // opcode
                   ((Val << 4) & 0x7000) |    // imm3
                   (Val & 0x00ff));           // imm8
     break;
+  case R_ARM_V4BX:
+    // V4BX is just a marker to indicate there's a "bx rN" instruction at the
+    // given address. It can be used to implement a special linker mode which
+    // rewrites ARMv4T inputs to ARMv4. Since we support only ARMv4 input and
+    // not ARMv4 output, we can just ignore it.
+    break;
   default:
     error(getErrorLocation(Loc) + "unrecognized reloc " + Twine(Type));
   }
 }
 
 int64_t ARM::getImplicitAddend(const uint8_t *Buf, RelType Type) const {
   switch (Type) {
   default:

lld/trunk/test/ELF/v4bx.yaml

+# REQUIRES: arm
+# RUN: yaml2obj %s -o %t.o
+# RUN: ld.lld %t.o -o %t
+# RUN: llvm-readelf -sections %t | FileCheck %s
+
+# CHECK: .text
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_ARM
+  Flags:           [ EF_ARM_EABI_VER5 ]
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000004
+    Content:         1EFF2F01
+  - Name:            .rel.text
+    Type:            SHT_REL
+    Link:            .symtab
+    AddressAlign:    0x0000000000000004
+    Info:            .text
+    Relocations:
+      - Offset:          0x0000000000000000
+        Symbol:          ''
+        Type:            R_ARM_V4BX
+Symbols:
+  Global:
+    - Name:            _start
+      Section:         .text
+  Local:
+    - Name:            .text
+      Type:            STT_SECTION
+      Section:         .text
+DynamicSymbols: