Patchwork Fix PR middle-end/52894

login
register
mail settings
Submitter John David Anglin
Date April 10, 2012, 12:45 a.m.
Message ID <20120410004546.GA28872@hiauly1.hia.nrc.ca>
Download mbox | patch
Permalink /patch/151503/
State New
Headers show

Comments

John David Anglin - April 10, 2012, 12:45 a.m.
The current 4.5, 4.6 and 4.7 branches are now seriously broken for all
PA target due to the fix applied for PR middle-end/52640, an optimization
fix.  This is because the PA backend defers output of function descriptors
and externals (hpux) using TARGET_ASM_FILE_END.  As a result, assemble_external
can be called after after varasm.c processes it's list of pending externals
and the vector used for this list deleted.

The attached change fixes this problem.  The change has been tested
on hppa-unknown-linux-gnu and hppa2.0w-hp-hpux11.11 with no observed
regressions.  I believe it to be the simplest way to fix these release
branches although it may not be optimal (externals are doublely delayed
on PA hpux).

Ok for the 4.5, 4.6 and 4.7 branches?  The fix for PR middle-end/52640
was not applied to the trunk.

Dave
Richard Guenther - April 10, 2012, 11:06 a.m.
On Tue, Apr 10, 2012 at 2:45 AM, John David Anglin
<dave@hiauly1.hia.nrc.ca> wrote:
> The current 4.5, 4.6 and 4.7 branches are now seriously broken for all
> PA target due to the fix applied for PR middle-end/52640, an optimization
> fix.  This is because the PA backend defers output of function descriptors
> and externals (hpux) using TARGET_ASM_FILE_END.  As a result, assemble_external
> can be called after after varasm.c processes it's list of pending externals
> and the vector used for this list deleted.
>
> The attached change fixes this problem.  The change has been tested
> on hppa-unknown-linux-gnu and hppa2.0w-hp-hpux11.11 with no observed
> regressions.  I believe it to be the simplest way to fix these release
> branches although it may not be optimal (externals are doublely delayed
> on PA hpux).
>
> Ok for the 4.5, 4.6 and 4.7 branches?  The fix for PR middle-end/52640
> was not applied to the trunk.

I can't immediately see how your description of "the list of pending externals
and the vector" is deleted.  pa.c keeps its own vector which references the
decls and the only issue I see is that if you call assemble_external after
processing externals you ICE because the pointer-set is not initialized?

Why does the pa backend end up calling assemble_external at final time?

Richard.

> Dave
> --
> J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
> National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)
>
> 2012-04-09  John David Anglin  <dave.anglin@nrc-cnrc.gc.ca>
>
>        PR middle-end/52894
>        * varasm.c (process_pending_assemble_externals): Set
>        pending_assemble_externals_processed true.
>        (assemble_external): Call assemble_external_real if the pending
>        assemble externals have been processed.
>
> Index: varasm.c
> ===================================================================
> --- varasm.c    (revision 186213)
> +++ varasm.c    (working copy)
> @@ -2108,6 +2108,11 @@
>    the entire pending_assemble_externals list.  See assemble_external().  */
>  static struct pointer_set_t *pending_assemble_externals_set;
>
> +/* Some targets delay some output to final using TARGET_ASM_FILE_END.
> +   As a result, assemble_external can be called after the list of externals
> +   is processed and the pointer set destroyed.  */
> +static bool pending_assemble_externals_processed;
> +
>  #ifdef ASM_OUTPUT_EXTERNAL
>  /* True if DECL is a function decl for which no out-of-line copy exists.
>    It is assumed that DECL's assembler name has been set.  */
> @@ -2160,6 +2165,7 @@
>     assemble_external_real (TREE_VALUE (list));
>
>   pending_assemble_externals = 0;
> +  pending_assemble_externals_processed = true;
>   pointer_set_destroy (pending_assemble_externals_set);
>  #endif
>  }
> @@ -2201,6 +2207,12 @@
>     weak_decls = tree_cons (NULL, decl, weak_decls);
>
>  #ifdef ASM_OUTPUT_EXTERNAL
> +  if (pending_assemble_externals_processed)
> +    {
> +      assemble_external_real (decl);
> +      return;
> +    }
> +
>   if (! pointer_set_insert (pending_assemble_externals_set, decl))
>     pending_assemble_externals = tree_cons (NULL, decl,
>                                            pending_assemble_externals);
John David Anglin - April 10, 2012, 12:28 p.m.
On 10-Apr-12, at 7:06 AM, Richard Guenther wrote:

