[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#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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73230