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

[ben via Phabricator via llvm-commits]

bd1976llvm added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73230