[PATCH] D155732: [MC][COFF][AArch64] Avoid incorrect IMAGE_REL_ARM64_BRANCH26 relocations.

Fri Jul 21 01:24:21 PDT 2023

mstorsjo added a comment.

In D155732#4520749 <https://reviews.llvm.org/D155732#4520749>, @hjyamauchi wrote:

>>> I had expected those cases to be working correctly though, for many years now, but perhaps there is some case where the original offset indeed happens to get ignored.
>
> F28371736: branch26-test-log.txt <https://reviews.llvm.org/F28371736>
> The attach text file contains a test (*) and the command line trace that shows the relocation behaviors (without this patch) in the first half, and the same sequence of commands with this patch in the second half.
>
> The first part shows that both link.exe and lld relocate the branches to the beginning of the text and the other section, as opposed to the ret instructions, respectively, because the offsets embedded in the instruction are ignored. The second part shows that, with this patch, the linkers relocate them as expected.
>
> (*) A tweaked version of the test in this patch (because some important details were unexpectedly lost along the test reduction and nops were added to avoid accidental offset correctness as small offsets from a symbol can happen to be correct).

Thanks for the test transcript! It does indeed look like link.exe ignores the existing offset. I also independently tested this, and it actually looks like link.exe blindly `or`s the target offset bits on top of the existing opcode, just like lld does: https://github.com/llvm/llvm-project/blob/llvmorg-16.0.6/lld/COFF/Chunks.cpp#L306-L322 Depending on the contents of the original offset and the intended symbol difference, the offset may end up being partially visible or not. In any case - the linker doesn't indeed seem to be able to handle immediate offsets here.

Strictly speaking I would consider that a bug in both linkers, but I totally agree we should avoid producing such relocations. I didn't have time to look at the implementation strategy itself yet (but trust others' judgement on it).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155732