diff mbox

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

Message ID CAMe9rOpT6-1rcnW7g+TB0Sy-aZY+KPDyfo=jKS=ers9YkqDm4A@mail.gmail.com
State New
Headers show

Commit Message

H.J. Lu Feb. 5, 2015, 8:57 p.m. UTC
On Thu, Feb 5, 2015 at 8:42 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> 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;
>>  }
>
> We should call release_input_file only if file is claimed.  Otherwise,
> ld will close file descriptor of non-IR file. I checked it in as obvious fix.
>
> --
> H.J.
> ---
> Index: ChangeLog
> ===================================================================
> --- ChangeLog (revision 220454)
> +++ ChangeLog (working copy)
> @@ -1,3 +1,8 @@
> +2015-02-05  H.J. Lu  <hongjiu.lu@intel.com>
> +
> + * lto-plugin.c (claim_file_handler): Call release_input_file only
> + if file is claimed.
> +
>  2015-01-28  H.J. Lu  <hongjiu.lu@intel.com>
>
>   * lto-plugin.c (claim_file_handler): Call release_input_file only
> Index: lto-plugin.c
> ===================================================================
> --- lto-plugin.c (revision 220454)
> +++ lto-plugin.c (working copy)
> @@ -998,6 +998,9 @@ claim_file_handler (const struct ld_plug
>
>    *claimed = 1;
>
> +  if (release_input_file)
> +    release_input_file (file);
> +
>    goto cleanup;
>
>   err:
> @@ -1007,9 +1010,6 @@ claim_file_handler (const struct ld_plug
>    if (obj.objfile)
>      simple_object_release_read (obj.objfile);
>
> -  if (release_input_file)
> -    release_input_file (file);
> -
>    return LDPS_OK;
>  }

We should pass handle, not file, to release_input_file.
I checked it in as an obvious fix.

Comments

Markus Trippelsdorf Feb. 6, 2015, 12:56 p.m. UTC | #1
On 2015.02.05 at 12:57 -0800, H.J. Lu wrote:
> 
> We should pass handle, not file, to release_input_file.
> I checked it in as an obvious fix.

This commit causes:

 % echo "int main () {}" | gcc -fuse-ld=gold -flto -x c++ -
ld.gold: internal error in remove_writer, at token.h:132
collect2: error: ld returned 1 exit status
H.J. Lu Feb. 6, 2015, 1:10 p.m. UTC | #2
On Fri, Feb 6, 2015 at 4:56 AM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2015.02.05 at 12:57 -0800, H.J. Lu wrote:
>>
>> We should pass handle, not file, to release_input_file.
>> I checked it in as an obvious fix.
>
> This commit causes:
>
>  % echo "int main () {}" | gcc -fuse-ld=gold -flto -x c++ -
> ld.gold: internal error in remove_writer, at token.h:132
> collect2: error: ld returned 1 exit status
>

ld.bfd closes fd without release_input_file being called
and ld.gold leaks file descriptors if release_input_file
isn't called.  We can either fix gold:

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

or revert the PR lto/64837 fix.
Markus Trippelsdorf Feb. 6, 2015, 1:18 p.m. UTC | #3
On 2015.02.06 at 05:10 -0800, H.J. Lu wrote:
> On Fri, Feb 6, 2015 at 4:56 AM, Markus Trippelsdorf
> <markus@trippelsdorf.de> wrote:
> > On 2015.02.05 at 12:57 -0800, H.J. Lu wrote:
> >>
> >> We should pass handle, not file, to release_input_file.
> >> I checked it in as an obvious fix.
> >
> > This commit causes:
> >
> >  % echo "int main () {}" | gcc -fuse-ld=gold -flto -x c++ -
> > ld.gold: internal error in remove_writer, at token.h:132
> > collect2: error: ld returned 1 exit status
> >
> 
> ld.bfd closes fd without release_input_file being called
> and ld.gold leaks file descriptors if release_input_file
> isn't called.  We can either fix gold:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=17896
> 
> or revert the PR lto/64837 fix.

Given that LTO would be completely broken with any gold version in
gcc-5, a revert seems to be the only viable option.
H.J. Lu Feb. 6, 2015, 1:30 p.m. UTC | #4
On Fri, Feb 6, 2015 at 5:18 AM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2015.02.06 at 05:10 -0800, H.J. Lu wrote:
>> On Fri, Feb 6, 2015 at 4:56 AM, Markus Trippelsdorf
>> <markus@trippelsdorf.de> wrote:
>> > On 2015.02.05 at 12:57 -0800, H.J. Lu wrote:
>> >>
>> >> We should pass handle, not file, to release_input_file.
>> >> I checked it in as an obvious fix.
>> >
>> > This commit causes:
>> >
>> >  % echo "int main () {}" | gcc -fuse-ld=gold -flto -x c++ -
>> > ld.gold: internal error in remove_writer, at token.h:132
>> > collect2: error: ld returned 1 exit status
>> >
>>
>> ld.bfd closes fd without release_input_file being called
>> and ld.gold leaks file descriptors if release_input_file
>> isn't called.  We can either fix gold:
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=17896
>>
>> or revert the PR lto/64837 fix.
>
> Given that LTO would be completely broken with any gold version in
> gcc-5, a revert seems to be the only viable option.
>

Fine with me and reopen PR lto/64837.
H.J. Lu Feb. 6, 2015, 1:56 p.m. UTC | #5
On Fri, Feb 6, 2015 at 5:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Feb 6, 2015 at 5:18 AM, Markus Trippelsdorf
> <markus@trippelsdorf.de> wrote:
>> On 2015.02.06 at 05:10 -0800, H.J. Lu wrote:
>>> On Fri, Feb 6, 2015 at 4:56 AM, Markus Trippelsdorf
>>> <markus@trippelsdorf.de> wrote:
>>> > On 2015.02.05 at 12:57 -0800, H.J. Lu wrote:
>>> >>
>>> >> We should pass handle, not file, to release_input_file.
>>> >> I checked it in as an obvious fix.
>>> >
>>> > This commit causes:
>>> >
>>> >  % echo "int main () {}" | gcc -fuse-ld=gold -flto -x c++ -
>>> > ld.gold: internal error in remove_writer, at token.h:132
>>> > collect2: error: ld returned 1 exit status
>>> >
>>>
>>> ld.bfd closes fd without release_input_file being called
>>> and ld.gold leaks file descriptors if release_input_file
>>> isn't called.  We can either fix gold:
>>>
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=17896
>>>
>>> or revert the PR lto/64837 fix.
>>
>> Given that LTO would be completely broken with any gold version in
>> gcc-5, a revert seems to be the only viable option.
>>
>
> Fine with me and reopen PR lto/64837.
>

Done with r220477.
diff mbox

Patch

Index: ChangeLog
===================================================================
--- ChangeLog (revision 220455)
+++ ChangeLog (working copy)
@@ -1,5 +1,10 @@ 
 2015-02-05  H.J. Lu  <hongjiu.lu@intel.com>

+ * lto-plugin.c (claim_file_handler): Pass handle to
+ release_input_file.
+
+2015-02-05  H.J. Lu  <hongjiu.lu@intel.com>
+
  * lto-plugin.c (claim_file_handler): Call release_input_file only
  if file is claimed.

Index: lto-plugin.c
===================================================================
--- lto-plugin.c (revision 220455)
+++ lto-plugin.c (working copy)
@@ -999,7 +999,7 @@  claim_file_handler (const struct ld_plug
   *claimed = 1;

   if (release_input_file)
-    release_input_file (file);
+    release_input_file (file->handle);

   goto cleanup;