[PATCH] D23680: [ThinLTO] Emit files for distributed builds for all modules

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: tools/gold/gold-plugin.cpp:755
@@ -754,1 +754,3 @@
 
+// Write empty files that may be expected by a distributed build
+// system when invoked with thinlto_index_only. This is invoked when
----------------
pcc wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > tejohnson wrote:
> > > > pcc wrote:
> > > > > I am not sure if this is needed, but even if it is, can't this be done inside lib/LTO?
> > > > This is needed because the distributed build system (in this case Bazel) wants to check that its expected outputs were generated. It doesn't know what the linker decided to include in the final link or not. This was a regression from the earlier behavior before we moved to the new LTO API.
> > > > 
> > > > My earlier version of the patch had this fix in lib/LTO, but Mehdi expressed a preference (in an offline discussion) to move this into the client.
> > > Right: it seems "client specific" as it is a constraint that comes from Bazel.
> > Although note that Bazel is independent of gold. It just happens that we build with using the gold linker and the Bazel distributed build system.
> When you say "It doesn't know what the linker decided to include" are you referring to which archive members the linker chose?
> 
> So the regression is that the distributed build is missing non-ThinLTO objects from archives, and the fix is to create empty .thinlto.bc files to cause the build system to include the non-ThinLTO objects in the final link?
> 
> Or am I misunderstanding?
Correct, it is to deal with the fact that with archives (--start-lib/--end-lib in our case), gold will not include some of the files in the final link. Those are not passed to the backend and no ThinLTO outputs are created for them. 

We do emit the list of files to include in the final link (into the thinlto_linked_objects_file, which is an optional file provided to the thinlto-index-only= option), and that ends up informing the final link done by Bazel (essentially passed in via the @ linker option).

However, the issue here is with the way Bazel does sanity checking of expected outputs - all it knows when it kicks off the ThinLink action is that it is passing a set of object (IR) files to the Thin Link action, and each action needs to be told what output files it should bring back. The lower level job handling checks that these expected outputs are created and fetches them back. With the current LTO code, that contract is being broken, because not all expected outputs are being created. 


https://reviews.llvm.org/D23680