[PATCH] D150940: [lld][RISCV][test] Add test for relaxation combined with --wrap

Tue May 30 19:14:20 PDT 2023

MaskRay added a comment.

In D150940#4359968 <https://reviews.llvm.org/D150940#4359968>, @jobnoorman wrote:

> In D150940#4356721 <https://reviews.llvm.org/D150940#4356721>, @kishore-rivos wrote:
>
>> To clarify, it isn't necessary for both foo and _wrap_foo to be defined in the same object file.  This bug was also triggered when they were defined in different object files.
>
> Thanks for mentioning this! I just noticed that situation is worse: it's still broken when `bar` is in a different object file but for a different reason than before.
>
> The following test is broken because the value of `bar` does not get adjusted:
>
>   #--- main.s
>   .globl _start
>   _start:
>     call bar
>   .globl bar
>   bar:
>   ## Note that this can be any instruction that gets relaxed. The goal is to force
>   ## the value of __wrap_bar to change.
>     call __wrap_bar
>   
>   #--- wrap.s
>   .globl __wrap_bar
>   __wrap_bar:
>     call __real_bar
>
> This is caused by this loop <https://github.com/llvm/llvm-project/blob/bf497c0be2301c77a18d37ef104fcf6758095956/lld/ELF/Arch/RISCV.cpp#L553-L564>: for some reason, `bar` is part of the symbols returned by `getSymbols()` on `wrap.o` while its `file` points to `main.o`. So it never gets added to `anchors` and hence its value never gets adjusted.
>
> @MaskRay is this something that should be fixed in the relaxation implementation or is there an underlying issue with `--wrap` that should be addressed?

Sorry for my belated response. I sent D151768 <https://reviews.llvm.org/D151768> to fix this case and add a test for the bug fixed by D149735 <https://reviews.llvm.org/D149735>.

> Note that I'm not quite sure whether inserting this symbol twice is actually intended behavior. Could it be worthwhile to look into this?

Having duplicate entries in `anchors` is certainly not ideal, but I think is acceptable as eliminating duplicates will add complexity, increase memory usage, and make the code more difficult to parallelize.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150940