[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.s

 ## This test checks processing of R_AARCH64_CALL26 relocation
 ## when option `--funcs` is enabled
 
+## We want to test on relocations against functions with both higher
+## and lower addresses. The '--force-patch' option is used to prevent
+## the functions func1 and func2 from being optimized, so that their
+## addresses can remain unchanged. Therefore, the relocations can be
+## updated via encodeValueAArch64 and the address order in the output
+## binary is func1 < _start < func2.
+
 # 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
+# RUN: llvm-bolt %t.exe -o %t.bolt --funcs=func1,func2 \
+# RUN:   --force-patch 2>&1 | FileCheck %s -check-prefix=CHECK-BOLT
 # RUN: llvm-objdump -d --disassemble-symbols='_start' %t.bolt | \
 # RUN:   FileCheck %s
+# RUN: llvm-nm --numeric-sort --extern-only %t.bolt  | FileCheck \
+# RUN:   %s -check-prefix=CHECK-FUNC-ORDER
 
+# CHECK-BOLT: BOLT-WARNING: failed to patch entries in func1. The function will not be optimized.
+# CHECK-BOLT: BOLT-WARNING: failed to patch entries in func2. The function will not be optimized.
 # CHECK: {{.*}} bl {{.*}} <func1>
+# CHECK: {{.*}} bl {{.*}} <func2>
+
+# CHECK-FUNC-ORDER: {{.*}} func1
+# CHECK-FUNC-ORDER-NEXT: {{.*}} _start
+# CHECK-FUNC-ORDER-NEXT: {{.*}} func2
 
   .text
   .align 4
+  .global func1
+  .type func1, %function
+func1:
+  ret
+  .size func1, .-func1
   .global _start
   .type _start, %function
 _start:
   bl func1
+  bl func2
   mov     w8, #93
   svc     #0
   .size _start, .-_start
-
-  .global func1
-  .type func1, %function
-func1:
+  .global func2
+  .type func2, %function
+func2:
   ret
-  .size func1, .-func1
+  .size func2, .-func2