[PATCH] D157533: [JITLink][AArch32] Implement ELF::R_ARM_CALL relocation

Wed Sep 13 07:08:51 PDT 2023

Eymay added inline comments.

================
Comment at: llvm/test/ExecutionEngine/JITLink/AArch32/ELF_static_arm_reloc.s:16
+# CHECK-INSTR: 	       4: e12fff1e     bx      lr
+# jitlink-check: decode_operand(call_site, 0) = call_target - next_pc(call_site) - 4
+	.globl	call_site
----------------
sgraenitz wrote:
> This is correct and works, but the attentive reader might wonder where the `-4` is coming from.
> 
> The branch offset in Thumb state is equivalent to the size of the branch instruction, i.e. `next_pc` does a `+4` and thus we don't need the `-4` [in the Thumb test here](https://github.com/llvm/llvm-project/blob/24f882287666ea782bf84be9855c756755689561/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_static_thumb_reloc.s#L20).
> 
> Surprisingly, the branch offset in ARM state is `8` and not `4` as `next_pc` tells us. That's because the branch instruction increments the PC register twice since it runs an implicit prefetch instruction (seems to be a ARM legacy thing that the linker has to know that). I guess ideally we could write `next_pc(next_pc(call_site))` but the checker cannot evaluate that expression. Thus, I wonder if it would be better to drop the `next_pc` here, write out the branch offset explicitly and add a comment like this.
> 
> What do you think?
The magic number 4 doesn't really look good and it makes sense to show the condition explicitly. Thanks for the explanation of why the offset is present!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157533/new/

https://reviews.llvm.org/D157533