public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Fix libgo breakage (PR tree-optimization/67284)
@ 2015-08-21 11:00 Marek Polacek
  2015-08-21 12:07 ` Richard Biener
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Polacek @ 2015-08-21 11:00 UTC (permalink / raw)
  To: GCC Patches, Jeff Law, Richard Biener

This fixes the libgo breakage.  Seems I really should have removed the
edge after we split the block with null dereference after __builtin_trap 
statement so that it's unreachable.

Bootstrapped/regtested on x86_64-linux + ppc64-linux + bootstrapped on
aarch64-linux, ok for trunk?

2015-08-21  Marek Polacek  <polacek@redhat.com>

	PR tree-optimization/67284
	* gimple-ssa-isolate-paths.c (insert_trap): Remove the edge after
	we split a block.

diff --git gcc/gimple-ssa-isolate-paths.c gcc/gimple-ssa-isolate-paths.c
index ca2322d..5ac61d3 100644
--- gcc/gimple-ssa-isolate-paths.c
+++ gcc/gimple-ssa-isolate-paths.c
@@ -115,7 +115,8 @@ insert_trap (gimple_stmt_iterator *si_p, tree op)
   else
     gsi_insert_before (si_p, seq, GSI_NEW_STMT);
 
-  split_block (gimple_bb (new_stmt), new_stmt);
+  edge e = split_block (gimple_bb (new_stmt), new_stmt);
+  remove_edge (e);
   *si_p = gsi_for_stmt (stmt);
 }
 

	Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 11:00 [PATCH] Fix libgo breakage (PR tree-optimization/67284) Marek Polacek
@ 2015-08-21 12:07 ` Richard Biener
  2015-08-21 12:50   ` Marek Polacek
  0 siblings, 1 reply; 13+ messages in thread
From: Richard Biener @ 2015-08-21 12:07 UTC (permalink / raw)
  To: Marek Polacek; +Cc: GCC Patches, Jeff Law, Richard Biener

On Fri, Aug 21, 2015 at 12:52 PM, Marek Polacek <polacek@redhat.com> wrote:
> This fixes the libgo breakage.  Seems I really should have removed the
> edge after we split the block with null dereference after __builtin_trap
> statement so that it's unreachable.
>
> Bootstrapped/regtested on x86_64-linux + ppc64-linux + bootstrapped on
> aarch64-linux, ok for trunk?

Hum.  I don't see why this is needed - CFG cleanup (which of course needs
to run!) should do this for you.  In fact stray unreachable blocks are usually
more of a problem.

Richard.

> 2015-08-21  Marek Polacek  <polacek@redhat.com>
>
>         PR tree-optimization/67284
>         * gimple-ssa-isolate-paths.c (insert_trap): Remove the edge after
>         we split a block.
>
> diff --git gcc/gimple-ssa-isolate-paths.c gcc/gimple-ssa-isolate-paths.c
> index ca2322d..5ac61d3 100644
> --- gcc/gimple-ssa-isolate-paths.c
> +++ gcc/gimple-ssa-isolate-paths.c
> @@ -115,7 +115,8 @@ insert_trap (gimple_stmt_iterator *si_p, tree op)
>    else
>      gsi_insert_before (si_p, seq, GSI_NEW_STMT);
>
> -  split_block (gimple_bb (new_stmt), new_stmt);
> +  edge e = split_block (gimple_bb (new_stmt), new_stmt);
> +  remove_edge (e);
>    *si_p = gsi_for_stmt (stmt);
>  }
>
>
>         Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 12:07 ` Richard Biener
@ 2015-08-21 12:50   ` Marek Polacek
  2015-08-21 13:40     ` Richard Biener
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Polacek @ 2015-08-21 12:50 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches, Jeff Law, Richard Biener

