[LLD][ELF][ARM] Fix support for SBREL type relocations

[tamas.petz]

Summary

With this patch lld recognizes ARM SBREL relocations.
R_ARM*_MOVW_BREL relocations are not tested because they are not used.

[psmith]

Thanks for the patch, I'll aim to take a look early next week, hopefully Monday.

[psmith]

The code changes look good to me. I'm unfortunately going to have be a pain and ask that you split the additional tests not related to the new relocation support into a separate change, something like "[LLD][ELF][ARM] add missing test cases for ... " that I'd be happy to approve. This helps keep tracking down failures via bisection easier.

I've added a couple more frequent reviewers to see if they have any additional comments.

For the benefit of other reviewers, reference to ELF for the ARM Architecture for definitions of the relocations https://static.docs.arm.com/ihi0044/g/aaelf32.pdf

lld/test/ELF/arm-mov-relocs.s

32

Another convention is where the address isn't significant to the line and follows on from the previous instruction is to only put the address on the first instruction. I think it is also the trend to remove the early indentation. For example:

// CHECK: 12000: movw  r0, #0
// CHECK:              movw  r1, #4
// CHECK:              movw  r2, #12
// CHECK:              movw  r3, #65532
// CHECK:              movw  r4, #0

[MaskRay]

In D74604#1879493, @psmith wrote:

The code changes look good to me. I'm unfortunately going to have be a pain and ask that you split the additional tests not related to the new relocation support into a separate change, something like "[LLD][ELF][ARM] add missing test cases for ... " that I'd be happy to approve. This helps keep tracking down failures via bisection easier.

I've added a couple more frequent reviewers to see if they have any additional comments.

For the benefit of other reviewers, reference to ELF for the ARM Architecture for definitions of the relocations https://static.docs.arm.com/ihi0044/g/aaelf32.pdf

Peter has mentioned most points I wanted to say. If we can think of a good category name, we can place new tests in arm-reloc-$category.s.

Use CHECK-LABEL: instead of CHECK: for section .R_* (see ppc32-reloc-addr.s). They can improve FileCheck diagnostics when something changes.

Don't just write // 0x20000 - . = 56832. It is unclear what 0x20000 refers to. In riscv-tls-*, you can see I write ## rv32: &.got[0] - . = 0x2214 - . = 4096*1+112

lld/test/ELF/arm-mov-relocs.s

23

Is the overalignment (8) significant?

[tamas.petz]

lld/test/ELF/arm-mov-relocs.s

23

Technically it is not.
It is only for readability, the different sections have their start addresses increasing by 0x100.
When I tried to follow the existing pattern where all addresses are written I found this to be useful.
It was easier to add new tests as it will happen in an upcoming patch.

[peter.smith]

Thanks for the update. Apart from the extra / needed in some of the comment strings this looks good to me. Happy for you to add those prior to commit.

lld/test/ELF/arm-mov-relocs.s

9–17

The comment doesn't involve lit commands so these should start with /// for example:

///  * R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS
///  * R_ARM_MOVW_PREL_NC and R_ARM_MOVT_PREL
///  * R_ARM_MOVW_BREL_NC and R_ARM_MOVT_BREL
///
///  * R_ARM_THM_MOVW_BREL_NC and R_ARM_THM_MOVT_BREL

50

This type of explanatory comment would need the extra /
/// :upper16:label3 + 4 = :upper16:0x30000 = 3

[tamas.petz]

Thank you, Peter.
Yes, add the missing /-es and commit this change on my behalf, please.
I will add the missing test cases shortly after.

lld/ELF/Arch/ARM.cpp

case R_ARM_BASE_PREL:
     // platforms.
     return R_GOTONLY_PC;
   case R_ARM_MOVW_PREL_NC:
   case R_ARM_MOVT_PREL:
   case R_ARM_REL32:
   case R_ARM_THM_MOVW_PREL_NC:
   case R_ARM_THM_MOVT_PREL:
     return R_PC;
