[LLD][ELF] Always write non-immediate bits for AArch64 branch instruction.

[peter.smith]

Summary

To support errata patching on AArch64 we need to be able to overwrite an arbitrary instruction with a branch. For AArch64 it is sufficient to always write all the bits of the branch instruction and not just the immediate field. This is safe as the non-immediate bits of the branch instruction are always the same.

This is patch 2 of 3 to fix pr33463 https://bugs.llvm.org/show_bug.cgi?id=33463 although it can stand independently. The general idea for the fix will be to change an instruction in the section to be patched by adding a branch relocation (or modifying an existing relocation) at the same offset as the instruction.

[grimar]

This needs testcase I think.

[javed.absar]

I am afraid it seems you forgot to do a diff -U9999, as the full context is not visible.

[peter.smith]

Thanks both for the comments. My apologies for forgetting the context, I've added a new diff with it in. Adding a comprehensive test at this stage is difficult because (sensibly) there is no way with llvm-mc to generate a R_AARCH64_ JUMP26 relocation on a non branch instruction. What I've done here is added a test case that uses the full range of the immediate field of the JUMP26, if the extra overwriting would break anything in the immediate the test will fail. The next review in the sequence that implements the patching will heavily test this.

[grimar]

In D36745#860283, @peter.smith wrote:

Thanks both for the comments. My apologies for forgetting the context, I've added a new diff with it in. Adding a comprehensive test at this stage is difficult because (sensibly) there is no way with llvm-mc to generate a R_AARCH64_ JUMP26 relocation on a non branch instruction.

Thanks for test. I was not aware about such problem.
I think it should be possible to use yaml2obj to simplify it (up to you, just want to share the idea).

For example lld\test\ELF\relocation-none-aarch64.testis something very close to what I am thinking about,
it has single relocation and you probably should be able to replace it with R_AARCH64_JUMP26 and R_AARCH64_CALL26 relocations
to check which bytes LLD emits for them (and check that bytes are different after this patch).

[peter.smith]

Thanks for pointing the yaml tests out, I've been able to use it to write a test case to show that the b opcode is written.

[grimar]

In D36745#860316, @peter.smith wrote:

Thanks for pointing the yaml tests out, I've been able to use it to write a test case to show that the b opcode is written.

Great ! So do you need aarch64-branch-fullrange.s testcase still then ?

[peter.smith]

In D36745#860321, @grimar wrote:

In D36745#860316, @peter.smith wrote:

Thanks for pointing the yaml tests out, I've been able to use it to write a test case to show that the b opcode is written.

Great ! So do you need aarch64-branch-fullrange.s testcase still then ?

It is adding some value in that it shows the immediate field isn't getting corrupted but I'm happy to remove it if it is seen to be redundant, or not worth it.

[grimar]

! In D36745#860322, @peter.smith wrote:
It is adding some value in that it shows the immediate field isn't getting corrupted but I'm happy to remove it if it is seen to be redundant, or not worth it.

Single test IMO should be enough, please see my comments inline.

test/ELF/relocation-b-aarch64.test

7 ↗

