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

[jobnoorman]

Summary

D150220 proposed a fix for a relaxation bug when --wrap is used. The
issue happened to be already (accidentally) fixed by D149735. This patch
adds a test to ensure the issue isn't reintroduced in the future.

The issue is the following: when --wrap bar is used and an object file
defines both bar and __wrap_bar, __wrap_bar is inserted twice in
the symbol table pointing to the same Defined. Before D149735, symbol
values were adjusted by repeated subtraction causing the delta to be
subtracted twice from the duplicated symbol.

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

[kishore-rivos]

Thanks @jobnoorman for turning this into a proper test. 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.

[jobnoorman]

In 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: 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?

[MaskRay]

In D150940#4359968, @jobnoorman wrote:

In 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: 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 to fix this case and add a test for the bug fixed by 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.

[jobnoorman]

In D150940#4382913, @MaskRay wrote:

In D150940#4359968, @jobnoorman wrote:

In 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: 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 to fix this case and add a test for the bug fixed by D149735.

Thanks! I'll close this one now since your patch includes the necessary tests.

lld/test/ELF/riscv-relax-wrap.s

+## Check that relaxation doesn't break when using --wrap. See D150220
+
+# REQUIRES: riscv
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+relax %t/main.s -o %t/main.o
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+relax %t/wrap.s -o %t/wrap.o
+# RUN: ld.lld %t/main.o %t/wrap.o --wrap bar -o %t1
+# RUN: llvm-objdump -t %t1 | FileCheck %s
+
+# CHECK-DAG: 0000000000011120 g       .text 0000000000000000 _start
+# CHECK-DAG: 0000000000011124 g       .text 0000000000000000 bar
+# CHECK-DAG: 0000000000011128 g       .text 0000000000000000 __wrap_bar
+
+#--- main.s
+.globl _start
+_start:
+  call bar
+
+#--- wrap.s
+.globl bar, __wrap_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_bar:
+  call __real_bar