[PATCH] D39392: Do not add weak undefined symbols to .dynsym unless -pic or -shared are given.

[Rafael Avila de Espindola via llvm-commits]

This is an explicit regression of PR34301, no? If so I don't think we
should do it.

The complication you describe comes from the compiler assuming that a
symbol is local and leaving the linker to do magic if it turns out it is
not.

The only way I see out of this is changing the compiler to not make such
assumptions and have the linker optimize the converse: improve access
when the symbol is local.

The optimization of access to local symbols already exists.

In llvm we now have a dso_local flag in the IR. All that is missing is

* Having clang set it in the cases shouldAssumeDSOLocal currently
  guesses the symbol is local.

* Change shouldAssumeDSOLocal to just return dso_local

* Add an option to clang (-fno-assume-local-declarations?) to have it
  not guess that declarations are local even without -fPIC.

* Make that option the default.

Cheers,
Rafael


Rui Ueyama <ruiu at google.com> writes:

> This patch changes the existing broken semantics of the linker with a new,
> simplified one. So, I'll copy the comment that I added in this patch here
> for those who are interested in linker's weak undefined symbol semantics
> when dynamic linking involved.
>
> ======
>
> Exported weak symbols are not representable unless they are resolved
> through GOT. We simply export weak symbols only when -shared or -pie
> were given.
>
> As a result, if an weak symbol cannot be resolved within the current
> output file, and if no -shared nor -pie were given, the symbol is
> resolved to zero at link-time. If you want to give it a second chance
> to be resolved at load-time, you need to pass -shared or -pie.
>
> So, why are they not representable? To understand that, assume the
> following code:
>
>   __attribute__((weak)) int foo();
>   void bar() { if (foo) foo(); }
>
> If this code is compiled without -fPIC, function pointer foo in the
> "if" is compiled to a direct reference to a function. The linker would
> create a PLT entry for foo and use its address as a function pointer
> value for foo. It would usually work, because when you jump to foo's
> PLT, it in turn jumps to foo's real function definition.
>
> However, that mechanism can result in a bad combination of pointer
> values if the symbol is weak. Symbol foo's function pointer value is
> always non-zero because it is resolved to a linker-synthesized foo's
> PLT entry (that means the "if" condition is always true). But, if the
> loader cannot resolve foo at load-time, it leaves foo's PLT entry zero.
> That means when you jump there, it then jumps to an unexpected
> address, and the program crashes. As a result, if foo cannot be
> resolved at load-time, the program crashes. This is apparently what
> users would not expect.
>
> If all references to symbol foo go through GOT, weak symbols are
> representable. If the loader cannot resolve foo, it leaves foo's GOT
> entry zero, and the "if" condition simply becomes false.
>
> So, we want to export weak symbols only when we know they are
> referenced through GOT.
>
> (The decision we are making here is not perfect; even if -shared or
> -pie are given to the linker, input object files may have direct
> references to weak symbols. We'll report them as errors in
> scanRelocations. Likewise, even if -shared nor -pie were not given,
> all relocations to weak symbols could go through GOT. Even so, we
> won't export them, because it is hard to detect it ahead of time. It
> is also generally preferable to let users explicitly control the
> linker's behavior via command line options rather than changing
> the behavior implicitly depending on existence or absense of some
> relocations.)
>
> On Sat, Nov 4, 2017 at 5:31 PM, Rui Ueyama <ruiu at google.com> wrote:
>
>> This is the first step to simplify and improve relocation handling in lld.
>> I'd like to submit it so that I can make further changes. Can you take a
>> look?
>>
>> On Sat, Nov 4, 2017 at 5:27 PM, Rui Ueyama via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> ruiu updated this revision to Diff 121606.
>>> ruiu added a comment.
>>> Herald added a subscriber: arichardson.
>>>
>>> - Rebased
>>>
>>>
>>> https://reviews.llvm.org/D39392
>>>
>>> Files:
>>>   lld/ELF/Relocations.cpp
>>>   lld/ELF/Symbols.cpp
>>>   lld/test/ELF/weak-undef-export.s
>>>   lld/test/ELF/weak-undef-pic-nopic.s
>>>
>>>
>>