This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by peter.smith on Aug 15 2017, 8:22 AM.

Details

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.

Diff Detail

Event Timeline

peter.smith created this revision.Aug 15 2017, 8:22 AM
grimar added a subscriber: grimar.Sep 4 2017, 2:05 AM

This needs testcase I think.

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

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 added a comment.Sep 4 2017, 7:32 AM

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).

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 added a comment.Sep 4 2017, 7:58 AM

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 ?

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 added a comment.EditedSep 4 2017, 9:00 AM

! 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
8

Duplicate requires line.

40

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.

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

grimar added a comment.Sep 5 2017, 1:28 AM

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

test/ELF/relocation-b-aarch64.test
8

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

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

ruiu added inline comments.Sep 5 2017, 2:57 PM
ELF/Arch/AArch64.cpp
236

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

Can you use LLVM_FALLTHROUGH intead?

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 accepted this revision.Sep 6 2017, 10:26 AM

LGTM

ELF/Arch/AArch64.cpp
241

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.

This revision is now accepted and ready to land.Sep 6 2017, 10:26 AM
This revision was automatically updated to reflect the committed changes.