+  case R_ARM_MOVW_BREL_NC:
+  case R_ARM_MOVW_BREL:
+  case R_ARM_MOVT_BREL:
+  case R_ARM_THM_MOVW_BREL_NC:
+  case R_ARM_THM_MOVW_BREL:
+  case R_ARM_THM_MOVT_BREL:
+    return R_ARM_SBREL;
   case R_ARM_NONE:
     return R_NONE;
   case R_ARM_TLS_LE32:
     return R_TLS;
   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
case R_ARM_THM_JUMP24:
     write16le(loc + 2,
               (read16le(loc + 2) & 0xd000) |                  // opcode
                   (((~(val >> 10)) ^ (val >> 11)) & 0x2000) | // J1
                   (((~(val >> 11)) ^ (val >> 13)) & 0x0800) | // J2
                   ((val >> 1) & 0x07ff));                     // imm11
     break;
   case R_ARM_MOVW_ABS_NC:
   case R_ARM_MOVW_PREL_NC:
+  case R_ARM_MOVW_BREL_NC:
     write32le(loc, (read32le(loc) & ~0x000f0fff) | ((val & 0xf000) << 4) |
                        (val & 0x0fff));
     break;
   case R_ARM_MOVT_ABS:
   case R_ARM_MOVT_PREL:
+  case R_ARM_MOVT_BREL:
     write32le(loc, (read32le(loc) & ~0x000f0fff) |
                        (((val >> 16) & 0xf000) << 4) | ((val >> 16) & 0xfff));
     break;
   case R_ARM_THM_MOVT_ABS:
   case R_ARM_THM_MOVT_PREL:
+  case R_ARM_THM_MOVT_BREL:
     // Encoding T1: A = imm4:i:imm3:imm8
     write16le(loc,
               0xf2c0 |                     // opcode
                   ((val >> 17) & 0x0400) | // i
                   ((val >> 28) & 0x000f)); // imm4
     write16le(loc + 2,
               (read16le(loc + 2) & 0x8f00) | // opcode
                   ((val >> 12) & 0x7000) |   // imm3
                   ((val >> 16) & 0x00ff));   // imm8
     break;
   case R_ARM_THM_MOVW_ABS_NC:
   case R_ARM_THM_MOVW_PREL_NC:
+  case R_ARM_THM_MOVW_BREL_NC:
     // Encoding T3: A = imm4:i:imm3:imm8
     write16le(loc,
               0xf240 |                     // opcode
                   ((val >> 1) & 0x0400) |  // i
                   ((val >> 12) & 0x000f)); // imm4
     write16le(loc + 2,
               (read16le(loc + 2) & 0x8f00) | // opcode
                   ((val << 4) & 0x7000) |    // imm3
return SignExtend64<24>(((hi & 0x0400) << 14) |                    // S
                             ((hi & 0x003ff) << 12) |                   // imm0
                             ((lo & 0x007ff) << 1)); // imm11:0
   }
   // ELF for the ARM Architecture 4.6.1.1 the implicit addend for MOVW and
   // MOVT is in the range -32768 <= A < 32768
   case R_ARM_MOVW_ABS_NC:
   case R_ARM_MOVT_ABS:
   case R_ARM_MOVW_PREL_NC:
