[ThinLTO/gold] Support for getting list of included objects from gold

[tejohnson]

Summary

In the distributed backend case, the ThinLink step and the final native
object link are separate processes. This can be problematic when archive
libraries are involved in the link (e.g. via --start-lib/--end-lib
pairs). The linker only includes objects from libraries when
there is a strong reference to them, and depending on the intervening
ThinLTO backend processes' importing/inlining, the strong references
may appear different in the two link steps. See D22356 and D22467
for two scenarios where this causes issues.

To ensure that the final link includes the same objects, this patch
adds support for an "=filename" form of the thinlto-index-only plugin
option, in which case objects gold included in the link are emitted to
the given filename. This should be used as input to the final link (e.g.
via the @filename option to gold), instead of listing all the objects
within --start-lib/--end-lib pairs again.

Note that the support for the gold callback that identifies included
objects was added in gold version 1.12.

[davidxl]

How about adding functional test cases to cover issues mentioned in the mentioned previous reviews?

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

Perhaps add test case about real archive case as well.

21 ↗

(On Diff #65073)

Can the name between bitcode file and final object files be made more related ?

[tejohnson]

In D22677#492908, @davidxl wrote:

How about adding functional test cases to cover issues mentioned in the mentioned previous reviews?

Because the support for the callback returning the LDPS_NO_SYMS value was added only recently, and there isn't a way to guard against an older version of gold in the tests (mentioned in the comment within the new test).

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

For a distributed build the build system needs to extract the constituent objects (otherwise the combined index paths will not point to an object file)

21 ↗

(On Diff #65073)

The ThinLink gold invocation doesn't know the name of the final object files. But note that if a prefix replace path is specified (thinlto-prefix-replace=oldprefix:newprefix plugin option), then the names emitted will have the matching prefix path replaced with the new one. This can be used to get the paths to the final object files into the file, as long as the final objects use the same names but a different path (true in bazel). Otherwise the build system would need to do post-processing of the list to map to the final object files it is going to create - known by the build system but not by gold here.

[davidxl]

For the functional case, I meant the tests in D22356 -- that with this patch we don't end up with unsats in second link.

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

I think this feature is more about two step link (not about a particular distributed build system). It is possible that some other distributed build system uses archive, right?

21 ↗

(On Diff #65073)

This request is more about test case readability. Suppose --start-lib and --end-lib pair includes two bit code files, but one of them is dropped in the first link. In the second step, we need to be able to tell which one maps to the kept one..

[tejohnson]

In D22677#492930, @davidxl wrote:

For the functional case, I meant the tests in D22356 -- that with this patch we don't end up with unsats in second link.

Right, for this patch to fix that case it needs gold v1.12, and there isn't a way to require a certain gold version on the bots.

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

Using archives directly with a distributed build won't work - the combined index module paths need to point to an object file for the distributed backends to import from. The build system needs to extract the objects (e.g. into a temp dir) and pass those.

Note that archives passed to a non-distributed build work fine because we load the modules from the offset within the archive passed to the plugin from gold and serve those to the importer.

21 ↗

(On Diff #65073)

Is the concern that it isn't obvious how %t.o and %t2.o map to lines in the CHECK? Note though that both are included in the link due to the gold version issue mentioned in the comments, so it should be pretty clear in this test case I think?

[davidxl]

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

how about thin archive?

21 ↗

(On Diff #65073)

yes for this case it is clear more or less. A more general concern is that if we have a very large link that has a bug in the command line, how do we debug the problem? Whatever debugging facility we can use to trace the native .o back to the bitcode file can be used in tests like this.

[tejohnson]

I thought one of the issue is that the first link does not drop any, but the second link drops it if the same command line is used. If the first link does not drop any, is v1.12 still needed?

Ah, good point, I had forgotten that the issue there was that we don't want to drop one file (and always include all the rest). (In some cases you can get into trouble if you include all objects from the archives, but not there).

That being said, since the patch no longer addresses the specific instance in that test case, I think it is a bit too heavy-weight - we know that it is fixed by including that second file in the link, and the test case I added here ensures that the file does include objects from libraries that are needed by the link. So my preference is not to include it here, but if you feel strongly I will add it too.

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

Gold does not seem to like thin archives (regardless of LTO vs ThinLTO). I get an error (from gold, not the plugin) like:

error: foo.a: no archive symbol table (run ranlib)

(even when I run ranlib)

21 ↗

(On Diff #65073)

I'm not following - that mechanism has to be in the build system. The separate processes in the distributed backend case mean that the two invocations of gold don't have a way to identify what the intervening clang invocations use as the output native object file name (the backend clang processes take the bitcode file and the index produced by the thin link as input, and an output file name, and invokes the importing/optimization pipelines).

[tejohnson]

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

Note this issue is specific to bitcode files in the archive - gold with regular native objects in a thin archive works just fine. Looks like ranlib doesn't work on an archive of bitcode files, and that seems to be needed with a thin archive.

[mehdi_amini]

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

Do you mean "ranlib doesn't work on a *thin* archive of bitcode files"?
Otherwise how do you build LLVM itself with LTO?

[tejohnson]

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

According to http://llvm.org/docs/GoldPlugin.html it isn't supported for archives of bitcode files at all, but apparently isn't needed for a whole (non-thin) archive:

"export RANLIB=/bin/true #ranlib is not needed, and doesn't support .bc files in .a"

[tejohnson]

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

So I can't get gold to work with a (non-thin) archive created by ar, it gives the same error. Maybe it needs to take the -plugin option? In any case, a non-thin archive created via llvm-ar is accepted and works fine. I don't see an llvm-ar option to create a thin archive though?

[davidxl]

this patch lgtm (with potential follow ups for additional tests)

[tejohnson]

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

Aha - llvm-ar does support thin archives, it's just that the T option is not documented in the help. =)

With that, it does work for llvm-ar thin archives! Will add another case to the new test that does the same for thin archives before submitting.

[mehdi_amini]

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

According to http://llvm.org/docs/GoldPlugin.html it isn't supported for archives of bitcode files at all.

Documentation patch?

[tejohnson]

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

Reading through that page some more, it gives instructions earlier on getting the plugin to work with ar:
"ar and nm also accept the -plugin option and it’s possible to to install LLVMgold.so to /usr/lib/bfd-plugins for a seamless setup. If you built your own gold, be sure to install the ar and nm-new you built to /usr/bin."

So I think it is ok. I just took the latter comment out of context.

However, I will send a patch to document the llvm-ar T modifier

[tejohnson]

test/tools/gold/X86/thinlto_emit_linked_objects.ll

18 ↗

(On Diff #65073)

With that, it does work for llvm-ar thin archives! Will add another case to the new test that does the same for thin archives before submitting.

And, removed in r276453 due to bot failure - looks like gold v1.11 does not like the thin archive (gold v1.12 is fine though!).

llvm/trunk/test/tools/gold/X86/Inputs/thinlto_emit_linked_objects.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @foo() {
+entry:
+  ret void
+}

llvm/trunk/test/tools/gold/X86/thinlto_emit_linked_objects.ll

+; First generate bitcode with a module summary index for each file
+; RUN: opt -module-summary %s -o %t.o
+; RUN: opt -module-summary %p/Inputs/thinlto_emit_linked_objects.ll -o %t2.o
+
+; Next do the ThinLink step, specifying thinlto-index-only so that the gold
+; plugin exits after generating individual indexes. The objects the linker
+; decided to include in the link should be emitted into the file specified
+; after 'thinlto-index-only='. Note that in this test both files should
+; be included in the link, but in a case where there was an object in
+; a library that had no strongly referenced symbols, that file would not
+; be included in the link and listed in the emitted file. However, this
+; requires gold version 1.12.
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=thinlto \
+; RUN:    --plugin-opt=thinlto-index-only=%t3 \
+; RUN:    -o %t5 \
+; RUN:    %t.o \
+; RUN:    --start-lib %t2.o --end-lib
+
+; Do the same check for a thin archive (note that non-thin archives don't play
+; well with thinlto-index-only because we need to have an object file to
+; import from in the distributed ThinLTO backends).
+; RUN: llvm-ar Tr %t2.a %t2.o
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=thinlto \
+; RUN:    --plugin-opt=thinlto-index-only=%t4 \
+; RUN:    -o %t5 \
+; RUN:    %t.o \
+; RUN:    %t2.a
+
+; RUN: cat %t3 | FileCheck %s
+; RUN: cat %t4 | FileCheck %s
+; CHECK: thinlto_emit_linked_objects.ll.tmp.o
+; CHECK: thinlto_emit_linked_objects.ll.tmp2.o
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @main() {
+entry:
+  call void (...) @foo()
+  ret i32 0
+}
+
+declare void @foo(...)

llvm/trunk/tools/gold/gold-plugin.cpp

#endif
   // global index for the ThinLTO backends.
   static bool thinlto = false;
   // If false, all ThinLTO backend compilations through code gen are performed
   // using multiple threads in the gold-plugin, before handing control back to
   // gold. If true, write individual backend index files which reflect
   // the import decisions, and exit afterwards. The assumption is
   // that the build system will launch the backend processes.
   static bool thinlto_index_only = false;
+  // If non-empty, holds the name of a file in which to write the list of
+  // oject files gold selected for inclusion in the link after symbol
+  // resolution (i.e. they had selected symbols). This will only be non-empty
+  // in the thinlto_index_only case. It is used to identify files, which may
+  // have originally been within archive libraries specified via
+  // --start-lib/--end-lib pairs, that should be included in the final
+  // native link process (since intervening function importing and inlining
+  // may change the symbol resolution detected in the final link and which
+  // files to include out of --start-lib/--end-lib libraries as a result).
+  static std::string thinlto_linked_objects_file;
   // If true, when generating individual index files for distributed backends,
   // also generate a "${bitcodefile}.imports" file at the same location for each
   // bitcode file, listing the files it imports from in plain text. This is to
   // support distributed build file staging.
   static bool thinlto_emit_imports_files = false;
   // Option to control where files for a distributed backend (the individual
   // index files and optional imports files) are created.
   // If specified, expects a string of the form "oldprefix:newprefix", and
static void process_plugin_option(const char *opt_)
     } else if (opt == "save-temps") {
       TheOutputType = OT_SAVE_TEMPS;
     } else if (opt == "disable-output") {
       TheOutputType = OT_DISABLE;
     } else if (opt == "thinlto") {
       thinlto = true;
     } else if (opt == "thinlto-index-only") {
       thinlto_index_only = true;
+    } else if (opt.startswith("thinlto-index-only=")) {
+      thinlto_index_only = true;
+      thinlto_linked_objects_file = opt.substr(strlen("thinlto-index-only="));
     } else if (opt == "thinlto-emit-imports-files") {
       thinlto_emit_imports_files = true;
     } else if (opt.startswith("thinlto-prefix-replace=")) {
       thinlto_prefix_replace = opt.substr(strlen("thinlto-prefix-replace="));
       if (thinlto_prefix_replace.find(";") == std::string::npos)
         message(LDPL_FATAL, "thinlto-prefix-replace expects 'old;new' format");
     } else if (opt.size() == 2 && opt[0] == 'O') {
       if (opt[1] < '0' || opt[1] > '3')
message(LDPL_WARNING,
             "thinlto-emit-imports-files ignored unless thinlto-index-only");
 
   if (options::thinlto_index_only) {
     // If the thinlto-prefix-replace option was specified, parse it and
     // extract the old and new prefixes.
     std::string OldPrefix, NewPrefix;
     getThinLTOOldAndNewPrefix(OldPrefix, NewPrefix);
 
+    // If the user requested a list of objects gold included in the link,
+    // create and open the requested file.
+    raw_fd_ostream *ObjFileOS = nullptr;
+    if (!options::thinlto_linked_objects_file.empty()) {
+      std::error_code EC;
+      ObjFileOS = new raw_fd_ostream(options::thinlto_linked_objects_file, EC,
+                                     sys::fs::OpenFlags::F_None);
+      if (EC)
+        message(LDPL_FATAL, "Unable to open %s for writing: %s",
+                options::thinlto_linked_objects_file.c_str(),
+                EC.message().c_str());
+    }
     // For each input bitcode file, generate an individual index that
     // contains summaries only for its own global values, and for any that
     // should be imported.
     for (claimed_file &F : Modules) {
       std::error_code EC;
 
       std::string NewModulePath =
           getThinLTOOutputFile(F.name, OldPrefix, NewPrefix);
+
+      if (!options::thinlto_linked_objects_file.empty()) {
+        // If gold included any symbols from ths file in the link, emit path
+        // to the final object file, which should be included in the final
+        // native link.
+        if (get_symbols(F.handle, F.syms.size(), F.syms.data()) !=
+            LDPS_NO_SYMS) {
+          assert(ObjFileOS);
+          *ObjFileOS << NewModulePath << "\n";
+        }
+      }
+
       raw_fd_ostream OS((Twine(NewModulePath) + ".thinlto.bc").str(), EC,
                         sys::fs::OpenFlags::F_None);
       if (EC)
         message(LDPL_FATAL, "Unable to open %s.thinlto.bc for writing: %s",
                 NewModulePath.c_str(), EC.message().c_str());
       // Build a map of module to the GUIDs and summary objects that should
       // be written to its index.
       std::map<std::string, GVSummaryMapTy> ModuleToSummariesForIndex;
       gatherImportedSummariesForModule(F.name, ModuleToDefinedGVSummaries,
                                        ImportLists, ModuleToSummariesForIndex);
       WriteIndexToFile(CombinedIndex, OS, &ModuleToSummariesForIndex);
 
       if (options::thinlto_emit_imports_files) {
         if ((EC = EmitImportsFiles(F.name,
                                    (Twine(NewModulePath) + ".imports").str(),
                                    ImportLists)))
           message(LDPL_FATAL, "Unable to open %s.imports",
                   NewModulePath.c_str(), EC.message().c_str());
       }
     }
 
+    if (ObjFileOS)
+      ObjFileOS->close();
+
     cleanup_hook();
     exit(0);
   }
 
   // Create OS in nested scope so that it will be closed on destruction.
   {
     std::error_code EC;
     raw_fd_ostream OS(output_name + ".thinlto.bc", EC,