[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#2208462 <https://reviews.llvm.org/D73230#2208462>, @bd1976llvm wrote:

> In D73230#2207837 <https://reviews.llvm.org/D73230#2207837>, @MaskRay wrote:
>
>> In D73230#2207735 <https://reviews.llvm.org/D73230#2207735>, @bd1976llvm wrote:
>>
>>> In D73230#2207634 <https://reviews.llvm.org/D73230#2207634>, @MaskRay wrote:
>>>
>>>> In D73230#2207477 <https://reviews.llvm.org/D73230#2207477>, @bd1976llvm wrote:
>>>>
>>>>> So, clang's behavior has changed so that --wrap no longer wraps symbol definitions for default symbols + -fpic + -fno-semantic-interposition (-fno-semantic-interposition is the default); however, you can restore the old behavior via -fsemantic-interposition. For hidden symbols + -fpic --wrap no longer wraps symbol definitions and there is no way to restore the old definition wrapping behavior.
>>>>
>>>> I think the summary is correct. -fvisibility=hidden nullifies -fsemantic-interposition when the definition is available in the same translation unit. Given how GCC and GNU ld handle/document it, I'd say the previous `hidden` clang behavior working with -Wl,--wrap=foo is accidental rather than intentional. If you don't pass explicit -fsemantic-interposition, in -fPIC mode clang can freely inline foo into call sites, which will also defeat the intended -Wl,--wrap=foo behavior.
>>>>
>>>>> Any undefined reference to symbol will be resolved to "__wrap_symbol".
>>>>
>>>> I think the reasonably portable approach making the wrapping scheme work is `__attribute__((weak))`. An alternative is to move the definitions to a separate translation unit (it does not work with -r or GCC LTO, though).
>>>
>>> .. but now we have this difference in behaviour for -normal vs flto links:
>>>
>>>   ben at ben-VirtualBox:~/tests/wrap$ cat smaller.c 
>>>   void __wrap_foo () {
>>>   	puts ("__wrap_foo");
>>>   	__real_foo();
>>>   }
>>>   
>>>   void foo () { puts("foo()"); }
>>>   
>>>   int main() { foo(); }
>>>   ben at ben-VirtualBox:~/tests/wrap$ clang smaller.c -Wno-implicit-function-declaration -fpic -ffunction-sections -Wl,--wrap=foo -o old.elf -fvisibility=hidden -fuse-ld=lld
>>>   ben at ben-VirtualBox:~/tests/wrap$ ./old.elf
>>>   __wrap_foo
>>>   foo()
>>>   ben at ben-VirtualBox:~/tests/wrap$ clang smaller.c -Wno-implicit-function-declaration -fpic -ffunction-sections -Wl,--wrap=foo -o old_lto.elf -fvisibility=hidden -fuse-ld=lld -flto
>>>   ben at ben-VirtualBox:~/tests/wrap$ ./old_lto.elf
>>>   __wrap_foo
>>>   foo()
>>>   ben at ben-VirtualBox:~/tests/wrap$ ~/u/build/bin/clang smaller.c -Wno-implicit-function-declaration -fpic -ffunction-sections -Wl,--wrap=foo -o new.elf -fvisibility=hidden -fuse-ld=lld
>>>   ben at ben-VirtualBox:~/tests/wrap$ ./new.elf
>>>   foo()
>>>   ben at ben-VirtualBox:~/tests/wrap$ ~/u/build/bin/clang smaller.c -Wno-implicit-function-declaration -fpic -ffunction-sections -Wl,--wrap=foo -o new_lto.elf -fvisibility=hidden -fuse-ld=lld -flto
>>>   ben at ben-VirtualBox:~/tests/wrap$ ./new_lto.elf
>>>   __wrap_foo
>>>   foo()
>>>
>>> ... and the same will be true for default symbols + -fpic when we enable -fno-semantic-interposition by default for -fpic :(
>>
>> I think the non-LTO vs Full LTO difference is due to the way we implement --wrap for LTO: D33621 <https://reviews.llvm.org/D33621>.
>> It adds WeakAny linkage which obtains wrapping behavior.
>>
>> `clang -fuse-ld=lld smaller.c -Wno-implicit-function-declaration -fpic -ffunction-sections -Wl,--wrap=foo -fvisibility=hidden -flto -Wl,--save-temps`
>>
>>   % llvm-dis < a.out.0.0.preopt.bc
>>   ...
>>   define weak hidden void @foo() #0 {  # Note the weakany linkage
>>   ...
>>
>> I think the only way making --wrap sufficiently reliable is to communicate --wrap to the LTO code generation. It was considered unworthy at the implementation time https://bugs.llvm.org/show_bug.cgi?id=33145#c7
>>
>> To make the non-LTO case behave like the LTO case, add `__attribute__((weak))`
>
> I had come to the same conclusion and I agree with your assessment of what is needed to improve the LTO behavior.
>
> Another way of obtaining consistent behavior would be to adjust the current canBenefitFromLocalAlias():
>
>    bool GlobalValue::canBenefitFromLocalAlias() const {
>      // See AsmPrinter::getSymbolPreferLocal().
>   -  return GlobalObject::isExternalLinkage(getLinkage()) && !isDeclaration() &&
>   +  return hasDefaultVisibility() && GlobalObject::isExternalLinkage(getLinkage()) && !isDeclaration() &&
>             !isa<GlobalIFunc>(this) && !hasComdat();
>    }
>
> Doing this would:
>
> 1. Make the code read a bit better (it was not clear to me without going back to the code reviews why this function doesn't consider visibility).
> 2. Remove the introduced difference in behavior for --wrap and hidden definitions.
> 3. Remove the difference in --wrap behavior between LTO and normal builds.
> 4. Improve the size of object files with hidden symbols (and most symbols are hidden now IIUC) by preventing the need for superfluous STT_SECTION symbols.

The proposed canBenefitFromLocalAlias change looks good to me. Mind sending a patch? I think 4 is the important one. I agree that it happens to provide 2 and 3 here but the --wrap behavior still looks unreliable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73230