> I can't immediately see how your description of "the list of pending  
> externals
> and the vector" is deleted.  pa.c keeps its own vector which  
> references the
> decls and the only issue I see is that if you call assemble_external  
> after
> processing externals you ICE because the pointer-set is not  
> initialized?
>
> Why does the pa backend end up calling assemble_external at final  
> time?

The backend calls assemble_integer to output function descriptors at  
final
time.  This indirectly calls assemble_external from output_addr_const.
This can be seen in comment #4 in the PR.  This occurs after the pending
externals are processed.

We have to ensure that we only output function descriptors that are  
referenced,
and we can only determine this at final.

As a result, assemble_external is called after the VEC has been deleted.
The compiler doesn't ICE.  It goes into an infinite loop when a call to
pointer_set_insert tries to extend the deleted VEC.  It does this even  
if the
pointer is already in the VEC hash.

Previously, the storage wasn't deleted.  I suspect the ineffective  
calls on hpux
weren't noticed because assemble_external had already been called for  
the
function descriptors earlier, so the list in the backend was complete.

The problem was first seen with the Linux target.  There is no backend
list for this target because it doesn't use or need assemble_external.

I personally think it was wrong to add the deferral in  
assemble_external,
but that's another issue.

Dave.
Richard Guenther - April 10, 2012, 12:44 p.m.
On Tue, Apr 10, 2012 at 2:28 PM, Dave Anglin <dave.anglin@bell.net> wrote:
> On 10-Apr-12, at 7:06 AM, Richard Guenther wrote:
>
>> I can't immediately see how your description of "the list of pending
>> externals
>> and the vector" is deleted.  pa.c keeps its own vector which references
>> the
>> decls and the only issue I see is that if you call assemble_external after
>> processing externals you ICE because the pointer-set is not initialized?
>>
>> Why does the pa backend end up calling assemble_external at final time?
>
>
> The backend calls assemble_integer to output function descriptors at final
> time.  This indirectly calls assemble_external from output_addr_const.
> This can be seen in comment #4 in the PR.  This occurs after the pending
> externals are processed.
>
> We have to ensure that we only output function descriptors that are
> referenced,
> and we can only determine this at final.

You mean function destcriptors for functions that are output?  You already
know this at cgraph clone materialization time.

> As a result, assemble_external is called after the VEC has been deleted.
> The compiler doesn't ICE.  It goes into an infinite loop when a call to
> pointer_set_insert tries to extend the deleted VEC.  It does this even if
> the
> pointer is already in the VEC hash.

Isn't the bug then that the backend deletes the VEC too early?

> Previously, the storage wasn't deleted.  I suspect the ineffective calls on
> hpux
> weren't noticed because assemble_external had already been called for the
> function descriptors earlier, so the list in the backend was complete.
>
> The problem was first seen with the Linux target.  There is no backend
> list for this target because it doesn't use or need assemble_external.
>
> I personally think it was wrong to add the deferral in assemble_external,
> but that's another issue.

I think the best thing would be to revert the offending patch on old branches
(4.6 and 4.5) and see if the deferal can be fixed properly - though I didn't
look at the pa issue in detail.  Maybe Steven can do this.

Richard.

> Dave.
Steven Bosscher - April 10, 2012, 1:36 p.m.
On Tue, Apr 10, 2012 at 2:44 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Tue, Apr 10, 2012 at 2:28 PM, Dave Anglin <dave.anglin@bell.net> wrote:
>> On 10-Apr-12, at 7:06 AM, Richard Guenther wrote:
>>
>>> I can't immediately see how your description of "the list of pending
>>> externals
>>> and the vector" is deleted.  pa.c keeps its own vector which references
>>> the
>>> decls and the only issue I see is that if you call assemble_external after
>>> processing externals you ICE because the pointer-set is not initialized?
>>>
>>> Why does the pa backend end up calling assemble_external at final time?
>>
>>
>> The backend calls assemble_integer to output function descriptors at final
>> time.  This indirectly calls assemble_external from output_addr_const.
>> This can be seen in comment #4 in the PR.  This occurs after the pending
>> externals are processed.
>>
>> We have to ensure that we only output function descriptors that are
>> referenced,
>> and we can only determine this at final.
>
> You mean function destcriptors for functions that are output?  You already
> know this at cgraph clone materialization time.
>
>> As a result, assemble_external is called after the VEC has been deleted.
>> The compiler doesn't ICE.  It goes into an infinite loop when a call to
>> pointer_set_insert tries to extend the deleted VEC.  It does this even if
>> the
>> pointer is already in the VEC hash.
>
> Isn't the bug then that the backend deletes the VEC too early?
>
>> Previously, the storage wasn't deleted.  I suspect the ineffective calls on
>> hpux
>> weren't noticed because assemble_external had already been called for the
>> function descriptors earlier, so the list in the backend was complete.
>>
>> The problem was first seen with the Linux target.  There is no backend
>> list for this target because it doesn't use or need assemble_external.
>>
>> I personally think it was wrong to add the deferral in assemble_external,
>> but that's another issue.
>
> I think the best thing would be to revert the offending patch on old branches
> (4.6 and 4.5) and see if the deferral can be fixed properly - though I didn't
> look at the pa issue in detail.  Maybe Steven can do this.

