[PATCH] D123334: [BOLT] Update skipRelocation for aarch64

Wed Apr 13 07:22:14 PDT 2022

yota9 marked 2 inline comments as done.
yota9 added inline comments.

================
Comment at: bolt/test/runtime/plt-lld.test:1-2
 // This test checks that the pointers to PLT are properly updated.
 // The test is using lld linker.

----------------
rafauler wrote:
> Is it possible to add a check by dumping the binary symbol table to verify that these are properly updated? having the test as is (checking if a binary runs) is quite opaque for reviewers and makes it a bit hard for us to be helpful and understand what is going on and what your patch is fixing. If we treat these relocations that were relaxed to ADR as real relocs (meaning, we do not have your patch), what does it happen? Does BOLT crash? Do PLT pointers get messed up?
I will add extra test now. 
>> what does it happen? 
The pointers in code are messed up, so we will have runtime problems. E.g. 
Let's suppose the GOT relocations were relaxed to the local symbol definition

   1036c:       d503201f        nop
                        1036c: R_AARCH64_ADR_GOT_PAGE   foo2
   10370:       10000080        adr     x0, 10380 <foo2>
                        10370: R_AARCH64_LD64_GOT_LO12_NC       foo2

After bolt without this patch we will have: 

  400024:       90ffe000        adrp    x0, 0 <_DYNAMIC-0x20388>
  400028:       91000000        add     x0, x0, #0x0

Because the extracted value was messed up (we were expecting other instruction)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123334