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

[Timm Bäder via Phabricator via llvm-commits]

tbaeder added a comment.

In D104230#2833962 <https://reviews.llvm.org/D104230#2833962>, @tejohnson wrote:

> 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 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?


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

https://reviews.llvm.org/D104230