On Fri, Aug 21, 2015 at 01:27:43PM +0200, Richard Biener wrote:
> On Fri, Aug 21, 2015 at 12:52 PM, Marek Polacek <polacek@redhat.com> wrote:
> > This fixes the libgo breakage.  Seems I really should have removed the
> > edge after we split the block with null dereference after __builtin_trap
> > statement so that it's unreachable.
> >
> > Bootstrapped/regtested on x86_64-linux + ppc64-linux + bootstrapped on
> > aarch64-linux, ok for trunk?
> 
> Hum.  I don't see why this is needed - CFG cleanup (which of course needs
> to run!) should do this for you.  In fact stray unreachable blocks are usually
> more of a problem.

Aha.  It seems cleanup does that if I change the code to generate
__builtin_unreachable instead of __builtin_trap.  A hint maybe? ;)

What's going on here is that we find a potential NULL dereference
(PHI argument has NULL), we call isolate_path, that duplicates a bb
and then cuts off the outgoing edges of this duplicated bb.  And in
some cases it leaves COND_EXPR at the end of the bb -> ICE.
The we insert __builtin_trap on this isolated path.

	Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 12:50   ` Marek Polacek
@ 2015-08-21 13:40     ` Richard Biener
  2015-08-21 14:43       ` Marek Polacek
  0 siblings, 1 reply; 13+ messages in thread
From: Richard Biener @ 2015-08-21 13:40 UTC (permalink / raw)
  To: Marek Polacek; +Cc: GCC Patches, Jeff Law, Richard Biener

On Fri, Aug 21, 2015 at 2:49 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Fri, Aug 21, 2015 at 01:27:43PM +0200, Richard Biener wrote:
>> On Fri, Aug 21, 2015 at 12:52 PM, Marek Polacek <polacek@redhat.com> wrote:
>> > This fixes the libgo breakage.  Seems I really should have removed the
>> > edge after we split the block with null dereference after __builtin_trap
>> > statement so that it's unreachable.
>> >
>> > Bootstrapped/regtested on x86_64-linux + ppc64-linux + bootstrapped on
>> > aarch64-linux, ok for trunk?
>>
>> Hum.  I don't see why this is needed - CFG cleanup (which of course needs
>> to run!) should do this for you.  In fact stray unreachable blocks are usually
>> more of a problem.
>
> Aha.  It seems cleanup does that if I change the code to generate
> __builtin_unreachable instead of __builtin_trap.  A hint maybe? ;)

Not really...

static bool
cleanup_control_flow_bb (basic_block bb)
{
...
  /* Check for indirect calls that have been turned into
     noreturn calls.  */
  else if (is_gimple_call (stmt)
           && gimple_call_noreturn_p (stmt)
           && remove_fallthru_edge (bb->succs))
    retval = true;

and __builtin_trap is NORETURN.  But there is the hint where to debug.

Richard.

> What's going on here is that we find a potential NULL dereference
> (PHI argument has NULL), we call isolate_path, that duplicates a bb
> and then cuts off the outgoing edges of this duplicated bb.  And in
> some cases it leaves COND_EXPR at the end of the bb -> ICE.
> The we insert __builtin_trap on this isolated path.
>
>         Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 13:40     ` Richard Biener
@ 2015-08-21 14:43       ` Marek Polacek
  2015-08-21 15:05         ` Jeff Law
  2015-08-21 15:37         ` Jeff Law
  0 siblings, 2 replies; 13+ messages in thread
From: Marek Polacek @ 2015-08-21 14:43 UTC (permalink / raw)
  To: Richard Biener; +Cc: GCC Patches, Jeff Law, Richard Biener

On Fri, Aug 21, 2015 at 03:37:38PM +0200, Richard Biener wrote:
> On Fri, Aug 21, 2015 at 2:49 PM, Marek Polacek <polacek@redhat.com> wrote:
> > On Fri, Aug 21, 2015 at 01:27:43PM +0200, Richard Biener wrote:
> >> On Fri, Aug 21, 2015 at 12:52 PM, Marek Polacek <polacek@redhat.com> wrote:
> >> > This fixes the libgo breakage.  Seems I really should have removed the
> >> > edge after we split the block with null dereference after __builtin_trap
> >> > statement so that it's unreachable.
> >> >
> >> > Bootstrapped/regtested on x86_64-linux + ppc64-linux + bootstrapped on
> >> > aarch64-linux, ok for trunk?
> >>
> >> Hum.  I don't see why this is needed - CFG cleanup (which of course needs
> >> to run!) should do this for you.  In fact stray unreachable blocks are usually
> >> more of a problem.
> >
> > Aha.  It seems cleanup does that if I change the code to generate
> > __builtin_unreachable instead of __builtin_trap.  A hint maybe? ;)
> 
> Not really...
> 
> static bool
> cleanup_control_flow_bb (basic_block bb)
> {
> ...
>   /* Check for indirect calls that have been turned into
>      noreturn calls.  */
>   else if (is_gimple_call (stmt)
>            && gimple_call_noreturn_p (stmt)
>            && remove_fallthru_edge (bb->succs))
>     retval = true;
> 
> and __builtin_trap is NORETURN.  But there is the hint where to debug.

Yes, but gimple_call_noreturn_p is false on __builtin_trap.  That's quite
confusing... but flags_from_decl_or_type really returns 0 for __builtin_trap.

So can't we use __builtin_unreachable in isolate-path code?

	Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 14:43       ` Marek Polacek
