[PATCH] D104230: [gold] Release input files in claim_file

Tue Jun 22 11:30:19 PDT 2021

tejohnson added a comment.

In D104230#2832337 <https://reviews.llvm.org/D104230#2832337>, @tbaeder wrote:

> In D104230#2831318 <https://reviews.llvm.org/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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104230/new/

https://reviews.llvm.org/D104230