PR lto/64837: lto plugin doesn't call ld_plugin_release_input_file

On Wed, Jan 28, 2015 at 11:37 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Jan 28, 2015 at 11:19 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On January 28, 2015 7:12:43 PM CET, "H.J. Lu" <hongjiu.lu@intel.com> wrote:
>>>Hi,
>>>
>>>This patch makes claim_file_handler to call release_input_file after it
>>>finishes processing input file.  OK for trunk?
>>
>> OK.  How did you test this?
>
> I did normal bootstrap and "make check" on Linux/x86-64.
> I also run ld.bfd and ld.gold by hand to verify that release_input_file
> is called.
>

This is needed for LTO build.  ar/nm/ranlib don't provide
release_input_file.  I checked it in as an obvious fix.

The plugin is not supposed to call release_input_file from the
claim_file handler. That interface is only for releasing a file
descriptor obtained via get_input_file during the all_symbols_read
callback. When the linker calls the claim_file handler, the file
descriptor is open, and the plugin is required to leave it open; the
linker manages the file descriptor at that point. The
get_input_file/release_input_file pair of interfaces was added later,
for the benefit of another (non-LTO) plugin (although I think the LLVM
LTO plugin does use that pair during the all_symbols_read callback).

This is described here:

   https://gcc.gnu.org/wiki/whopr/driver

If you're going to insist on calling the release_input_file API from
the claim_file handler, I'm going to have to fix gold to ignore the
call to avoid a premature unlock of the object file.

-cary



On Wed, Jan 28, 2015 at 4:02 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Jan 28, 2015 at 11:37 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, Jan 28, 2015 at 11:19 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On January 28, 2015 7:12:43 PM CET, "H.J. Lu" <hongjiu.lu@intel.com> wrote:
>>>>Hi,
>>>>
>>>>This patch makes claim_file_handler to call release_input_file after it
>>>>finishes processing input file.  OK for trunk?
>>>
>>> OK.  How did you test this?
>>
>> I did normal bootstrap and "make check" on Linux/x86-64.
>> I also run ld.bfd and ld.gold by hand to verify that release_input_file
>> is called.
>>
>
> This is needed for LTO build.  ar/nm/ranlib don't provide
> release_input_file.  I checked it in as an obvious fix.
>
> --
> H.J.
> ---
> Index: ChangeLog
> ===================================================================
> --- ChangeLog (revision 220212)
> +++ ChangeLog (working copy)
> @@ -1,5 +1,10 @@
>  2015-01-28  H.J. Lu  <hongjiu.lu@intel.com>
>
> + * lto-plugin.c (claim_file_handler): Call release_input_file only
> + if it is not NULL.
> +
> +2015-01-28  H.J. Lu  <hongjiu.lu@intel.com>
> +
>   PR lto/64837
>   * lto-plugin.c (release_input_file): New.
>   (claim_file_handler): Call release_input_file.
> Index: lto-plugin.c
> ===================================================================
> --- lto-plugin.c (revision 220212)
> +++ lto-plugin.c (working copy)
> @@ -1007,7 +1007,8 @@ claim_file_handler (const struct ld_plug
>    if (obj.objfile)
>      simple_object_release_read (obj.objfile);
>
> -  release_input_file (file);
> +  if (release_input_file)
> +    release_input_file (file);
>
>    return LDPS_OK;
>  }

On February 4, 2015 5:49:13 AM CET, Cary Coutant <ccoutant@google.com> wrote:
>The plugin is not supposed to call release_input_file from the
>claim_file handler. That interface is only for releasing a file
>descriptor obtained via get_input_file during the all_symbols_read
>callback. When the linker calls the claim_file handler, the file
>descriptor is open, and the plugin is required to leave it open; the
>linker manages the file descriptor at that point. The
>get_input_file/release_input_file pair of interfaces was added later,
>for the benefit of another (non-LTO) plugin (although I think the LLVM
>LTO plugin does use that pair during the all_symbols_read callback).
>
>This is described here:
>
>   https://gcc.gnu.org/wiki/whopr/driver
>
>If you're going to insist on calling the release_input_file API from
>the claim_file handler, I'm going to have to fix gold to ignore the
>call to avoid a premature unlock of the object file.