-  case R_ARM_MOVT_PREL: {
+  case R_ARM_MOVT_PREL:
+  case R_ARM_MOVW_BREL_NC:
+  case R_ARM_MOVT_BREL: {
     uint64_t val = read32le(buf) & 0x000f0fff;
     return SignExtend64<16>(((val & 0x000f0000) >> 4) | (val & 0x00fff));
   }
   case R_ARM_THM_MOVW_ABS_NC:
   case R_ARM_THM_MOVT_ABS:
   case R_ARM_THM_MOVW_PREL_NC:
-  case R_ARM_THM_MOVT_PREL: {
+  case R_ARM_THM_MOVT_PREL:
+  case R_ARM_THM_MOVW_BREL_NC:
+  case R_ARM_THM_MOVT_BREL: {
     // Encoding T3: A = imm4:i:imm3:imm8
     uint16_t hi = read16le(buf);
     uint16_t lo = read16le(buf + 2);
     return SignExtend64<16>(((hi & 0x000f) << 12) | // imm4
                             ((hi & 0x0400) << 1) |  // i
                             ((lo & 0x7000) >> 4) |  // imm3
                             (lo & 0x00ff));         // imm8
   }

lld/test/ELF/arm-mov-relocs.s

 // REQUIRES: arm
 // RUN: llvm-mc -filetype=obj -triple=armv7a-unknown-linux-gnueabi %s -o %t
 // RUN: ld.lld %t -o %t2
 // RUN: llvm-objdump -d %t2 -triple=armv7a-unknown-linux-gnueabi --no-show-raw-insn | FileCheck %s
 // RUN: llvm-mc -filetype=obj -triple=thumbv7a-unknown-linux-gnueabi %s -o %t3
 // RUN: ld.lld %t3 -o %t4
 // RUN: llvm-objdump -d %t4 -triple=thumbv7a-unknown-linux-gnueabi --no-show-raw-insn | FileCheck %s
 
-// Test the R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS relocations as well as
-// the R_ARM_THM_MOVW_ABS_NC and R_ARM_THM_MOVT_ABS relocations.
+// Test the following relocations pairs:
+//  * R_ARM_MOVW_ABS_NC and R_ARM_MOVT_ABS
+//  * R_ARM_MOVW_PREL_NC and R_ARM_MOVT_PREL
+//  * R_ARM_MOVW_BREL_NC and R_ARM_MOVT_BREL
+//
+//  * R_ARM_THM_MOVW_ABS_NC and R_ARM_THM_MOVT_ABS
+//  * R_ARM_THM_MOVW_PREL_NC and R_ARM_THM_MOVT_PREL
+//  * R_ARM_THM_MOVW_BREL_NC and R_ARM_THM_MOVT_BREL
+
  .syntax unified
  .globl _start
+ .align 12
 _start:
  .section .R_ARM_MOVW_ABS_NC, "ax",%progbits
+ .align 8
  movw r0, :lower16:label
  movw r1, :lower16:label1
  movw r2, :lower16:label2 + 4
  movw r3, :lower16:label3
  movw r4, :lower16:label3 + 4
 // CHECK: Disassembly of section .R_ARM_MOVW_ABS_NC
 // CHECK-EMPTY:
-// CHECK: movw  r0, #0
-// CHECK: movw  r1, #4
-// CHECK: movw  r2, #12
-// CHECK: movw  r3, #65532
-// CHECK: movw  r4, #0
+// CHECK: 12000:       movw r0, #0
+// CHECK: 12004:       movw r1, #4
+// CHECK: 12008:       movw r2, #12
+// CHECK: 1200c:       movw r3, #65532
+// CHECK: 12010:       movw r4, #0
+
  .section .R_ARM_MOVT_ABS, "ax",%progbits
+ .align 8
  movt r0, :upper16:label
  movt r1, :upper16:label1
  movt r2, :upper16:label2 + 4
  movt r3, :upper16:label3
  movt r4, :upper16:label3 + 4
 // CHECK: Disassembly of section .R_ARM_MOVT_ABS
 // CHECK-EMPTY:
-// CHECK: movt  r0, #2
-// CHECK: movt  r1, #2
-// CHECK: movt  r2, #2
-// CHECK: movt  r3, #2
-// CHECK: movt  r4, #3
+// CHECK: 12100:       movt r0, #2
+// CHECK: 12104:       movt r1, #2
+// CHECK: 12108:       movt r2, #2
+// CHECK: 1210c:       movt r3, #2
+// CHECK: 12110:       movt r4, #3
 
 .section .R_ARM_MOVW_PREL_NC, "ax",%progbits
+.align 8
  movw r0, :lower16:label - .
  movw r1, :lower16:label1 - .
  movw r2, :lower16:label2 + 4 - .
  movw r3, :lower16:label3 - .
- movw r4, :lower16:label3 + 0x103c - .
-// 0x20000 - . = 61188
-// CHECK: 110fc:       movw    r0, #61188
-// 0x20004 - . = 61188
-// CHECK: 11100:       movw    r1, #61188
-// 0x20008 - . + 4 = 61192
-// CHECK: 11104:       movw    r2, #61192
-// 0x2fffc - . = 61172
-// CHECK: 11108:       movw    r3, #61172
-// 0x2fffc - . +0x103c = 65324
-// CHECK: 1110c:       movw    r4, #65324
+ movw r4, :lower16:label3 + 0x2214 - .
+// CHECK: Disassembly of section .R_ARM_MOVW_PREL_NC
+// CHECK-EMPTY:
+// 0x20000 - . = 56832
+// CHECK: 12200:       movw    r0, #56832
+// 0x20004 - . = 56832
+// CHECK: 12204:       movw    r1, #56832
+// 0x20008 - . + 4 = 56836
+// CHECK: 12208:       movw    r2, #56836
+// 0x2fffc - . = 56816
+// CHECK: 1220c:       movw    r3, #56816
+// 0x2fffc - . + 0x2214 = 0x20000
+// CHECK: 12210:       movw    r4, #0
 
 .section .R_ARM_MOVT_PREL, "ax",%progbits
+.align 8
  movt r0, :upper16:label - .
  movt r1, :upper16:label1 - .
  movt r2, :upper16:label2 + 0x4 - .
  movt r3, :upper16:label3 - .
- movt r4, :upper16:label3 + 0x1120 - .
-// 0x20000 - . = :upper16:0xeef0  = 0
-// CHECK: 11110:       movt    r0, #0
-// 0x20004 - . = :upper16:0xeef0 = 0
-// CHECK: 11114:       movt    r1, #0
-// 0x20008 - . + 4 = :upper16:0xeef4 = 0
-// CHECK: 11118:       movt    r2, #0
-// 0x2fffc - . = :upper16:0x1eee0 = 1
-// CHECK: 1111c:       movt    r3, #1
-// 0x2fffc - . + 0x1120 = :upper16:0x20000 = 2
-// CHECK: 11120:       movt    r4, #1
+ movt r4, :upper16:label3 + 0x2314 - .
+// CHECK: Disassembly of section .R_ARM_MOVT_PREL
+// CHECK-EMPTY:
+// 0x20000 - . = :upper16:0xdd00  = 0
+// CHECK: 12300:       movt    r0, #0
+// 0x20004 - . = :upper16:0xdd00 = 0
+// CHECK: 12304:       movt    r1, #0
+// 0x20008 - . + 4 = :upper16:0xdd04 = 0
+// CHECK: 12308:       movt    r2, #0
+// 0x2fffc - . = :upper16:0x1dcf0 = 1
+// CHECK: 1230c:       movt    r3, #1
+// 0x2fffc - . + 0x2314 = :upper16:0x20000 = 2
+// CHECK: 12310:       movt    r4, #2
+
+.section .R_ARM_MOVW_BREL_NC, "ax",%progbits
+.align 8
+ movw r0, :lower16:label(sbrel)
+ movw r1, :lower16:label1(sbrel)
+ movw r2, :lower16:label2(sbrel)
+ movw r3, :lower16:label3(sbrel)
+ movw r4, :lower16:label3.4(sbrel)
+// CHECK: Disassembly of section .R_ARM_MOVW_BREL_NC
+// CHECK-EMPTY:
+// SB = 0x20000
+// :lower16:(0x20000 - SB) = 0
+// CHECK: 12400:       movw    r0, #0
+// :lower16:(0x20004 - SB) = 4
+// CHECK: 12404:       movw    r1, #4
+// :lower16:(0x20008 - SB) = 8
+// CHECK: 12408:       movw    r2, #8
+// :lower16:(0x2fffc - SB) = 0xfffc
+// CHECK: 1240c:       movw    r3, #65532
+// :lower16:(0x2fffc + 4 - SB) = 0x0
+// CHECK: 12410:       movw    r4, #0
+
+.section .R_ARM_MOVT_BREL, "ax",%progbits
+.align 8
+ movt r0, :upper16:label(sbrel)
+ movt r1, :upper16:label1(sbrel)
+ movt r2, :upper16:label2(sbrel)
+ movt r3, :upper16:label3(sbrel)
+ movt r4, :upper16:label3.4(sbrel)
+// CHECK: Disassembly of section .R_ARM_MOVT_BREL
+// CHECK-EMPTY:
+// SB = 0x20000
+// :upper16:(0x20000 - SB) = 0
+// CHECK: 12500:       movt    r0, #0
+// :upper16:(0x20004 - SB) = 0
+// CHECK: 12504:       movt    r1, #0
+// :upper16:(0x20008 - SB) = 0
+// CHECK: 12508:       movt    r2, #0
+// :upper16:(0x2fffc - SB) = 0
+// CHECK: 1250c:       movt    r3, #0
+// :upper16:(0x2fffc + 4 - SB) = 1
+// CHECK: 12510:       movt    r4, #1
+
+.section .R_ARM_THM_MOVW_ABS_NC, "ax",%progbits
+.align 8
+ movw r0, :lower16:label
+ movw r1, :lower16:label1
+ movw r2, :lower16:label2 + 4
+ movw r3, :lower16:label3
+ movw r4, :lower16:label3 + 4
+// CHECK-THM: Disassembly of section .R_ARM_THM_MOVW_ABS_NC
+// CHECK-THM-EMPTY:
+// CHECK-THM: 12600:       movw r0, #0
+// CHECK-THM: 12604:       movw r1, #4
+// CHECK-THM: 12608:       movw r2, #12
+// CHECK-THM: 1260c:       movw r3, #65532
+// CHECK-THM: 12610:       movw r4, #0
+
+.section .R_ARM_THM_MOVT_ABS, "ax",%progbits
+.align 8
+ movt r0, :upper16:label
+ movt r1, :upper16:label1
+ movt r2, :upper16:label2 + 4
+ movt r3, :upper16:label3
+ movt r4, :upper16:label3 + 4
+// CHECK-THM: Disassembly of section .R_ARM_THM_MOVT_ABS
+// CHECK-THM-EMPTY:
+// CHECK-THM: 12700:       movt r0, #2
+// CHECK-THM: 12704:       movt r1, #2
+// CHECK-THM: 12708:       movt r2, #2
+// CHECK-THM: 1270c:       movt r3, #2
+// CHECK-THM: 12710:       movt r4, #3
+
+.section .R_ARM_THM_MOVW_PREL_NC, "ax",%progbits
+.align 8
+ movw r0, :lower16:label - .
+ movw r1, :lower16:label1 - .
+ movw r2, :lower16:label2 + 4 - .
+ movw r3, :lower16:label3 - .
+ movw r4, :lower16:label3 + 0x2814 - .
+// CHECK-THM: Disassembly of section .R_ARM_THM_MOVW_PREL_NC
+// CHECK-THM-EMPTY:
+// 0x20000 - . = 55296
+// CHECK-THM: 12800:       movw    r0, #55296
+// 0x20004 - . = 55296
+// CHECK-THM: 12804:       movw    r1, #55296
+// 0x20008 - . + 4 = 55300
+// CHECK-THM: 12808:       movw    r2, #55300
+// 0x2fffc - . = 55280
+// CHECK-THM: 1280c:       movw    r3, #55280
+// 0x2fffc - . + 0x2814 = 0x20000
+// CHECK-THM: 12810:       movw    r4, #0
+
+.section .R_ARM_THM_MOVT_PREL, "ax",%progbits
+.align 8
+ movt r0, :upper16:label - .
+ movt r1, :upper16:label1 - .
+ movt r2, :upper16:label2 + 0x4 - .
+ movt r3, :upper16:label3 - .
+ movt r4, :upper16:label3 + 0x2914 - .
+// CHECK-THM: Disassembly of section .R_ARM_THM_MOVT_PREL
+// CHECK-THM-EMPTY:
+// 0x20000 - . = :upper16:0xd700  = 0
+// CHECK-THM: 12900:       movt    r0, #0
+// 0x20004 - . = :upper16:0xd700 = 0
+// CHECK-THM: 12904:       movt    r1, #0
+// 0x20008 - . + 4 = :upper16:0xd704 = 0
+// CHECK-THM: 12908:       movt    r2, #0
+// 0x2fffc - . = :upper16:0x1d6f0 = 1
+// CHECK-THM: 1290c:       movt    r3, #1
+// 0x2fffc - . + 0x2914 = :upper16:0x20000 = 2
+// CHECK-THM: 12910:       movt    r4, #2
+
+.section .R_ARM_THM_MOVW_BREL_NC, "ax",%progbits
+.align 8
+ movw r0, :lower16:label(sbrel)
+ movw r1, :lower16:label1(sbrel)
+ movw r2, :lower16:label2(sbrel)
+ movw r3, :lower16:label3(sbrel)
+ movw r4, :lower16:label3.4(sbrel)
+// CHECK-THM: Disassembly of section .R_ARM_THM_MOVW_BREL_NC
+// CHECK-THM-EMPTY:
+// SB = 0x20000
+// :lower16:(0x20000 - SB) = 0
+// CHECK-THM: 12a00:       movw    r0, #0
+// :lower16:(0x20004 - SB) = 4
+// CHECK-THM: 12a04:       movw    r1, #4
+// :lower16:(0x20008 - SB) = 8
+// CHECK-THM: 12a08:       movw    r2, #8
+// :lower16:(0x2fffc - SB) = 0xfffc
+// CHECK-THM: 12a0c:       movw    r3, #65532
+// :lower16:(0x2fffc + 4 - SB) = 0x0
+// CHECK-THM: 12a10:       movw    r4, #0
+
+.section .R_ARM_THM_MOVT_BREL, "ax",%progbits
+.align 8
+ movt r0, :upper16:label(sbrel)
+ movt r1, :upper16:label1(sbrel)
+ movt r2, :upper16:label2(sbrel)
+ movt r3, :upper16:label3(sbrel)
+ movt r4, :upper16:label3.4(sbrel)
+// CHECK-THM: Disassembly of section .R_ARM_THM_MOVT_BREL
+// CHECK-THM-EMPTY:
+// SB = 0x20000
+// :upper16:(0x20000 - SB) = 0
+// CHECK-THM: 12b00:       movt    r0, #0
+// :upper16:(0x20004 - SB) = 0
+// CHECK-THM: 12b04:       movt    r1, #0
+// :upper16:(0x20008 - SB) = 0
+// CHECK-THM: 12b08:       movt    r2, #0
+// :upper16:(0x2fffc - SB) = 0
+// CHECK-THM: 12b0c:       movt    r3, #0
+// :upper16:(0x2fffc + 4 - SB) = 1
+// CHECK-THM: 12b10:       movt    r4, #1
+
  .section .destination, "aw",%progbits
  .balign 65536
 // 0x20000
 label:
  .word 0
 // 0x20004
 label1:
  .word 1
 // 0x20008
 label2:
  .word 2
 // Test label3 is immediately below 2^16 alignment boundary
  .space 65536 - 16
 // 0x2fffc
 label3:
  .word 3
 // label3 + 4 is on a 2^16 alignment boundary
+label3.4:
  .word 4