[Bolt] fix a relocation bug for R_AARCH64_CALL26

[sinan]

Summary

If the R_AARCH64_CALL26 against a symbol that has a lower address, then encodeValueAArch64 will return a wrong value.

In the included test case, the expected output of encodeValueAArch64 is 97ffffff, but it returns 3fffffffffffffff, and then an invalid instruction is encoded.

[Kepontry]

Yes, you are absolutely right. This is my mistake, I didn't consider the sign bit when performing the right shift.

Thank you for your correction; I'm just getting started in this field, and I'll improve the quality of the patches I submit in the future.

Let's wait for the opinion from the Meta team.

[yota9]

Thanks for the fix! Could you please incorporate your test to the old reloc-call26.s file? It seems to be easy to do, thanks!

[sinan]

Merged into test case bolt/test/AArch64/reloc-call26.s

1. Enhanced the test for relocations against functions with both higher and lower addresses.
2. Added --force-patch option in the test case; otherwise, encodeValueAArch64 won't be invoked.

[sinan]

In D159513#4645837, @Kepontry wrote:

Yes, you are absolutely right. This is my mistake, I didn't consider the sign bit when performing the right shift.

Thank you for your correction; I'm just getting started in this field, and I'll improve the quality of the patches I submit in the future.

Let's wait for the opinion from the Meta team.

Hi @Kepontry

Take it easy, I've made even worse mistakes when I just started. Keep going and good luck :)

[yota9]

One more thing. It seems to be without force-patch the encodeValueAArch64 still would be called, but since functions would be moved to the new text there won't be negative address problem. I don't think there is a better way then using force-funcs so it would fail to patch entries & functions would stay on its current place, but please:

1. Please write the reason of using force-funcs option in the test, because after some time even I won't remember what I wrote above :)
2. Please add checks on llvm-bolt output that func1 and func2 were failed to patch and won't be optimised, since otherwise the test might pass, but the problem won't be checked. Alternatively (maybe even better) please check that in output binary the addresses are func1 < _start < func2. Maybe I would do both since the first option also shows more clearly why force-patch was used, but it's up to you.

Thanks!

[sinan]

update the test case.

1. check address order with llvm-nm --numeric-sort;
2. check the message of fail to patch entries;
3. add comment

[sinan]

In D159513#4645982, @yota9 wrote:

One more thing. It seems to be without force-patch the encodeValueAArch64 still would be called, but since functions would be moved to the new text there won't be negative address problem. I don't think there is a better way then using force-funcs so it would fail to patch entries & functions would stay on its current place, but please:

1. Please write the reason of using force-funcs option in the test, because after some time even I won't remember what I wrote above :)
2. Please add checks on llvm-bolt output that func1 and func2 were failed to patch and won't be optimised, since otherwise the test might pass, but the problem won't be checked. Alternatively (maybe even better) please check that in output binary the addresses are func1 < _start < func2. Maybe I would do both since the first option also shows more clearly why force-patch was used, but it's up to you.

Thanks!

your analysis is right. encodeValueAArch64 will be called without force-patch. What this option really does is keep the address order of func1, _start and func2 since func1 and func2 fail to patch entries. Thanks!

[yota9]

Thanks for the fix, @sinan ! LGTM

[Amir]

@sinan – please properly capitalize the title and use 50-72 commit formatting rule to align with the rest of BOLT commits for your future commits. Thank you for the fix!

bolt/lib/Core/Relocation.cpp

static uint64_t encodeValueAArch64(uint64_t Type, uint64_t Value, uint64_t PC) {
   case ELF::R_AARCH64_PREL64:
     Value -= PC;
     break;
   case ELF::R_AARCH64_CALL26:
     Value -= PC;
     assert(isInt<28>(Value) && "only PC +/- 128MB is allowed for direct call");
     // Immediate goes in bits 25:0 of BL.
     // OP 1001_01 goes in bits 31:26 of BL.
-    Value = (Value >> 2) | 0x94000000ULL;
+    Value = ((Value >> 2) & 0x3ffffff) | 0x94000000ULL;
     break;
   }
   return Value;
 }
 
 static uint64_t extractValueX86(uint64_t Type, uint64_t Contents, uint64_t PC) {
   if (Type == ELF::R_X86_64_32S)
     return SignExtend64<32>(Contents);

bolt/test/AArch64/reloc-call26-overflow.s

+# REQUIRES: system-linux
+
+# RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown \
+# RUN:   %s -o %t.o
+# RUN: %clang %cflags %t.o -o %t.exe -Wl,-q
+# RUN: llvm-bolt %t.exe -o %t.bolt --funcs=func1 --force-patch
+# RUN: llvm-objdump -d --disassemble-symbols='_start' %t.bolt | \
+# RUN:   FileCheck %s
+
+# CHECK: {{.*}} bl {{.*}} <func1>
+
+  .text
+  .align 4
+  .global func1
+  .type func1, %function
+func1:
+  ret
+  .size func1, .-func1
+  .global _start
+  .type _start, %function
+_start:
+  bl func1
+  mov     w8, #93
+  svc     #0
+  .size _start, .-_start