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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D22356#487942, @mehdi_amini wrote:

> > > > 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?
>
> > 
>
> > 
>
> > We use --start-lib/--end-lib in all of our links - not regular archive libraries. Yes, a distributed build on regular archive libraries will require build system integration to extract the individual object files first.
>
>
> Ok I see, makes sense, I thought you were avoiding re-creating the static library and emulating it with these options.
>
> > > > > > 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.
>
> > 
>
> > > 
>
> > 
>
> > > > 
>
> > 
>
> > > 
>
> > 
>
> > > > 
>
> > 
>
> > > 
>
> > 
>
> > > > > 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.
>
> > 
>
> > 
>
> > We use --start-lib/--end-lib internally instead of regular objects. So it is not a matter of justification, it is a matter of keeping that working.
>
>
> I don't believe this is relevant: the fact that the first link is taking libraries as an input does not make it a compelling case to use them for the second link. Static libraries or start-lib/end-lib are a specific semantic model, and I believe it is just wrong to pass them to the final link.
>
> The reason is that the first link is performing linker resolution: this decision process carry some specific semantic with archives. After this resolution and the ThinLTO process, there is no reason that makes sense to me right now to repeat this process.


But it shouldn't be a correctness issue to do so.

> It is possible that it is because I have a different mental model of static archives right now. AFAIK, the semantic difference between plain objects and archive is that an object defined in an archive is loaded and selected by the linker only if one the symbol it defines is referenced.


That is my understanding as well.

> Keeping the linker semantic with ThinLTO means that the objects and symbols selected during the first link should be the "source of truth": i.e. we don't want a different linker resolution during the second link. Every objects that was selected for the first link should be included in the second link (hence it is wrong to use --start-lib/--end-lib).


I disagree that it should be wrong from a correctness point to do the final link with the same options.

I'm not convinced this is a better approach. It makes the build system's job more complicated as noted earlier, and requires it for correctness. I think it is preferable to have the correctness managed by the compiler itself where it is doing the importing and linkonce resolution in the first place, using the necessary conservative behavior in this situation.

> Also, the distributed build system probably needs to handle the case where an object in the archive was not selected to be part of the link at all, won't be processed by ThinLTO, and there won't be any object to pass to the final link. I'm not sure how you're handling this with gold right now though.


The gold plugin processes all the bitcode files, and gold simply tells it which symbols will be prevailing and which are preempted. So there is always a resulting object file to pass to the final link.


https://reviews.llvm.org/D22356