[gold/ThinLTO] Restore ThinLTO file management in gold plugin

[tejohnson]

Summary

The gold-plugin changes added along with the new LTO API in r278338 had
the effect of removing the management of the PluginInputFile that
ensured the files weren't released back to gold until the backend
threads were complete. Add back the old file handling.

Fixes PR29020.

[mehdi_amini]

tools/gold/gold-plugin.cpp

833 ↗

(On Diff #68715)

Fo you need a map here? Can't you just push back to a vector?

[tejohnson]

tools/gold/gold-plugin.cpp

833 ↗

(On Diff #68715)

This is necessary for correct file locking when we have archives. See the first part of the summary for r270814. Specifically, ensure we only attempt to lock and unlock the underlying archive file once.

[mehdi_amini]

LGTM.

tools/gold/gold-plugin.cpp

833 ↗

(On Diff #68715)

For some reason I missed the HandleToInputFile.count(..) the first time I read this. It makes sense now.

[pcc]

This change doesn't seem right to me. If the link command has ThinLTO inputs but does not pass in the thinlto plugin option, we will release files early.

Regrettably I think we may need to move some of the memory management into the lib/LTO interface.

[tejohnson]

In D23721#543923, @pcc wrote:

This change doesn't seem right to me. If the link command has ThinLTO inputs but does not pass in the thinlto plugin option, we will release files early.

Good point (before the new API gold would only handle the file as ThinLTO with the plugin option).

Regrettably I think we may need to move some of the memory management into the lib/LTO interface.

Since this is a gold-specific issue (particularly the way it ensures that an archive is only opened once), how about we simply check whether it is a ThinLTO file here in place of using the option? I know it is conceptually nice to not have to check the IR object type, but I think the file management is better left in the client.

[mehdi_amini]

It seems to me that this should be part of the contract of the API: at the moment all the input buffers are supposed to stay available for the duration of the process.
This is not an issue for mapped file I believe (which I guess every clients are using right now?), but a client that have to decompress buffers in memory before calling the API may be happy to have the ability to release buffers early. We could rework the InputFile to carry the lifetime if needed.

[pcc]

I agree with Mehdi here.

One possibility would be to change InputFile to store a MemoryBuffer instead of a MemoryBufferRef. Clients that require special handling of freed InputFiles can supply a custom derived class of MemoryBuffer with an overridden destructor.

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

 ; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
 ; RUN:    --plugin-opt=save-temps \
 ; RUN:    --plugin-opt=thinlto \
 ; RUN:    --plugin-opt=jobs=1 \
 ; RUN:    -shared %t.o %t2.o -o %t4
 ; RUN: llvm-bcanalyzer -dump %t4.index.bc | FileCheck %s --check-prefix=COMBINED
 ; RUN: llvm-nm %t4 | FileCheck %s --check-prefix=NM
 
+; Check with --no-map-whole-files
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=save-temps \
+; RUN:    --plugin-opt=thinlto \
+; RUN:    --plugin-opt=jobs=1 \
+; RUN:    --no-map-whole-files \
+; RUN:    -shared %t.o %t2.o -o %t4
+; RUN: llvm-bcanalyzer -dump %t4.index.bc | FileCheck %s --check-prefix=COMBINED
+; RUN: llvm-nm %t4 | FileCheck %s --check-prefix=NM
+
 ; Next force multi-threaded mode
 ; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
 ; RUN:    --plugin-opt=save-temps \
 ; RUN:    --plugin-opt=thinlto \
 ; RUN:    --plugin-opt=jobs=2 \
 ; RUN:    -shared %t.o %t2.o -o %t4
 ; RUN: llvm-bcanalyzer -dump %t4.index.bc | FileCheck %s --check-prefix=COMBINED
 ; RUN: llvm-nm %t4 | FileCheck %s --check-prefix=NM

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

 /// native object file. Then, perform optimization and codegen.
 static ld_plugin_status allSymbolsReadHook() {
   if (Modules.empty())
     return LDPS_OK;
 
   if (unsigned NumOpts = options::extra.size())
     cl::ParseCommandLineOptions(NumOpts, &options::extra[0]);
 
+  // Map to own RAII objects that manage the file opening and releasing
+  // interfaces with gold. This is needed only for ThinLTO mode, since
+  // unlike regular LTO, where addModule will result in the opened file
+  // being merged into a new combined module, we need to keep these files open
+  // through Lto->run().
+  DenseMap<void *, std::unique_ptr<PluginInputFile>> HandleToInputFile;
+
   std::unique_ptr<LTO> Lto = createLTO();
 
   for (claimed_file &F : Modules) {
-    PluginInputFile InputFile(F.handle);
+    if (options::thinlto && !HandleToInputFile.count(F.leader_handle))
+      HandleToInputFile.insert(std::make_pair(
+          F.leader_handle, llvm::make_unique<PluginInputFile>(F.handle)));
     const void *View = getSymbolsAndView(F);
     if (!View)
       continue;
     addModule(*Lto, F, View);
   }
 
   SmallString<128> Filename;
   // Note that getOutputFileName will append a unique ID for each task