[PATCH] D22356: [ThinLTO] Perform conservative weak/linkonce resolution in distributed backend case

[Xinliang David Li via llvm-commits]

On Mon, Jul 18, 2016 at 7:36 PM, Teresa Johnson <tejohnson at google.com>
wrote:

>
>
> On Mon, Jul 18, 2016 at 7:02 PM, Xinliang David Li <davidxl at google.com>
> wrote:
>
>>
>>
>> On Mon, Jul 18, 2016 at 5:37 PM, Mehdi AMINI <mehdi.amini at apple.com>
>> wrote:
>>
>>> mehdi_amini added a comment.
>>>
>>> In https://reviews.llvm.org/D22356#487800, @tejohnson wrote:
>>>
>>> > In https://reviews.llvm.org/D22356#487784, @mehdi_amini wrote:
>>> >
>>> > > > That's a good question and an idea I thought about briefly but
>>> discarded for a couple reasons. I was concerned about requiring
>>> communication between the ThinLink and final link to build the link line
>>> (it would be more difficult to support in a build system, and also seems
>>> conceptually more complicated).
>>> > >
>>> > >
>>> > > How is the final link invocation computed right now?
>>> >
>>> >
>>> > The link line is essentially the same, but with native .o instead of
>>> the bitcode .o. (See the new test case for an example)
>>>
>>>
>>> The question is: in the presence of static archives, how do you
>>> generates --start-lib/--end-lib? This seems to already require some
>>> build-system integration?
>>>
>>
>> There is one to one mapping from bitcode .o and native .o -- so the
>> command line can be formed.
>>
>>
>>>
>>> > >
>>>
>>> >
>>>
>>> > >
>>>
>>> >
>>>
>>> > > > Also I'm not 100% convinced that removing the
>>> --start-lib/--end-lib, even if we only include those object files the
>>> linker decided to select symbols from, would result in the same linking
>>> behavior.
>>>
>>> >
>>>
>>> > >
>>>
>>> >
>>>
>>> > >
>>>
>>> >
>>>
>>> > > Your observations about the linker picking different symbols seem to
>>> indicate that the --start-lib/--end-lib model is already broken.
>>>
>>> >
>>>
>>> >
>>>
>>> > When you say "is already broken" do you mean even in non-ThinLTO mode?
>>> I'm not sure why - it is just like having an archive of the objects between
>>> each start/end pair.
>>>
>>>
>>> I'm only talking about ThinLTO and the two-stage linking, i.e. the
>>> second invocation of the linker does not end-up with the same prevailing
>>> resolution as the first invocation. Your current patches are working around
>>> this deficiency.
>>>
>>
>>
>> Since they are orginally from LinkOnceODR defs, whatever definition is
>> picked up is implementation dependent.
>>
>
> Right for the multiply defined ODR refs, it shouldn't matter that the
> second link picks a different copy (we just need to do the conservative
> thing as in this and my other patch to ensure we don't introduce backwards
> refs).
>
> I do have a worry about non-ODR multiply defined case. E.g. In the example
> if the first copy originally chosen as prevailing is weak (non-odr), and
> due to the importing of strong references to that object we no longer
> linked in its symbols in the second link, we might end up linking with a
> different weak copy that has a different body - resulting in different
> program behavior. If that is a real concern then I suppose we must do
> something else to ensure that the same copy is prevailing in the second
> link...
>

In this case, whatever copy (of the weak def) that is picked up is
implementation defined -- for instance link line order. Practically
speaking, it should not be a concern.

David


>
>
>>> >
>>>
>>> >
>>>
>>> > > If a list of `.o` on the command line is not enough for relinking,
>>> there's gonna be a need for a "linker resolution map" file that drives the
>>> linker.
>>>
>>> >
>>>
>>> >
>>>
>>> > In ThinLTO it is because of the change (between the ThinLink and
>>> native object link) in which strong references exist between
>>> objects/libraries due to importing and inlining. But I believe with this
>>> patch and the follow-on https://reviews.llvm.org/D22467 the importing
>>> and symbol resolution is made suitably conservative.
>>>
>>>
>>> I don't see any justification for --start-lib/--end-lib right now.
>>>
>>>
>> Can you clarify what do you mean by 'justification' here?
>>
>
> Reading through Mehdi's comments, I think he didn't realize we were
> already using --start-lib/--end-lib instead of regular archive libraries.
>
>
>>
>> thanks,
>>
>> David
>>
>>
>>>
>>> https://reviews.llvm.org/D22356
>>>
>>>
>>>
>>>
>>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
> 408-460-2413
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160718/742dc292/attachment.html>