@ 2015-08-21 15:05         ` Jeff Law
  2015-08-21 15:37         ` Jeff Law
  1 sibling, 0 replies; 13+ messages in thread
From: Jeff Law @ 2015-08-21 15:05 UTC (permalink / raw)
  To: Marek Polacek, Richard Biener; +Cc: GCC Patches, Richard Biener

On 08/21/2015 08:41 AM, Marek Polacek wrote:
> On Fri, Aug 21, 2015 at 03:37:38PM +0200, Richard Biener wrote:
>> On Fri, Aug 21, 2015 at 2:49 PM, Marek Polacek <polacek@redhat.com> wrote:
>>> On Fri, Aug 21, 2015 at 01:27:43PM +0200, Richard Biener wrote:
>>>> On Fri, Aug 21, 2015 at 12:52 PM, Marek Polacek <polacek@redhat.com> wrote:
>>>>> This fixes the libgo breakage.  Seems I really should have removed the
>>>>> edge after we split the block with null dereference after __builtin_trap
>>>>> statement so that it's unreachable.
>>>>>
>>>>> Bootstrapped/regtested on x86_64-linux + ppc64-linux + bootstrapped on
>>>>> aarch64-linux, ok for trunk?
>>>>
>>>> Hum.  I don't see why this is needed - CFG cleanup (which of course needs
>>>> to run!) should do this for you.  In fact stray unreachable blocks are usually
>>>> more of a problem.
>>>
>>> Aha.  It seems cleanup does that if I change the code to generate
>>> __builtin_unreachable instead of __builtin_trap.  A hint maybe? ;)
>>
>> Not really...
>>
>> static bool
>> cleanup_control_flow_bb (basic_block bb)
>> {
>> ...
>>    /* Check for indirect calls that have been turned into
>>       noreturn calls.  */
>>    else if (is_gimple_call (stmt)
>>             && gimple_call_noreturn_p (stmt)
>>             && remove_fallthru_edge (bb->succs))
>>      retval = true;
>>
>> and __builtin_trap is NORETURN.  But there is the hint where to debug.
>
> Yes, but gimple_call_noreturn_p is false on __builtin_trap.  That's quite
> confusing... but flags_from_decl_or_type really returns 0 for __builtin_trap.
>
> So can't we use __builtin_unreachable in isolate-path code?
No, we really want the trap to halt execution rather than executing 
whatever code is following in the instruction stream.  The latter is a 
significant security problem.



jeff

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 14:43       ` Marek Polacek
  2015-08-21 15:05         ` Jeff Law
@ 2015-08-21 15:37         ` Jeff Law
  2015-08-21 16:23           ` Richard Biener
  2015-08-21 16:30           ` Richard Biener
  1 sibling, 2 replies; 13+ messages in thread
From: Jeff Law @ 2015-08-21 15:37 UTC (permalink / raw)
  To: Marek Polacek, Richard Biener; +Cc: GCC Patches, Richard Biener

On 08/21/2015 08:41 AM, Marek Polacek wrote:
> On Fri, Aug 21, 2015 at 03:37:38PM +0200, Richard Biener wrote:
>> On Fri, Aug 21, 2015 at 2:49 PM, Marek Polacek <polacek@redhat.com> wrote:
>>> On Fri, Aug 21, 2015 at 01:27:43PM +0200, Richard Biener wrote:
>>>> On Fri, Aug 21, 2015 at 12:52 PM, Marek Polacek <polacek@redhat.com> wrote:
>>>>> This fixes the libgo breakage.  Seems I really should have removed the
>>>>> edge after we split the block with null dereference after __builtin_trap
>>>>> statement so that it's unreachable.
>>>>>
>>>>> Bootstrapped/regtested on x86_64-linux + ppc64-linux + bootstrapped on
>>>>> aarch64-linux, ok for trunk?
>>>>
>>>> Hum.  I don't see why this is needed - CFG cleanup (which of course needs
>>>> to run!) should do this for you.  In fact stray unreachable blocks are usually
>>>> more of a problem.
>>>
>>> Aha.  It seems cleanup does that if I change the code to generate
>>> __builtin_unreachable instead of __builtin_trap.  A hint maybe? ;)
>>
>> Not really...
>>
>> static bool
>> cleanup_control_flow_bb (basic_block bb)
>> {
>> ...
>>    /* Check for indirect calls that have been turned into
>>       noreturn calls.  */
>>    else if (is_gimple_call (stmt)
>>             && gimple_call_noreturn_p (stmt)
>>             && remove_fallthru_edge (bb->succs))
>>      retval = true;
>>
>> and __builtin_trap is NORETURN.  But there is the hint where to debug.
>
> Yes, but gimple_call_noreturn_p is false on __builtin_trap.  That's quite
> confusing... but flags_from_decl_or_type really returns 0 for __builtin_trap.
Well, if that's intentional (and offhand I have no idea if it is), then 
you could emit a __builtin_trap followed by a __builtin_unreachable.

jeff

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 15:37         ` Jeff Law
@ 2015-08-21 16:23           ` Richard Biener
  2015-08-21 16:30           ` Richard Biener
  1 sibling, 0 replies; 13+ messages in thread
From: Richard Biener @ 2015-08-21 16:23 UTC (permalink / raw)
  To: Jeff Law, Marek Polacek, Richard Biener; +Cc: GCC Patches

On August 21, 2015 5:09:46 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>On 08/21/2015 08:41 AM, Marek Polacek wrote:
>> On Fri, Aug 21, 2015 at 03:37:38PM +0200, Richard Biener wrote:
>>> On Fri, Aug 21, 2015 at 2:49 PM, Marek Polacek <polacek@redhat.com>
>wrote:
>>>> On Fri, Aug 21, 2015 at 01:27:43PM +0200, Richard Biener wrote:
>>>>> On Fri, Aug 21, 2015 at 12:52 PM, Marek Polacek
><polacek@redhat.com> wrote:
>>>>>> This fixes the libgo breakage.  Seems I really should have
>removed the
>>>>>> edge after we split the block with null dereference after
>__builtin_trap
>>>>>> statement so that it's unreachable.
>>>>>>
>>>>>> Bootstrapped/regtested on x86_64-linux + ppc64-linux +
>bootstrapped on
>>>>>> aarch64-linux, ok for trunk?
>>>>>
>>>>> Hum.  I don't see why this is needed - CFG cleanup (which of
>course needs
>>>>> to run!) should do this for you.  In fact stray unreachable blocks
>are usually
>>>>> more of a problem.
>>>>
>>>> Aha.  It seems cleanup does that if I change the code to generate
>>>> __builtin_unreachable instead of __builtin_trap.  A hint maybe? ;)
>>>
>>> Not really...
>>>
>>> static bool
>>> cleanup_control_flow_bb (basic_block bb)
>>> {
>>> ...
>>>    /* Check for indirect calls that have been turned into
>>>       noreturn calls.  */
>>>    else if (is_gimple_call (stmt)
>>>             && gimple_call_noreturn_p (stmt)
>>>             && remove_fallthru_edge (bb->succs))
>>>      retval = true;
>>>
>>> and __builtin_trap is NORETURN.  But there is the hint where to
>debug.
>>
>> Yes, but gimple_call_noreturn_p is false on __builtin_trap.  That's
>quite
>> confusing... but flags_from_decl_or_type really returns 0 for
>__builtin_trap.
>Well, if that's intentional (and offhand I have no idea if it is), then
>
>you could emit a __builtin_trap followed by a __builtin_unreachable.

Looking at builtins.def I'd say it should be no return.

Richard.

>jeff


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 15:37         ` Jeff Law
  2015-08-21 16:23           ` Richard Biener
@ 2015-08-21 16:30           ` Richard Biener
  2015-08-21 16:52             ` Marek Polacek
  1 sibling, 1 reply; 13+ messages in thread
From: Richard Biener @ 2015-08-21 16:30 UTC (permalink / raw)
  To: Jeff Law, Marek Polacek, Richard Biener; +Cc: GCC Patches

On August 21, 2015 5:09:46 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>On 08/21/2015 08:41 AM, Marek Polacek wrote:
>> On Fri, Aug 21, 2015 at 03:37:38PM +0200, Richard Biener wrote:
>>> On Fri, Aug 21, 2015 at 2:49 PM, Marek Polacek <polacek@redhat.com>
>wrote:
>>>> On Fri, Aug 21, 2015 at 01:27:43PM +0200, Richard Biener wrote:
>>>>> On Fri, Aug 21, 2015 at 12:52 PM, Marek Polacek
><polacek@redhat.com> wrote:
>>>>>> This fixes the libgo breakage.  Seems I really should have
>removed the
>>>>>> edge after we split the block with null dereference after
>__builtin_trap
>>>>>> statement so that it's unreachable.
>>>>>>
>>>>>> Bootstrapped/regtested on x86_64-linux + ppc64-linux +
>bootstrapped on
>>>>>> aarch64-linux, ok for trunk?
>>>>>
>>>>> Hum.  I don't see why this is needed - CFG cleanup (which of
>course needs
>>>>> to run!) should do this for you.  In fact stray unreachable blocks
>are usually
>>>>> more of a problem.
>>>>
>>>> Aha.  It seems cleanup does that if I change the code to generate
>>>> __builtin_unreachable instead of __builtin_trap.  A hint maybe? ;)
>>>
>>> Not really...
>>>
>>> static bool
>>> cleanup_control_flow_bb (basic_block bb)
>>> {
>>> ...
>>>    /* Check for indirect calls that have been turned into
>>>       noreturn calls.  */
>>>    else if (is_gimple_call (stmt)
>>>             && gimple_call_noreturn_p (stmt)
>>>             && remove_fallthru_edge (bb->succs))
>>>      retval = true;
>>>
>>> and __builtin_trap is NORETURN.  But there is the hint where to
>debug.
>>
>> Yes, but gimple_call_noreturn_p is false on __builtin_trap.  That's
>quite
>> confusing... but flags_from_decl_or_type really returns 0 for
>__builtin_trap.
>Well, if that's intentional (and offhand I have no idea if it is), then
>
>you could emit a __builtin_trap followed by a __builtin_unreachable.

But maybe go is non-call-exceptions  and that makes a difference?

>jeff


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 16:30           ` Richard Biener
@ 2015-08-21 16:52             ` Marek Polacek
  2015-08-21 17:31               ` Richard Biener
  0 siblings, 1 reply; 13+ messages in thread
From: Marek Polacek @ 2015-08-21 16:52 UTC (permalink / raw)
  To: Richard Biener; +Cc: Jeff Law, Richard Biener, GCC Patches

On Fri, Aug 21, 2015 at 06:23:09PM +0200, Richard Biener wrote:
> >> Yes, but gimple_call_noreturn_p is false on __builtin_trap.  That's
> >quite
> >> confusing... but flags_from_decl_or_type really returns 0 for
> >__builtin_trap.
> >Well, if that's intentional (and offhand I have no idea if it is), then
> >
> >you could emit a __builtin_trap followed by a __builtin_unreachable.
> 
> But maybe go is non-call-exceptions  and that makes a difference?

I suppose that's the case.  In Makefile.am I see
AM_CFLAGS = -fexceptions -fnon-call-exceptions -fplan9-extensions
But I'm not clear on how this could make a difference wrt whether
__builtin_trap is volatile (thus ECF_NORETURN).

This is a patch to also generate __builtin_unreachable, just for the record.

Bootstrapped/regtested on x86_64-linux.

2015-08-21  Marek Polacek  <polacek@redhat.com>

	PR tree-optimization/67284
	* gimple-ssa-isolate-paths.c (insert_trap): Also emit
	__builtin_unreachable.

diff --git gcc/gimple-ssa-isolate-paths.c gcc/gimple-ssa-isolate-paths.c
index ca2322d..06263d3 100644
--- gcc/gimple-ssa-isolate-paths.c
+++ gcc/gimple-ssa-isolate-paths.c
@@ -97,15 +97,18 @@ insert_trap (gimple_stmt_iterator *si_p, tree op)
     = gimple_build_call (builtin_decl_explicit (BUILT_IN_TRAP), 0);
   gimple_seq seq = NULL;
   gimple_seq_add_stmt (&seq, new_stmt);
+  new_stmt
+     = gimple_build_call (builtin_decl_explicit (BUILT_IN_UNREACHABLE), 0);
+  gimple_seq_add_stmt (&seq, new_stmt);
 
   /* If we had a NULL pointer dereference, then we want to insert the
-     __builtin_trap after the statement, for the other cases we want
-     to insert before the statement.  */
+     __builtin_trap/__builtin_unreachable pair after the statement, for
+     the other cases we want to insert before the statement.  */
   if (walk_stmt_load_store_ops (stmt, (void *)op,
 			        check_loadstore,
 				check_loadstore))
     {
-      gsi_insert_after (si_p, seq, GSI_NEW_STMT);
+      gsi_insert_seq_after (si_p, seq, GSI_NEW_STMT);
       if (stmt_ends_bb_p (stmt))
 	{
 	  split_block (gimple_bb (stmt), stmt);
@@ -113,7 +116,7 @@ insert_trap (gimple_stmt_iterator *si_p, tree op)
 	}
     }
   else
-    gsi_insert_before (si_p, seq, GSI_NEW_STMT);
+    gsi_insert_seq_before (si_p, seq, GSI_NEW_STMT);
 
   split_block (gimple_bb (new_stmt), new_stmt);
   *si_p = gsi_for_stmt (stmt);
@@ -194,7 +197,7 @@ isolate_path (basic_block bb, basic_block duplicate,
 
   /* If we did not run to the end of DUPLICATE, then SI points to STMT and
      SI2 points to the duplicate of STMT in DUPLICATE.  Insert a trap
-     before SI2 and remove SI2 and all trailing statements.  */
+     before SI2 and split the block.  */
   if (!gsi_end_p (si2))
     {
       if (ret_zero)

	Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 16:52             ` Marek Polacek
@ 2015-08-21 17:31               ` Richard Biener
  2015-08-24  9:17                 ` Jakub Jelinek
  0 siblings, 1 reply; 13+ messages in thread
From: Richard Biener @ 2015-08-21 17:31 UTC (permalink / raw)
  To: Marek Polacek; +Cc: Jeff Law, Richard Biener, GCC Patches

On August 21, 2015 6:42:15 PM GMT+02:00, Marek Polacek <polacek@redhat.com> wrote:
>On Fri, Aug 21, 2015 at 06:23:09PM +0200, Richard Biener wrote:
>> >> Yes, but gimple_call_noreturn_p is false on __builtin_trap. 
>That's
>> >quite
>> >> confusing... but flags_from_decl_or_type really returns 0 for
>> >__builtin_trap.
>> >Well, if that's intentional (and offhand I have no idea if it is),
>then
>> >
>> >you could emit a __builtin_trap followed by a __builtin_unreachable.
>> 
>> But maybe go is non-call-exceptions  and that makes a difference?
>
>I suppose that's the case.  In Makefile.am I see
>AM_CFLAGS = -fexceptions -fnon-call-exceptions -fplan9-extensions
>But I'm not clear on how this could make a difference wrt whether
>__builtin_trap is volatile (thus ECF_NORETURN).

Not sure either. Needs to be investigated. BTW, built-in trap is also nothrow AFAIR.

Richard.

>This is a patch to also generate __builtin_unreachable, just for the
>record.
>
>Bootstrapped/regtested on x86_64-linux.
>
>2015-08-21  Marek Polacek  <polacek@redhat.com>
>
>	PR tree-optimization/67284
>	* gimple-ssa-isolate-paths.c (insert_trap): Also emit
>	__builtin_unreachable.
>
>diff --git gcc/gimple-ssa-isolate-paths.c
>gcc/gimple-ssa-isolate-paths.c
>index ca2322d..06263d3 100644
>--- gcc/gimple-ssa-isolate-paths.c
>+++ gcc/gimple-ssa-isolate-paths.c
>@@ -97,15 +97,18 @@ insert_trap (gimple_stmt_iterator *si_p, tree op)
>     = gimple_build_call (builtin_decl_explicit (BUILT_IN_TRAP), 0);
>   gimple_seq seq = NULL;
>   gimple_seq_add_stmt (&seq, new_stmt);
>+  new_stmt
>+     = gimple_build_call (builtin_decl_explicit
>(BUILT_IN_UNREACHABLE), 0);
>+  gimple_seq_add_stmt (&seq, new_stmt);
> 
>   /* If we had a NULL pointer dereference, then we want to insert the
>-     __builtin_trap after the statement, for the other cases we want
>-     to insert before the statement.  */
>+     __builtin_trap/__builtin_unreachable pair after the statement,
>for
>+     the other cases we want to insert before the statement.  */
>   if (walk_stmt_load_store_ops (stmt, (void *)op,
> 			        check_loadstore,
> 				check_loadstore))
>     {
>-      gsi_insert_after (si_p, seq, GSI_NEW_STMT);
>+      gsi_insert_seq_after (si_p, seq, GSI_NEW_STMT);
>       if (stmt_ends_bb_p (stmt))
> 	{
> 	  split_block (gimple_bb (stmt), stmt);
>@@ -113,7 +116,7 @@ insert_trap (gimple_stmt_iterator *si_p, tree op)
> 	}
>     }
>   else
>-    gsi_insert_before (si_p, seq, GSI_NEW_STMT);
>+    gsi_insert_seq_before (si_p, seq, GSI_NEW_STMT);
> 
>   split_block (gimple_bb (new_stmt), new_stmt);
>   *si_p = gsi_for_stmt (stmt);
>@@ -194,7 +197,7 @@ isolate_path (basic_block bb, basic_block
>duplicate,
> 
>/* If we did not run to the end of DUPLICATE, then SI points to STMT
>and
>      SI2 points to the duplicate of STMT in DUPLICATE.  Insert a trap
>-     before SI2 and remove SI2 and all trailing statements.  */
>+     before SI2 and split the block.  */
>   if (!gsi_end_p (si2))
>     {
>       if (ret_zero)
>
>	Marek


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-21 17:31               ` Richard Biener
@ 2015-08-24  9:17                 ` Jakub Jelinek
  2015-08-24  9:23                   ` Marek Polacek
  0 siblings, 1 reply; 13+ messages in thread
From: Jakub Jelinek @ 2015-08-24  9:17 UTC (permalink / raw)
  To: Richard Biener; +Cc: Marek Polacek, Jeff Law, Richard Biener, GCC Patches

On Fri, Aug 21, 2015 at 07:22:24PM +0200, Richard Biener wrote:
> On August 21, 2015 6:42:15 PM GMT+02:00, Marek Polacek <polacek@redhat.com> wrote:
> >On Fri, Aug 21, 2015 at 06:23:09PM +0200, Richard Biener wrote:
> >> >> Yes, but gimple_call_noreturn_p is false on __builtin_trap. 
> >That's
> >> >quite
> >> >> confusing... but flags_from_decl_or_type really returns 0 for
> >> >__builtin_trap.
> >> >Well, if that's intentional (and offhand I have no idea if it is),
> >then
> >> >
> >> >you could emit a __builtin_trap followed by a __builtin_unreachable.
> >> 
> >> But maybe go is non-call-exceptions  and that makes a difference?
> >
> >I suppose that's the case.  In Makefile.am I see
> >AM_CFLAGS = -fexceptions -fnon-call-exceptions -fplan9-extensions
> >But I'm not clear on how this could make a difference wrt whether
> >__builtin_trap is volatile (thus ECF_NORETURN).
> 
> Not sure either. Needs to be investigated. BTW, built-in trap is also nothrow AFAIR.

I think the bug is in the Go FE which incorrectly doesn't set
TREE_THIS_VOLATILE on the __builtin_trap decl.

	Jakub

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Fix libgo breakage (PR tree-optimization/67284)
  2015-08-24  9:17                 ` Jakub Jelinek
@ 2015-08-24  9:23                   ` Marek Polacek
  0 siblings, 0 replies; 13+ messages in thread
From: Marek Polacek @ 2015-08-24  9:23 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: Richard Biener, Jeff Law, Richard Biener, GCC Patches

On Mon, Aug 24, 2015 at 11:07:41AM +0200, Jakub Jelinek wrote:
> On Fri, Aug 21, 2015 at 07:22:24PM +0200, Richard Biener wrote:
> > Not sure either. Needs to be investigated. BTW, built-in trap is also nothrow AFAIR.
> 
> I think the bug is in the Go FE which incorrectly doesn't set
> TREE_THIS_VOLATILE on the __builtin_trap decl.

I have a patch that fixes this; and on a simple testcase I don't see the ICE
anymore.  Will post the patch after further testing.

Thanks for the pointer Jakub.

	Marek

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-08-24  9:19 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-21 11:00 [PATCH] Fix libgo breakage (PR tree-optimization/67284) Marek Polacek
2015-08-21 12:07 ` Richard Biener
2015-08-21 12:50   ` Marek Polacek
2015-08-21 13:40     ` Richard Biener
2015-08-21 14:43       ` Marek Polacek
2015-08-21 15:05         ` Jeff Law
2015-08-21 15:37         ` Jeff Law
2015-08-21 16:23           ` Richard Biener
2015-08-21 16:30           ` Richard Biener
2015-08-21 16:52             ` Marek Polacek
2015-08-21 17:31               ` Richard Biener
2015-08-24  9:17                 ` Jakub Jelinek
2015-08-24  9:23                   ` Marek Polacek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).