[gold] Release input files in claim_file

[tbaeder]

Summary

The linker might create several fds per file here and they are not being
closed with ld.bfd. This breaks the build when using clang+ld.bfd to
e.g. link libLLVM.so with LTO.

Gold does not seem to have this problem, but it does not break with this
patch either.

I've also tested this always enabled (not only when thinlto is disabled), and it seemed to work as well.
But the code talks about parallel thinlto backends, so I'm not sure if I've tested all relevant cases.

[tbaeder]

@tejohnson Can you take a look at this?

[tejohnson]

Why aren't they being released at the end of runLTO (via the RAII PluginInputFile objects saved in the HandleToInputFile map) for both thin and regular LTO? I don't understand how this change is correct - we are saving the fd and handle here for use later in runLTO for both thin and regular LTO, but you will be releasing them early for regular lto.

[tbaeder]

In D104230#2831318, @tejohnson wrote:

Why aren't they being released at the end of runLTO (via the RAII PluginInputFile objects saved in the HandleToInputFile map) for both thin and regular LTO? I don't understand how this change is correct - we are saving the fd and handle here for use later in runLTO for both thin and regular LTO, but you will be releasing them early for regular lto.

They are being released, but we can't have that many fds open all at the same time. ld.bfd will call claim_file hundreds of times per file (depending on the file) and we end up with thousands of open fds.

IIRC the handle that's saved is later only used in the thinlto case and only relevant for getSymbolsAndView().

I have tested calling release_input_file() for thinlto as well and it seems to just work if I remove the PluginInfoFile RAII code as well, but I was worried about this from the comments about thinlto and parallel backends.

Note that immediately closing the fd again is basically what ld.bfd is doing for "the/a GCC plugin" but doesn't for the LLVM plugin, which it tries to differentiate here: https://github.com/bminor/binutils-gdb/blob/master/ld/plugin.c#L1229-L1241.

[tejohnson]

In D104230#2832337, @tbaeder wrote:

In D104230#2831318, @tejohnson wrote:

Why aren't they being released at the end of runLTO (via the RAII PluginInputFile objects saved in the HandleToInputFile map) for both thin and regular LTO? I don't understand how this change is correct - we are saving the fd and handle here for use later in runLTO for both thin and regular LTO, but you will be releasing them early for regular lto.

They are being released, but we can't have that many fds open all at the same time. ld.bfd will call claim_file hundreds of times per file (depending on the file) and we end up with thousands of open fds.

Is this in the case of archives? Is bfd creating a new fd for each claim file call on the archive? This seems different than gold. For example, see the comments where we set up LeaderHandle in gold-plugin.cc. We are saving the correspondence between the fd and the handle for the first time we see a given fd.

IIRC the handle that's saved is later only used in the thinlto case and only relevant for getSymbolsAndView().

getSymbolsAndView gets called for every claimed file for both types of LTO in runLTO.

I have tested calling release_input_file() for thinlto as well and it seems to just work if I remove the PluginInfoFile RAII code as well, but I was worried about this from the comments about thinlto and parallel backends.

I'm not sure how this works for gold given that afaik it keeps the same fd from claim file through the subsequent run of LTO which is using the handle to get a view on the file after all symbols are read, regardless of thin vs regular LTO.

Note that immediately closing the fd again is basically what ld.bfd is doing for "the/a GCC plugin" but doesn't for the LLVM plugin, which it tries to differentiate here: https://github.com/bminor/binutils-gdb/blob/master/ld/plugin.c#L1229-L1241.

[tbaeder]

In D104230#2833962, @tejohnson wrote:

In D104230#2832337, @tbaeder wrote:

In D104230#2831318, @tejohnson wrote:

Why aren't they being released at the end of runLTO (via the RAII PluginInputFile objects saved in the HandleToInputFile map) for both thin and regular LTO? I don't understand how this change is correct - we are saving the fd and handle here for use later in runLTO for both thin and regular LTO, but you will be releasing them early for regular lto.

They are being released, but we can't have that many fds open all at the same time. ld.bfd will call claim_file hundreds of times per file (depending on the file) and we end up with thousands of open fds.

Is this in the case of archives? Is bfd creating a new fd for each claim file call on the archive?

This is basically what I'm seeing, yes.

This seems different than gold. For example, see the comments where we set up LeaderHandle in gold-plugin.cc. We are saving the correspondence between the fd and the handle for the first time we see a given fd.

Right, the fds and handles for all the claim_file calls were different so the leader handle and the handle for all the invocations were different.

I have tested calling release_input_file() for thinlto as well and it seems to just work if I remove the PluginInfoFile RAII code as well, but I was worried about this from the comments about thinlto and parallel backends.

I'm not sure how this works for gold given that afaik it keeps the same fd from claim file through the subsequent run of LTO which is using the handle to get a view on the file after all symbols are read, regardless of thin vs regular LTO.

I could imagine that both gold and ld.bfd simply re-open the files on demand in get_view(). But I only tested if this works, not what the linkers do exactly.

