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

Wed Jun 30 09:26:16 PDT 2021

tejohnson added a subscriber: hjl.tools.
tejohnson added a comment.

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

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

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.

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

https://reviews.llvm.org/D104230