It seems to me that the patch Dave posted is fine. It basically
reverts to the old situation (i.e. before the
pending_assemble_external stuff was added) for all back ends.

Ciao!
Steven
Richard Guenther - April 10, 2012, 2:01 p.m.
On Tue, Apr 10, 2012 at 3:36 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Tue, Apr 10, 2012 at 2:44 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Tue, Apr 10, 2012 at 2:28 PM, Dave Anglin <dave.anglin@bell.net> wrote:
>>> On 10-Apr-12, at 7:06 AM, Richard Guenther wrote:
>>>
>>>> I can't immediately see how your description of "the list of pending
>>>> externals
>>>> and the vector" is deleted.  pa.c keeps its own vector which references
>>>> the
>>>> decls and the only issue I see is that if you call assemble_external after
>>>> processing externals you ICE because the pointer-set is not initialized?
>>>>
>>>> Why does the pa backend end up calling assemble_external at final time?
>>>
>>>
>>> The backend calls assemble_integer to output function descriptors at final
>>> time.  This indirectly calls assemble_external from output_addr_const.
>>> This can be seen in comment #4 in the PR.  This occurs after the pending
>>> externals are processed.
>>>
>>> We have to ensure that we only output function descriptors that are
>>> referenced,
>>> and we can only determine this at final.
>>
>> You mean function destcriptors for functions that are output?  You already
>> know this at cgraph clone materialization time.
>>
>>> As a result, assemble_external is called after the VEC has been deleted.
>>> The compiler doesn't ICE.  It goes into an infinite loop when a call to
>>> pointer_set_insert tries to extend the deleted VEC.  It does this even if
>>> the
>>> pointer is already in the VEC hash.
>>
>> Isn't the bug then that the backend deletes the VEC too early?
>>
>>> Previously, the storage wasn't deleted.  I suspect the ineffective calls on
>>> hpux
>>> weren't noticed because assemble_external had already been called for the
>>> function descriptors earlier, so the list in the backend was complete.
>>>
>>> The problem was first seen with the Linux target.  There is no backend
>>> list for this target because it doesn't use or need assemble_external.
>>>
>>> I personally think it was wrong to add the deferral in assemble_external,
>>> but that's another issue.
>>
>> I think the best thing would be to revert the offending patch on old branches
>> (4.6 and 4.5) and see if the deferral can be fixed properly - though I didn't
>> look at the pa issue in detail.  Maybe Steven can do this.
>
> It seems to me that the patch Dave posted is fine. It basically
> reverts to the old situation (i.e. before the
> pending_assemble_external stuff was added) for all back ends.

Well, ok then.

Thanks,
Richard.

> Ciao!
> Steven

Patch

Index: varasm.c
===================================================================
--- varasm.c	(revision 186213)
+++ varasm.c	(working copy)
@@ -2108,6 +2108,11 @@ 
    the entire pending_assemble_externals list.  See assemble_external().  */
 static struct pointer_set_t *pending_assemble_externals_set;
 
+/* Some targets delay some output to final using TARGET_ASM_FILE_END.
+   As a result, assemble_external can be called after the list of externals
+   is processed and the pointer set destroyed.  */
+static bool pending_assemble_externals_processed;
+
 #ifdef ASM_OUTPUT_EXTERNAL
 /* True if DECL is a function decl for which no out-of-line copy exists.
    It is assumed that DECL's assembler name has been set.  */
@@ -2160,6 +2165,7 @@ 
     assemble_external_real (TREE_VALUE (list));
 
   pending_assemble_externals = 0;
+  pending_assemble_externals_processed = true;
   pointer_set_destroy (pending_assemble_externals_set);
 #endif
 }
@@ -2201,6 +2207,12 @@ 
     weak_decls = tree_cons (NULL, decl, weak_decls);
 
 #ifdef ASM_OUTPUT_EXTERNAL
+  if (pending_assemble_externals_processed)
+    {
+      assemble_external_real (decl);
+      return;
+    }
+
   if (! pointer_set_insert (pending_assemble_externals_set, decl))
     pending_assemble_externals = tree_cons (NULL, decl,
 					    pending_assemble_externals);