What's the proper solution for not leaking those filedescriptors?

Richard.

>-cary
>
>
>
>On Wed, Jan 28, 2015 at 4:02 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, Jan 28, 2015 at 11:37 AM, H.J. Lu <hjl.tools@gmail.com>
>wrote:
>>> On Wed, Jan 28, 2015 at 11:19 AM, Richard Biener
>>> <richard.guenther@gmail.com> wrote:
>>>> On January 28, 2015 7:12:43 PM CET, "H.J. Lu"
><hongjiu.lu@intel.com> wrote:
>>>>>Hi,
>>>>>
>>>>>This patch makes claim_file_handler to call release_input_file
>after it
>>>>>finishes processing input file.  OK for trunk?
>>>>
>>>> OK.  How did you test this?
>>>
>>> I did normal bootstrap and "make check" on Linux/x86-64.
>>> I also run ld.bfd and ld.gold by hand to verify that
>release_input_file
>>> is called.
>>>
>>
>> This is needed for LTO build.  ar/nm/ranlib don't provide
>> release_input_file.  I checked it in as an obvious fix.
>>
>> --
>> H.J.
>> ---
>> Index: ChangeLog
>> ===================================================================
>> --- ChangeLog (revision 220212)
>> +++ ChangeLog (working copy)
>> @@ -1,5 +1,10 @@
>>  2015-01-28  H.J. Lu  <hongjiu.lu@intel.com>
>>
>> + * lto-plugin.c (claim_file_handler): Call release_input_file only
>> + if it is not NULL.
>> +
>> +2015-01-28  H.J. Lu  <hongjiu.lu@intel.com>
>> +
>>   PR lto/64837
>>   * lto-plugin.c (release_input_file): New.
>>   (claim_file_handler): Call release_input_file.
>> Index: lto-plugin.c
>> ===================================================================
>> --- lto-plugin.c (revision 220212)
>> +++ lto-plugin.c (working copy)
>> @@ -1007,7 +1007,8 @@ claim_file_handler (const struct ld_plug
>>    if (obj.objfile)
>>      simple_object_release_read (obj.objfile);
>>
>> -  release_input_file (file);
>> +  if (release_input_file)
>> +    release_input_file (file);
>>
>>    return LDPS_OK;
>>  }

>>If you're going to insist on calling the release_input_file API from
>>the claim_file handler, I'm going to have to fix gold to ignore the
>>call to avoid a premature unlock of the object file.
>
> What's the proper solution for not leaking those filedescriptors?

There was a bug in gold where it wasn't unlocking external members of
thin archives that got claimed by the plugin. See PR 15660:

   https://sourceware.org/bugzilla/show_bug.cgi?id=15660

-cary

On Wed, Feb 4, 2015 at 7:35 AM, Cary Coutant <ccoutant@google.com> wrote:
>>>If you're going to insist on calling the release_input_file API from
>>>the claim_file handler, I'm going to have to fix gold to ignore the
>>>call to avoid a premature unlock of the object file.
>>
>> What's the proper solution for not leaking those filedescriptors?
>
> There was a bug in gold where it wasn't unlocking external members of
> thin archives that got claimed by the plugin. See PR 15660:
>
>    https://sourceware.org/bugzilla/show_bug.cgi?id=15660
>

FWIW, ld doesn't have the file descriptor leak since it does

  /* fd belongs to us, not the plugin; but we don't need it.  */
  close (file->fd);

after calling claim_file_handler.  It only works with GCC plug-in
library.  I going to change it:

https://sourceware.org/ml/binutils/2015-02/msg00001.html