(On Diff #113755)

Duplicate requires line.

39 ↗

(On Diff #113755)

If you put 2 symbols here:

....
Sections:
  - Type:            SHT_PROGBITS
    Name:            .text
    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
    Content:         "0000000000000000"
...
Symbols:
 Global:
   - Type:             STT_FUNC
     Section:          .text
     Name:             foo
   - Type:             STT_FUNC
     Section:          .text
     Name:             bar

Then output will be:

Disassembly of section .text:
foo:
  20000:	00 00 00 14 	b	#0
  20004:	ff ff ff 17 	        b	#-4

What I believe shows that immediate field isn't getting corrupted.

[peter.smith]

Ok, I've done something similar with the yaml test and removed the aarch64-branch-fullrange.s test

[grimar]

Looks good, thanks ! I have no other comments (minor nit).

test/ELF/relocation-b-aarch64.test

7 ↗

(On Diff #113768)

Please remove the duplicate, you have it at first line already.

[peter.smith]

Thanks for pointing that out, I've removed the duplicate.

[ruiu]

ELF/Arch/AArch64.cpp

236 ↗

(On Diff #113808)

Can you mention that this is for --fix-cortex-a53-843419, as what this code does is unusual and doesn't make much sense unless the reader understand this is for a CPU bug.

240 ↗

(On Diff #113808)

Can you use LLVM_FALLTHROUGH intead?

[peter.smith]

Thanks very much for the comments. I've update the diff with a revised comment mentioning the -fix-cortex-a53-843419 option, I have also added LLVM_FALLTHROUGTH.

[ruiu]

LGTM

ELF/Arch/AArch64.cpp

241 ↗

(On Diff #113983)

Maybe I'd emphasize that this is irregular but necessary: If the cpu didn't have a bug, ovewriting opcode doesn't make sense, but this is not the case.

lld/trunk/ELF/Arch/AArch64.cpp

void AArch64::relocateOne(uint8_t *Loc, uint32_t Type, uint64_t Val) const {
   case R_AARCH64_TLSDESC_ADR_PAGE21:
     checkInt<33>(Loc, Val, Type);
     write32AArch64Addr(Loc, Val >> 12);
     break;
   case R_AARCH64_ADR_PREL_LO21:
     checkInt<21>(Loc, Val, Type);
     write32AArch64Addr(Loc, Val);
     break;
-  case R_AARCH64_CALL26:
   case R_AARCH64_JUMP26:
+    // Normally we would just write the bits of the immediate field, however
+    // when patching instructions for the cpu errata fix -fix-cortex-a53-843419
+    // we want to replace a non-branch instruction with a branch immediate
+    // instruction. By writing all the bits of the instruction including the
+    // opcode and the immediate (0 001 | 01 imm26) we can do this
+    // transformation by placing a R_AARCH64_JUMP26 relocation at the offset of
+    // the instruction we want to patch.
+    write32le(Loc, 0x14000000);
+    LLVM_FALLTHROUGH;
+  case R_AARCH64_CALL26:
     checkInt<28>(Loc, Val, Type);
     or32le(Loc, (Val & 0x0FFFFFFC) >> 2);
     break;
   case R_AARCH64_CONDBR19:
     checkInt<21>(Loc, Val, Type);
     or32le(Loc, (Val & 0x1FFFFC) << 3);
     break;
   case R_AARCH64_LD64_GOT_LO12_NC:

lld/trunk/test/ELF/relocation-b-aarch64.test

+# REQUIRES: aarch64
+
+# RUN: yaml2obj %s -o %t.o
+# RUN: ld.lld %t.o -o %t.out
+# RUN: llvm-objdump -d -triple=aarch64-none-linux %t.out | FileCheck %s
+
+# Check that the R_AARCH64_JUMP26 writes the branch opcode as well as the
+# immediate. We use this property to overwrite instructions with a branch.
+
+# CHECK: Disassembly of section .text:
+# CHECK-NEXT: foo:
+# CHECK-NEXT:    20000:        01 00 00 14     b       #4
+# CHECK: bar:
+# CHECK-NEXT:    20004:        ff ff ff 17     b       #-4
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_AARCH64
+Sections:
+  - Type:            SHT_PROGBITS
+    Name:            .text
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Content:         "0000000000000000"
+  - Type:            SHT_RELA
+    Name:            .rela.text
+    Link:            .symtab
+    Info:            .text
+    Relocations:
+      - Offset:          0
+        Symbol:          bar
+        Type:            R_AARCH64_JUMP26
+      - Offset:          4
+        Symbol:          foo
+        Type:            R_AARCH64_JUMP26
+
+Symbols:
+ Local:
+   - Type:             STT_FUNC
+     Section:          .text
+     Name:             foo
+     Value:            0
+   - Type:             STT_FUNC
+     Section:          .text
+     Name:             bar
+     Value:            4