[PATCH] D73230: [X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D73230#2207159 <https://reviews.llvm.org/D73230#2207159>, @bd1976llvm wrote:

> In D73230#2207023 <https://reviews.llvm.org/D73230#2207023>, @MaskRay wrote:
>
>> In D73230#2206232 <https://reviews.llvm.org/D73230#2206232>, @bd1976llvm wrote:
>>
>>> @MaskRay - this change causes a behaviour difference for --wrap.
>>>
>>> Here is the --wrap behaviour before this change:
>>>
>>>   ben at ben-VirtualBox:~/tests/wrap$ more main.c
>>>   void __wrap_foo () {
>>>   	puts ("__wrap_foo");
>>>   	__real_foo();
>>>   }
>>>   
>>>   void foo () { puts("foo()"); }
>>>   
>>>   int main() {
>>>   	__real_foo();
>>>   	puts("---");
>>>   	__wrap_foo();
>>>   	puts("---");
>>>   	foo();
>>>   	return 0;
>>>   }
>>>   ben at ben-VirtualBox:~/tests/wrap$ gcc main.c -Wl,--wrap=foo -ffunction-sections -fuse-ld=lld -o lld.elf -Wno-implicit-function-declaration
>>>   ben at ben-VirtualBox:~/tests/wrap$ ./lld.elf 
>>>   foo()
>>>   ---
>>>   __wrap_foo
>>>   foo()
>>>   ---
>>>   __wrap_foo
>>>   foo()
>>>
>>> … and here is the behaviour after this change:
>>>
>>>   ben at ben-VirtualBox:~/tests/wrap$ ./lld.elf 
>>>   foo()
>>>   ---
>>>   __wrap_foo
>>>   foo()
>>>   ---
>>>   foo()
>>>
>>> There is no behaviour change for -flto builds so the behaviour for --wrap is now effectively different for LTO vs normal builds.
>>
>> I think you missed a point in the description of --wrap:
>>
>>   You may wish to provide a "__real_malloc" function as well, so that links without the --wrap option will succeed.  If you do this, you
>>   should not put the definition of "__real_malloc" in the same file as "__wrap_malloc"; if you do, the assembler may resolve the call
>>   before the linker has a chance to wrap it to "malloc".
>>
>> Providing `foo` definition in the translation unit where they are referenced is not reliable when you are using `--wrap`.
>> Actually, this is where GNU ld and LLD differ. See https://sourceware.org/bugzilla/show_bug.cgi?id=26358 and the history of `lld/test/ELF/wrap-shlib-undefined.s`
>>
>> If you want to get guaranteed semantics, don't define `foo` when it is referenced. You may also try gcc and gcc -fPIC -fno-semantic-interposition, the behavior is similar to latest clang.
>
> Thanks for the summary. I am not particularly concerned about which behaviour we have w.r.t. wrapping intra-translation-unit references (although I have seen some evidence that lld's behaviour is useful e.g. https://stackoverflow.com/questions/13961774/gnu-gcc-ld-wrapping-a-call-to-symbol-with-caller-and-callee-defined-in-the-sam). However, you stated in https://sourceware.org/bugzilla/show_bug.cgi?id=26358 that for lld -r, lto, and normal links have the same behaviour - that is not true after this change. Furthermore, with the current clang it is not possible to go back to the old behaviour using -fsemantic-interposition for hidden symbols. IIUC I think that hidden symbols are probably the majority of opensource symbols now as the GNU toolchain encourages the use of -fvisiblity=hidden.

Let me summarize the cases:

- `{clang,gcc} -fuse-ld=lld main.c  -Wl,--wrap=foo` => wrapped
- `{clang,gcc} -fuse-ld=bfd main.c  -Wl,--wrap=foo` => not wrapped (in GNU ld, --wrap is per object file. --wrap is not effective when the symbol is defined)
- `{clang,gcc} -fuse-ld=lld main.c -fPIC -fno-semantic-interposition -Wl,--wrap=foo` => not wrapped (references go through `.Lfoo$local` which cannot be wrapped)

I think your make an integration between this commit and 872c5fb1432493c0a09b6f210765c0d94ce9b5d0 <https://reviews.llvm.org/rG872c5fb1432493c0a09b6f210765c0d94ce9b5d0>, so for -fno-PIC or -fPIE, you observe the `-fPIC -fno-semantic-interposition` behavior as well. If you cherry pick 872c5fb1432493c0a09b6f210765c0d94ce9b5d0 <https://reviews.llvm.org/rG872c5fb1432493c0a09b6f210765c0d94ce9b5d0> and don't use -fno-semantic-interposition, and use LLD, you should get a wrapping behavior.

I indeed prefer the LLD behavior, so I filed https://sourceware.org/bugzilla/show_bug.cgi?id=26358 yesterday, but I cannot say the wrapping behavior is promised. If you want better portability, make `foo` weak (and be aware of side effects with the change).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73230