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

[Mehdi Amini via llvm-commits]

> On 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 <mailto:davidxl at google.com>> wrote:
> 
> 
> On Mon, Jul 18, 2016 at 5:37 PM, Mehdi AMINI <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
> mehdi_amini added a comment.
> 
> In https://reviews.llvm.org/D22356#487800 <https://reviews.llvm.org/D22356#487800>, @tejohnson wrote:
> 
> > In https://reviews.llvm.org/D22356#487784 <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…


If we carry the proper linker-resolution to ThinLTO, there shouldn’t be multiple version of a weak symbol left during the second link (all of them will be turned into available_externally but the prevailing).

Even abstracting this, if you have multiple definitions of a weak symbol while linking a program, I can’t see how they wouldn’t be ODR. It would at least rely on implementation-defined linker behavior (assuming the objets are selected by the linker if they are in static archives).


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

I clarified my comment about the expectation for the second link in Phab.
(But yes I didn’t grasp that you were originally passing these options)

— 
Mehdi

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160718/8d7b7287/attachment.html>