* [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).