Note that immediately closing the fd again is basically what ld.bfd is doing for "the/a GCC plugin" but doesn't for the LLVM plugin, which it tries to differentiate here: https://github.com/bminor/binutils-gdb/blob/master/ld/plugin.c#L1229-L1241.

Removing the conditional here seems to work with ld.bfd for both full LTO and thinLTO as well. Do you think that's a better solution?

[tejohnson]

In D104230#2850131, @tbaeder wrote:

In D104230#2833962, @tejohnson wrote:

In D104230#2832337, @tbaeder wrote:

In D104230#2831318, @tejohnson wrote:

Why aren't they being released at the end of runLTO (via the RAII PluginInputFile objects saved in the HandleToInputFile map) for both thin and regular LTO? I don't understand how this change is correct - we are saving the fd and handle here for use later in runLTO for both thin and regular LTO, but you will be releasing them early for regular lto.

They are being released, but we can't have that many fds open all at the same time. ld.bfd will call claim_file hundreds of times per file (depending on the file) and we end up with thousands of open fds.

Is this in the case of archives? Is bfd creating a new fd for each claim file call on the archive?

This is basically what I'm seeing, yes.

That seems fundamentally different than what gold does.

This seems different than gold. For example, see the comments where we set up LeaderHandle in gold-plugin.cc. We are saving the correspondence between the fd and the handle for the first time we see a given fd.

Right, the fds and handles for all the claim_file calls were different so the leader handle and the handle for all the invocations were different.

Ditto.

I have tested calling release_input_file() for thinlto as well and it seems to just work if I remove the PluginInfoFile RAII code as well, but I was worried about this from the comments about thinlto and parallel backends.

I'm not sure how this works for gold given that afaik it keeps the same fd from claim file through the subsequent run of LTO which is using the handle to get a view on the file after all symbols are read, regardless of thin vs regular LTO.

I could imagine that both gold and ld.bfd simply re-open the files on demand in get_view(). But I only tested if this works, not what the linkers do exactly.

I spent some time looking at gold file handling. AFAICT after drilling down through plugin.cc get_view(), there is no support for re-invoking file opening. It calls Plugin_manager::get_view which accesses an already open file object. Looking through the call chain I don't see any checking for a need to reopen the file. So I'm not really sure how this change is working with gold, but if you want to move forward with it please do an investigation of how in the gold linker code this works.

My view is that LLVM's gold plugin was designed for gold, so we want to avoid any changes that, even if it we can prove it doesn't break, would pessimize the behavior of LTO on gold. Even if gold is somehow able to detect and reopen these files after they are closed eagerly, if it adds overhead I would not be in favor of making this change.

Note that immediately closing the fd again is basically what ld.bfd is doing for "the/a GCC plugin" but doesn't for the LLVM plugin, which it tries to differentiate here: https://github.com/bminor/binutils-gdb/blob/master/ld/plugin.c#L1229-L1241.

Removing the conditional here seems to work with ld.bfd for both full LTO and thinLTO as well. Do you think that's a better solution?

I'm not at all familiar with the bfd linker implementation, but just looking at that comment it isn't clear it would be correct to completely remove that either. I.e. gold keeps one fd per archive function open - can bfd do something similar?

@hjl.tools is the person who implemented the bfd support for it to use llvm's gold plugin. I'd recommend working with him to understand what is happening here and what the best solution is on the ld.bfd side.

[hjl.tools]

Please open a binutils bug with a testcase and CC me. I will take a look.

[tbaeder]

In D104230#2850639, @hjl.tools wrote:

Please open a binutils bug with a testcase and CC me. I will take a look.

I don't have a good testcase but I opened https://sourceware.org/bugzilla/show_bug.cgi?id=28040

[tbaeder]

This got fixed in binutils.

llvm/tools/gold/gold-plugin.cpp

 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 #include <list>
 #include <map>
 #include <plugin-api.h>
 #include <string>
 #include <system_error>
 #include <utility>
 #include <vector>
 
 // FIXME: remove this declaration when we stop maintaining Ubuntu Quantal and
 // Precise and Debian Wheezy (binutils 2.23 is required)
 #define LDPO_PIE 3
handleAllErrors(ObjOrErr.takeError(), [&](const ErrorInfoBase &EI) {
         message(LDPL_FATAL,
                 "LLVM gold plugin has failed to create LTO module: %s",
                 EI.message().c_str());
     });
 
     return *claimed ? LDPS_ERR : LDPS_OK;
   }
 
+  if (!options::thinlto) {
+    // Close the fd now. The linker opens it specifically for this claim_file
+    // call and doesn't close it.
+    // When thinlto is enabled, we need to keep it open but we can still close
+    // it when thinlto is disabled.
+    release_input_file(file->handle);
+  }
+
   std::unique_ptr<InputFile> Obj = std::move(*ObjOrErr);
 
   Modules.emplace_back();
   claimed_file &cf = Modules.back();
 
   cf.handle = file->handle;
   // Keep track of the first handle for each file descriptor, since there are
   // multiple in the case of an archive. This is used later in the case of