diff mbox series

Unreachable bb discovery for returns_twice/__builtin_setjmp_receiver/.ABNORMAL_DISPATCH (PR tree-optimization/89280)

Message ID 20190223090540.GP7611@tucnak
State New
Headers show
Series Unreachable bb discovery for returns_twice/__builtin_setjmp_receiver/.ABNORMAL_DISPATCH (PR tree-optimization/89280) | expand

Commit Message

Jakub Jelinek Feb. 23, 2019, 9:05 a.m. UTC
Hi!

AFAIK we use EDGE_ABNORMAL edges (without EDGE_EH) for the following
purposes:
1) edges from various spots in the IL to an .ABNORMAL_DISPATCHER basic block
2) edges from that .ABNORMAL_DISPATCHER to bbs with DECL_NONLOCAL label
   for non-local goto
3) edges from that .ABNORMAL_DISPATCHER to bbs with FORCED_LABEL label
   for computed jumps
4) edges from that .ABNORMAL_DISPATCHER to bbs starting with
   __builtin_setjmp_receiver (these actually also have a FORCED_LABEL,
   and the FORCED_LABEL's address is taken in the __builtin_setjmp_receiver
   call argument (and corresponding __builtin_setjmp_setup's second
   argument)
5) edges from that .ABNORMAL_DISPATCHER to bbs starting with a returns_twice
   call (like normal setjmp, vfork, ...)

4) and 5) above together represent returns_twice calls, which in reality
can't be reentered "second" time without actually be called "first" time
normally.  As the testcase show, we can get ICEs if we remove the normal
basic blocks through which they are entered (which could define pre-SSA
form SSA_NAMEs that are used after them).  If going successfully into SSA
form, we should be safe afterwards from this kind of issues, we just have
PHIs with (ab) SSA_NAMEs in it, but still if we can prove the returns_twice
call is not entered normally, we should be able to remove also the second
enter path and anything dominated by that as unreachable; right now that
can be eliminated only during RTL opts where we don't have those abnormal
edges.

The following patch implements this in cleanup_control_flow_pre which is
where GIMPLE removal of unreachable basic block happens.

For 5), we simply ignore such edges for visited propagation, we have
    |  +---AB2----.ABNORMAL_DISPATCHER
    | /                 ^
    v v                 |
  setjmp               AB1
    | \                 |
    v  +----------------+
normally if it is reachable, so by ignoring the AB2 edge, if there is
any normal edge to setjmp, it will be visited (and so will be
.ABNORMAL_DISPATCHER and anything that is reachable after setjmp).
If the normal incoming edge into setjmp is gone, setjmp will not be visited,
even when .ABNORMAL_DISPATCHER is reachable through some other AB edges
(we create those pessimistically from lots of spots).

For 4), we have
    |
    v
  __builtin_setjmp_setup (..., &<L20>)
    |             \
    v              +---------AB3------------>.ABNORMAL_DISPATCHER
                                                /
                   +---------AB4---------------+
                  /
                 v
           <L20>:
           __builtin_setjmp_receiver (&<L20>)
                 |
                 v
The patch ignores the AB4 edge above in normal processing
(.ABNORMAL_DISPATCHER could have again many incoming edges from lots of
spots, it could be visited before that __builtin_setjmp_setup or after it)
but has a special code when we make __builtin_setjmp_setup basic block
visited, we queue AB4 edge for special processing where it will not be
ignored.

And finally, for 1) the patch has a case for a dead .ABNORMAL_DISPATCHER,
one that has only incoming edges but no successors.  I didn't want to
complicate or slow down the processing too much, so it is actually done
only if .ABNORMAL_DISPATCHER has no outgoing edges, rather than if it has
some, but all of them are in the end to non-visited basic blocks.
This means when we for 5) or 4) above remove all such bbs as dead in one
cfg cleanup, we remove .ABNORMAL_DISPATCHER only in the following cfg
cleanup.

Edges 2) and 3) are handled normally as before.

The reason the patch tries to be conservative in the checks rather than
having gcc_asserts that things are the way they should be is that it seems
we unfortunately expose those __builtin_setjmp_setup and
__builtin_setjmp_receiver builtins to users and they can do stuff like:
void *buf[5];

void
foo (int a)
{
  __label__ lab;
  __builtin_setjmp_setup (buf, &&lab);
  lab:
  __builtin_setjmp_receiver (&&lab);
//  if (a)
//    __builtin_longjmp (buf, 1);
}
and it actually compiles (ICEs with the longjmp in there, without/with the
following patch; I just didn't want to introduce further ICEs on these
bogus testcases).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-02-23  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/89280
	* tree-cfgcleanup.c (maybe_dead_abnormal_edge_p,
	builtin_setjmp_setup_bb): New functions.
	(cleanup_control_flow_pre): Ignore maybe_dead_abnormal_edge_p edges.
	When visiting __builtin_setjmp_setup block, queue in special
	setjmp_vec vector edges from .ABNORMAL_DISPATCHER to corresponding
	__builtin_setjmp_receiver.

	* gcc.c-torture/compile/pr89280.c: New test.
	* gcc.dg/torture/pr57147-2.c: Don't expect a setjmp after noreturn
	function.  Skip the test for -O0.


	Jakub

Comments

Jakub Jelinek Feb. 26, 2019, 2:45 p.m. UTC | #1
On Sat, Feb 23, 2019 at 10:05:40AM +0100, Jakub Jelinek wrote:
> And finally, for 1) the patch has a case for a dead .ABNORMAL_DISPATCHER,
> one that has only incoming edges but no successors.  I didn't want to
> complicate or slow down the processing too much, so it is actually done
> only if .ABNORMAL_DISPATCHER has no outgoing edges, rather than if it has
> some, but all of them are in the end to non-visited basic blocks.
> This means when we for 5) or 4) above remove all such bbs as dead in one
> cfg cleanup, we remove .ABNORMAL_DISPATCHER only in the following cfg
> cleanup.

Thinking about the above, I think we could handle that cheaply in the same
cleanup cfg rather than in the next one.  Don't know right now if we can
have at most one .ABNORMAL_DISPATCHER in a function or more (I vaguely
remember we disallow inlining in some of the ab cases like setjmp,
non-local goto etc.), if there can be at most one, we could just make
the .ABNORMAL_DISPATCHER visited normally, but additionally note it in some
basic_block pointer, if we can have more than one, we could have auto_vec of
them, and then after the loop just go through that one or more
.ABNORMAL_DISPATCHER bbs and if they don't have any successors visited,
unset the visited bit for them as well.

Shall I modify the patch for that?

> 2019-02-23  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/89280
> 	* tree-cfgcleanup.c (maybe_dead_abnormal_edge_p,
> 	builtin_setjmp_setup_bb): New functions.
> 	(cleanup_control_flow_pre): Ignore maybe_dead_abnormal_edge_p edges.
> 	When visiting __builtin_setjmp_setup block, queue in special
> 	setjmp_vec vector edges from .ABNORMAL_DISPATCHER to corresponding
> 	__builtin_setjmp_receiver.
> 
> 	* gcc.c-torture/compile/pr89280.c: New test.
> 	* gcc.dg/torture/pr57147-2.c: Don't expect a setjmp after noreturn
> 	function.  Skip the test for -O0.

	Jakub
Richard Biener Feb. 26, 2019, 3:08 p.m. UTC | #2
On Tue, 26 Feb 2019, Jakub Jelinek wrote:

> On Sat, Feb 23, 2019 at 10:05:40AM +0100, Jakub Jelinek wrote:
> > And finally, for 1) the patch has a case for a dead .ABNORMAL_DISPATCHER,
> > one that has only incoming edges but no successors.  I didn't want to
> > complicate or slow down the processing too much, so it is actually done
> > only if .ABNORMAL_DISPATCHER has no outgoing edges, rather than if it has
> > some, but all of them are in the end to non-visited basic blocks.
> > This means when we for 5) or 4) above remove all such bbs as dead in one
> > cfg cleanup, we remove .ABNORMAL_DISPATCHER only in the following cfg
> > cleanup.
> 
> Thinking about the above, I think we could handle that cheaply in the same
> cleanup cfg rather than in the next one.  Don't know right now if we can
> have at most one .ABNORMAL_DISPATCHER in a function or more (I vaguely
> remember we disallow inlining in some of the ab cases like setjmp,
> non-local goto etc.), if there can be at most one, we could just make
> the .ABNORMAL_DISPATCHER visited normally, but additionally note it in some
> basic_block pointer, if we can have more than one, we could have auto_vec of
> them, and then after the loop just go through that one or more
> .ABNORMAL_DISPATCHER bbs and if they don't have any successors visited,
> unset the visited bit for them as well.

I think we can have multiple .ABNORMAL_DISPATCHERs - at least in theory
via inlining, though I see code in the inliner avoiding multiple ones
at least for nonlocal goto, not sure if that covers all cases.

> Shall I modify the patch for that?

Might it even simplify the patch?  If not the only comment on the
original patch is that it would be nice to test it on a SJLJ EH
target ...

Richard.

> > 2019-02-23  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR tree-optimization/89280
> > 	* tree-cfgcleanup.c (maybe_dead_abnormal_edge_p,
> > 	builtin_setjmp_setup_bb): New functions.
> > 	(cleanup_control_flow_pre): Ignore maybe_dead_abnormal_edge_p edges.
> > 	When visiting __builtin_setjmp_setup block, queue in special
> > 	setjmp_vec vector edges from .ABNORMAL_DISPATCHER to corresponding
> > 	__builtin_setjmp_receiver.
> > 
> > 	* gcc.c-torture/compile/pr89280.c: New test.
> > 	* gcc.dg/torture/pr57147-2.c: Don't expect a setjmp after noreturn
> > 	function.  Skip the test for -O0.
> 
> 	Jakub
> 
>
Jakub Jelinek Feb. 26, 2019, 4:18 p.m. UTC | #3
On Tue, Feb 26, 2019 at 04:08:21PM +0100, Richard Biener wrote:
> I think we can have multiple .ABNORMAL_DISPATCHERs - at least in theory
> via inlining, though I see code in the inliner avoiding multiple ones
> at least for nonlocal goto, not sure if that covers all cases.

Ok.

> > Shall I modify the patch for that?
> 
> Might it even simplify the patch?  If not the only comment on the
> original patch is that it would be nice to test it on a SJLJ EH
> target ...

 1 file changed, 29 insertions(+), 16 deletions(-)
so not really simplify it, but not terrible either.

Here is the incremental (untested) diff of what handles that.

I don't have access to any standard SJLJ EH targets, but will try
--enable-sjlj-exceptions on x86_64-linux to see how far I get with that.

--- gcc/tree-cfgcleanup.c.jj	2019-02-26 16:52:33.828719803 +0100
+++ gcc/tree-cfgcleanup.c	2019-02-26 17:11:07.735576027 +0100
@@ -731,12 +731,7 @@ cleanup_tree_cfg_bb (basic_block bb)
    normally are effectively unreachable as well.  Additionally ignore
    __builtin_setjmp_receiver starting blocks, which have one FORCED_LABEL
    and which are always only reachable through EDGE_ABNORMAL edge.  They are
-   handled in cleanup_control_flow_pre.
-   Similarly, return true for EDGE_ABNORMAL edge from any basic block to
-   .ABNORMAL_DISPATCHER basic block if the latter block has no successors.
-   .ABNORMAL_DISPATCHER basic blocks that don't dispatch to anything are dead,
-   don't really need any EDGE_ABNORMAL edges to them and can be safely
-   removed.  */
+   handled in cleanup_control_flow_pre.  */
 
 static bool
 maybe_dead_abnormal_edge_p (edge e)
@@ -747,16 +742,7 @@ maybe_dead_abnormal_edge_p (edge e)
   gimple_stmt_iterator gsi = gsi_start_nondebug_after_labels_bb (e->src);
   gimple *g = gsi_stmt (gsi);
   if (!g || !gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
-    {
-      if (EDGE_COUNT (e->dest->succs) == 0)
-	{
-	  gsi = gsi_start_nondebug_after_labels_bb (e->dest);
-	  g = gsi_stmt (gsi);
-	  if (g && gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
-	    return true;
-	}
-      return false;
-    }
+    return false;
 
   tree target = NULL_TREE;
   for (gsi = gsi_start_bb (e->dest); !gsi_end_p (gsi); gsi_next (&gsi))
@@ -846,6 +832,7 @@ cleanup_control_flow_pre ()
   bitmap_clear (visited);
 
   vec<edge, va_gc> *setjmp_vec = NULL;
+  auto_vec<basic_block, 4> abnormal_dispatchers;
 
   stack.quick_push (ei_start (ENTRY_BLOCK_PTR_FOR_FN (cfun)->succs));
 
@@ -877,6 +864,16 @@ cleanup_control_flow_pre ()
 		stack.quick_push (ei_start (setjmp_vec));
 	    }
 
+	  if ((ei_edge (ei)->flags
+	       & (EDGE_ABNORMAL | EDGE_EH)) == EDGE_ABNORMAL)
+	    {
+	      gimple_stmt_iterator gsi
+		= gsi_start_nondebug_after_labels_bb (dest);
+	      gimple *g = gsi_stmt (gsi);
+	      if (g && gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
+		abnormal_dispatchers.safe_push (dest);
+	    }
+
 	  if (EDGE_COUNT (dest->succs) > 0)
 	    stack.quick_push (ei_start (dest->succs));
 	}
@@ -895,6 +892,22 @@ cleanup_control_flow_pre ()
 
   vec_free (setjmp_vec);
 
+  /* If we've marked .ABNORMAL_DISPATCHER basic block(s) as visited
+     above, but haven't marked any of their successors as visited,
+     unmark them now, so that they can be removed as useless.  */
+  basic_block dispatcher_bb;
+  unsigned int k;
+  FOR_EACH_VEC_ELT (abnormal_dispatchers, k, dispatcher_bb)
+    {
+      edge e;
+      edge_iterator ei;
+      FOR_EACH_EDGE (e, ei, dispatcher_bb->succs)
+	if (bitmap_bit_p (visited, e->dest->index))
+	  break;
+      if (e == NULL)
+	bitmap_clear_bit (visited, dispatcher_bb->index);
+    }
+
   set_dom_info_availability (CDI_DOMINATORS, saved_state);
 
   /* We are deleting BBs in non-reverse dominator order, make sure


	Jakub
Richard Biener Feb. 26, 2019, 5:03 p.m. UTC | #4
On Tue, 26 Feb 2019, Jakub Jelinek wrote:

> On Tue, Feb 26, 2019 at 04:08:21PM +0100, Richard Biener wrote:
> > I think we can have multiple .ABNORMAL_DISPATCHERs - at least in theory
> > via inlining, though I see code in the inliner avoiding multiple ones
> > at least for nonlocal goto, not sure if that covers all cases.
> 
> Ok.
> 
> > > Shall I modify the patch for that?
> > 
> > Might it even simplify the patch?  If not the only comment on the
> > original patch is that it would be nice to test it on a SJLJ EH
> > target ...
> 
>  1 file changed, 29 insertions(+), 16 deletions(-)
> so not really simplify it, but not terrible either.
> 
> Here is the incremental (untested) diff of what handles that.
> 
> I don't have access to any standard SJLJ EH targets, but will try
> --enable-sjlj-exceptions on x86_64-linux to see how far I get with that.

Indeed not terrible.  Maybe Eric can help with testing on a SJLJ
target (though IIRC Ada uses its own EH scheme on such targets?)

Richard.

> --- gcc/tree-cfgcleanup.c.jj	2019-02-26 16:52:33.828719803 +0100
> +++ gcc/tree-cfgcleanup.c	2019-02-26 17:11:07.735576027 +0100
> @@ -731,12 +731,7 @@ cleanup_tree_cfg_bb (basic_block bb)
>     normally are effectively unreachable as well.  Additionally ignore
>     __builtin_setjmp_receiver starting blocks, which have one FORCED_LABEL
>     and which are always only reachable through EDGE_ABNORMAL edge.  They are
> -   handled in cleanup_control_flow_pre.
> -   Similarly, return true for EDGE_ABNORMAL edge from any basic block to
> -   .ABNORMAL_DISPATCHER basic block if the latter block has no successors.
> -   .ABNORMAL_DISPATCHER basic blocks that don't dispatch to anything are dead,
> -   don't really need any EDGE_ABNORMAL edges to them and can be safely
> -   removed.  */
> +   handled in cleanup_control_flow_pre.  */
>  
>  static bool
>  maybe_dead_abnormal_edge_p (edge e)
> @@ -747,16 +742,7 @@ maybe_dead_abnormal_edge_p (edge e)
>    gimple_stmt_iterator gsi = gsi_start_nondebug_after_labels_bb (e->src);
>    gimple *g = gsi_stmt (gsi);
>    if (!g || !gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
> -    {
> -      if (EDGE_COUNT (e->dest->succs) == 0)
> -	{
> -	  gsi = gsi_start_nondebug_after_labels_bb (e->dest);
> -	  g = gsi_stmt (gsi);
> -	  if (g && gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
> -	    return true;
> -	}
> -      return false;
> -    }
> +    return false;
>  
>    tree target = NULL_TREE;
>    for (gsi = gsi_start_bb (e->dest); !gsi_end_p (gsi); gsi_next (&gsi))
> @@ -846,6 +832,7 @@ cleanup_control_flow_pre ()
>    bitmap_clear (visited);
>  
>    vec<edge, va_gc> *setjmp_vec = NULL;
> +  auto_vec<basic_block, 4> abnormal_dispatchers;
>  
>    stack.quick_push (ei_start (ENTRY_BLOCK_PTR_FOR_FN (cfun)->succs));
>  
> @@ -877,6 +864,16 @@ cleanup_control_flow_pre ()
>  		stack.quick_push (ei_start (setjmp_vec));
>  	    }
>  
> +	  if ((ei_edge (ei)->flags
> +	       & (EDGE_ABNORMAL | EDGE_EH)) == EDGE_ABNORMAL)
> +	    {
> +	      gimple_stmt_iterator gsi
> +		= gsi_start_nondebug_after_labels_bb (dest);
> +	      gimple *g = gsi_stmt (gsi);
> +	      if (g && gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
> +		abnormal_dispatchers.safe_push (dest);
> +	    }
> +
>  	  if (EDGE_COUNT (dest->succs) > 0)
>  	    stack.quick_push (ei_start (dest->succs));
>  	}
> @@ -895,6 +892,22 @@ cleanup_control_flow_pre ()
>  
>    vec_free (setjmp_vec);
>  
> +  /* If we've marked .ABNORMAL_DISPATCHER basic block(s) as visited
> +     above, but haven't marked any of their successors as visited,
> +     unmark them now, so that they can be removed as useless.  */
> +  basic_block dispatcher_bb;
> +  unsigned int k;
> +  FOR_EACH_VEC_ELT (abnormal_dispatchers, k, dispatcher_bb)
> +    {
> +      edge e;
> +      edge_iterator ei;
> +      FOR_EACH_EDGE (e, ei, dispatcher_bb->succs)
> +	if (bitmap_bit_p (visited, e->dest->index))
> +	  break;
> +      if (e == NULL)
> +	bitmap_clear_bit (visited, dispatcher_bb->index);
> +    }
> +
>    set_dom_info_availability (CDI_DOMINATORS, saved_state);
>  
>    /* We are deleting BBs in non-reverse dominator order, make sure
> 
> 
> 	Jakub
> 
>
Jakub Jelinek Feb. 26, 2019, 8:59 p.m. UTC | #5
On Tue, Feb 26, 2019 at 05:18:17PM +0100, Jakub Jelinek wrote:
> > > Shall I modify the patch for that?
> > 
> > Might it even simplify the patch?  If not the only comment on the
> > original patch is that it would be nice to test it on a SJLJ EH
> > target ...
> 
>  1 file changed, 29 insertions(+), 16 deletions(-)
> so not really simplify it, but not terrible either.
> 
> Here is the incremental (untested) diff of what handles that.
> 
> I don't have access to any standard SJLJ EH targets, but will try
> --enable-sjlj-exceptions on x86_64-linux to see how far I get with that.

Full updated patch that passed normal bootstrap/regtest on x86_64-linux and
i686-linux.

--enable-sjlj-exceptions bootstrap on x86_64-linux failed miserably,
some entry-points are removed from libgcc_s.so in that case and
make: relocation error: /lib64/libgc.so.1: symbol __gcc_personality_v0 version GCC_3.3.1 not defined in file libgcc_s.so.1 with link time reference
On the other side, even without SJLJ EH, the testsuite coverage is quite
good, at least during development of the patch I made several mistakes and
each time there were dozens to hundreds of failing tests in the testsuite,
including __builtin_setjmp, non-local goto, etc.

That said, if anybody is able to test this on some SJLJ setup, it would be
greatly appreciated.

2019-02-26  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/89280
	* tree-cfgcleanup.c (maybe_dead_abnormal_edge_p,
	builtin_setjmp_setup_bb): New functions.
	(cleanup_control_flow_pre): Ignore maybe_dead_abnormal_edge_p edges.
	When visiting __builtin_setjmp_setup block, queue in special
	setjmp_vec vector edges from .ABNORMAL_DISPATCHER to corresponding
	__builtin_setjmp_receiver.  Remove .ABNORMAL_DISPATCHER basic blocks
	from visited after the loop if they don't have any visited successor
	blocks.

	* gcc.c-torture/compile/pr89280.c: New test.
	* gcc.dg/torture/pr57147-2.c: Don't expect a setjmp after noreturn
	function.  Skip the test for -O0.

--- gcc/tree-cfgcleanup.c.jj	2019-02-23 01:14:03.032266203 +0100
+++ gcc/tree-cfgcleanup.c	2019-02-23 01:40:03.589681687 +0100
@@ -723,6 +723,97 @@ cleanup_tree_cfg_bb (basic_block bb)
   return false;
 }
 
+/* Return true if E is an EDGE_ABNORMAL edge for returns_twice calls,
+   i.e. one going from .ABNORMAL_DISPATCHER to basic block which doesn't
+   start with a forced or nonlocal label.  Calls which return twice can return
+   the second time only if they are called normally the first time, so basic
+   blocks which can be only entered through these abnormal edges but not
+   normally are effectively unreachable as well.  Additionally ignore
+   __builtin_setjmp_receiver starting blocks, which have one FORCED_LABEL
+   and which are always only reachable through EDGE_ABNORMAL edge.  They are
+   handled in cleanup_control_flow_pre.  */
+
+static bool
+maybe_dead_abnormal_edge_p (edge e)
+{
+  if ((e->flags & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL)
+    return false;
+
+  gimple_stmt_iterator gsi = gsi_start_nondebug_after_labels_bb (e->src);
+  gimple *g = gsi_stmt (gsi);
+  if (!g || !gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
+    return false;
+
+  tree target = NULL_TREE;
+  for (gsi = gsi_start_bb (e->dest); !gsi_end_p (gsi); gsi_next (&gsi))
+    if (glabel *label_stmt = dyn_cast <glabel *> (gsi_stmt (gsi)))
+      {
+	tree this_target = gimple_label_label (label_stmt);
+	if (DECL_NONLOCAL (this_target))
+	  return false;
+	if (FORCED_LABEL (this_target))
+	  {
+	    if (target)
+	      return false;
+	    target = this_target;
+	  }
+      }
+    else
+      break;
+
+  if (target)
+    {
+      /* If there was a single FORCED_LABEL, check for
+	 __builtin_setjmp_receiver with address of that label.  */
+      if (!gsi_end_p (gsi) && is_gimple_debug (gsi_stmt (gsi)))
+	gsi_next_nondebug (&gsi);
+      if (gsi_end_p (gsi))
+	return false;
+      if (!gimple_call_builtin_p (gsi_stmt (gsi), BUILT_IN_SETJMP_RECEIVER))
+	return false;
+
+      tree arg = gimple_call_arg (gsi_stmt (gsi), 0);
+      if (TREE_CODE (arg) != ADDR_EXPR || TREE_OPERAND (arg, 0) != target)
+	return false;
+    }
+  return true;
+}
+
+/* If BB is a basic block ending with __builtin_setjmp_setup, return edge
+   from .ABNORMAL_DISPATCHER basic block to corresponding
+   __builtin_setjmp_receiver basic block, otherwise return NULL.  */
+static edge
+builtin_setjmp_setup_bb (basic_block bb)
+{
+  if (EDGE_COUNT (bb->succs) != 2
+      || ((EDGE_SUCC (bb, 0)->flags
+	   & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL
+	  && (EDGE_SUCC (bb, 1)->flags
+	      & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL))
+    return NULL;
+
+  gimple_stmt_iterator gsi = gsi_last_nondebug_bb (bb);
+  if (gsi_end_p (gsi)
+      || !gimple_call_builtin_p (gsi_stmt (gsi), BUILT_IN_SETJMP_SETUP))
+    return NULL;
+
+  tree arg = gimple_call_arg (gsi_stmt (gsi), 1);
+  if (TREE_CODE (arg) != ADDR_EXPR
+      || TREE_CODE (TREE_OPERAND (arg, 0)) != LABEL_DECL)
+    return NULL;
+
+  basic_block recv_bb = label_to_block (cfun, TREE_OPERAND (arg, 0));
+  if (EDGE_COUNT (recv_bb->preds) != 1
+      || (EDGE_PRED (recv_bb, 0)->flags
+	  & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL
+      || (EDGE_SUCC (bb, 0)->dest != EDGE_PRED (recv_bb, 0)->src
+	  && EDGE_SUCC (bb, 1)->dest != EDGE_PRED (recv_bb, 0)->src))
+    return NULL;
+
+  /* EDGE_PRED (recv_bb, 0)->src should be the .ABNORMAL_DISPATCHER bb.  */
+  return EDGE_PRED (recv_bb, 0);
+}
+
 /* Do cleanup_control_flow_bb in PRE order.  */
 
 static bool
@@ -736,10 +827,13 @@ cleanup_control_flow_pre ()
   dom_state saved_state = dom_info_state (CDI_DOMINATORS);
   set_dom_info_availability (CDI_DOMINATORS, DOM_NONE);
 
-  auto_vec<edge_iterator, 20> stack (n_basic_blocks_for_fn (cfun) + 1);
+  auto_vec<edge_iterator, 20> stack (n_basic_blocks_for_fn (cfun) + 2);
   auto_sbitmap visited (last_basic_block_for_fn (cfun));
   bitmap_clear (visited);
 
+  vec<edge, va_gc> *setjmp_vec = NULL;
+  auto_vec<basic_block, 4> abnormal_dispatchers;
+
   stack.quick_push (ei_start (ENTRY_BLOCK_PTR_FOR_FN (cfun)->succs));
 
   while (! stack.is_empty ())
@@ -749,12 +843,37 @@ cleanup_control_flow_pre ()
       basic_block dest = ei_edge (ei)->dest;
 
       if (dest != EXIT_BLOCK_PTR_FOR_FN (cfun)
-	  && ! bitmap_bit_p (visited, dest->index))
+	  && !bitmap_bit_p (visited, dest->index)
+	  && (ei_container (ei) == setjmp_vec
+	      || !maybe_dead_abnormal_edge_p (ei_edge (ei))))
 	{
 	  bitmap_set_bit (visited, dest->index);
 	  /* We only possibly remove edges from DEST here, leaving
 	     possibly unreachable code in the IL.  */
 	  retval |= cleanup_control_flow_bb (dest);
+
+	  /* Check for __builtin_setjmp_setup.  Edges from .ABNORMAL_DISPATCH
+	     to __builtin_setjmp_receiver will be normally ignored by
+	     maybe_dead_abnormal_edge_p.  If DEST is a visited
+	     __builtin_setjmp_setup, queue edge from .ABNORMAL_DISPATCH
+	     to __builtin_setjmp_receiver, so that it will be visited too.  */
+	  if (edge e = builtin_setjmp_setup_bb (dest))
+	    {
+	      vec_safe_push (setjmp_vec, e);
+	      if (vec_safe_length (setjmp_vec) == 1)
+		stack.quick_push (ei_start (setjmp_vec));
+	    }
+
+	  if ((ei_edge (ei)->flags
+	       & (EDGE_ABNORMAL | EDGE_EH)) == EDGE_ABNORMAL)
+	    {
+	      gimple_stmt_iterator gsi
+		= gsi_start_nondebug_after_labels_bb (dest);
+	      gimple *g = gsi_stmt (gsi);
+	      if (g && gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
+		abnormal_dispatchers.safe_push (dest);
+	    }
+
 	  if (EDGE_COUNT (dest->succs) > 0)
 	    stack.quick_push (ei_start (dest->succs));
 	}
@@ -763,10 +882,32 @@ cleanup_control_flow_pre ()
 	  if (!ei_one_before_end_p (ei))
 	    ei_next (&stack.last ());
 	  else
-	    stack.pop ();
+	    {
+	      if (ei_container (ei) == setjmp_vec)
+		vec_safe_truncate (setjmp_vec, 0);
+	      stack.pop ();
+	    }
 	}
     }
 
+  vec_free (setjmp_vec);
+
+  /* If we've marked .ABNORMAL_DISPATCHER basic block(s) as visited
+     above, but haven't marked any of their successors as visited,
+     unmark them now, so that they can be removed as useless.  */
+  basic_block dispatcher_bb;
+  unsigned int k;
+  FOR_EACH_VEC_ELT (abnormal_dispatchers, k, dispatcher_bb)
+    {
+      edge e;
+      edge_iterator ei;
+      FOR_EACH_EDGE (e, ei, dispatcher_bb->succs)
+	if (bitmap_bit_p (visited, e->dest->index))
+	  break;
+      if (e == NULL)
+	bitmap_clear_bit (visited, dispatcher_bb->index);
+    }
+
   set_dom_info_availability (CDI_DOMINATORS, saved_state);
 
   /* We are deleting BBs in non-reverse dominator order, make sure
--- gcc/testsuite/gcc.c-torture/compile/pr89280.c.jj	2019-02-23 01:28:52.633838814 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr89280.c	2019-02-23 01:28:52.632838830 +0100
@@ -0,0 +1,48 @@
+/* PR tree-optimization/89280 */
+
+int a;
+void foo (void);
+__attribute__ ((returns_twice)) int bar (void);
+void baz (int, int);
+void *buf[5];
+
+static inline void
+inl (int x)
+{
+  while (x)
+    foo ();
+}
+
+void
+test1 (void)
+{
+  for (;;)
+    foo ();
+  baz (bar (), a);
+}
+
+void
+test2 (void)
+{
+  for (;;)
+    foo ();
+  baz (__builtin_setjmp (buf), a);
+  if (a)
+    __builtin_longjmp (buf, 1);
+}
+
+void
+test3 (void)
+{
+  inl (1);
+  baz (bar (), a);
+}
+
+void
+test4 (void)
+{
+  inl (2);
+  baz (__builtin_setjmp (buf), a);
+  if (a)
+    __builtin_longjmp (buf, 1);
+}
--- gcc/testsuite/gcc.dg/torture/pr57147-2.c.jj	2019-02-23 01:14:03.218263169 +0100
+++ gcc/testsuite/gcc.dg/torture/pr57147-2.c	2019-02-23 01:42:41.529079696 +0100
@@ -1,6 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-fdump-tree-optimized" } */
 /* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } { "" } } */
+/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
 /* { dg-require-effective-target indirect_jumps } */
 
 struct __jmp_buf_tag {};
@@ -19,4 +20,4 @@ void TestSyscall(void)
   _setjmp (g_return_jmp_buf);
 }
 
-/* { dg-final { scan-tree-dump "setjmp" "optimized" } } */
+/* { dg-final { scan-tree-dump-not "setjmp" "optimized" } } */


	Jakub
Richard Biener Feb. 27, 2019, 8:20 a.m. UTC | #6
On Tue, 26 Feb 2019, Jakub Jelinek wrote:

> On Tue, Feb 26, 2019 at 05:18:17PM +0100, Jakub Jelinek wrote:
> > > > Shall I modify the patch for that?
> > > 
> > > Might it even simplify the patch?  If not the only comment on the
> > > original patch is that it would be nice to test it on a SJLJ EH
> > > target ...
> > 
> >  1 file changed, 29 insertions(+), 16 deletions(-)
> > so not really simplify it, but not terrible either.
> > 
> > Here is the incremental (untested) diff of what handles that.
> > 
> > I don't have access to any standard SJLJ EH targets, but will try
> > --enable-sjlj-exceptions on x86_64-linux to see how far I get with that.
> 
> Full updated patch that passed normal bootstrap/regtest on x86_64-linux and
> i686-linux.
> 
> --enable-sjlj-exceptions bootstrap on x86_64-linux failed miserably,
> some entry-points are removed from libgcc_s.so in that case and
> make: relocation error: /lib64/libgc.so.1: symbol __gcc_personality_v0 version GCC_3.3.1 not defined in file libgcc_s.so.1 with link time reference
> On the other side, even without SJLJ EH, the testsuite coverage is quite
> good, at least during development of the patch I made several mistakes and
> each time there were dozens to hundreds of failing tests in the testsuite,
> including __builtin_setjmp, non-local goto, etc.
> 
> That said, if anybody is able to test this on some SJLJ setup, it would be
> greatly appreciated.

Patch is OK.  I suppose auto-testers will pick up fallout and we
can always revert...

Richard.

> 2019-02-26  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/89280
> 	* tree-cfgcleanup.c (maybe_dead_abnormal_edge_p,
> 	builtin_setjmp_setup_bb): New functions.
> 	(cleanup_control_flow_pre): Ignore maybe_dead_abnormal_edge_p edges.
> 	When visiting __builtin_setjmp_setup block, queue in special
> 	setjmp_vec vector edges from .ABNORMAL_DISPATCHER to corresponding
> 	__builtin_setjmp_receiver.  Remove .ABNORMAL_DISPATCHER basic blocks
> 	from visited after the loop if they don't have any visited successor
> 	blocks.
> 
> 	* gcc.c-torture/compile/pr89280.c: New test.
> 	* gcc.dg/torture/pr57147-2.c: Don't expect a setjmp after noreturn
> 	function.  Skip the test for -O0.
> 
> --- gcc/tree-cfgcleanup.c.jj	2019-02-23 01:14:03.032266203 +0100
> +++ gcc/tree-cfgcleanup.c	2019-02-23 01:40:03.589681687 +0100
> @@ -723,6 +723,97 @@ cleanup_tree_cfg_bb (basic_block bb)
>    return false;
>  }
>  
> +/* Return true if E is an EDGE_ABNORMAL edge for returns_twice calls,
> +   i.e. one going from .ABNORMAL_DISPATCHER to basic block which doesn't
> +   start with a forced or nonlocal label.  Calls which return twice can return
> +   the second time only if they are called normally the first time, so basic
> +   blocks which can be only entered through these abnormal edges but not
> +   normally are effectively unreachable as well.  Additionally ignore
> +   __builtin_setjmp_receiver starting blocks, which have one FORCED_LABEL
> +   and which are always only reachable through EDGE_ABNORMAL edge.  They are
> +   handled in cleanup_control_flow_pre.  */
> +
> +static bool
> +maybe_dead_abnormal_edge_p (edge e)
> +{
> +  if ((e->flags & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL)
> +    return false;
> +
> +  gimple_stmt_iterator gsi = gsi_start_nondebug_after_labels_bb (e->src);
> +  gimple *g = gsi_stmt (gsi);
> +  if (!g || !gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
> +    return false;
> +
> +  tree target = NULL_TREE;
> +  for (gsi = gsi_start_bb (e->dest); !gsi_end_p (gsi); gsi_next (&gsi))
> +    if (glabel *label_stmt = dyn_cast <glabel *> (gsi_stmt (gsi)))
> +      {
> +	tree this_target = gimple_label_label (label_stmt);
> +	if (DECL_NONLOCAL (this_target))
> +	  return false;
> +	if (FORCED_LABEL (this_target))
> +	  {
> +	    if (target)
> +	      return false;
> +	    target = this_target;
> +	  }
> +      }
> +    else
> +      break;
> +
> +  if (target)
> +    {
> +      /* If there was a single FORCED_LABEL, check for
> +	 __builtin_setjmp_receiver with address of that label.  */
> +      if (!gsi_end_p (gsi) && is_gimple_debug (gsi_stmt (gsi)))
> +	gsi_next_nondebug (&gsi);
> +      if (gsi_end_p (gsi))
> +	return false;
> +      if (!gimple_call_builtin_p (gsi_stmt (gsi), BUILT_IN_SETJMP_RECEIVER))
> +	return false;
> +
> +      tree arg = gimple_call_arg (gsi_stmt (gsi), 0);
> +      if (TREE_CODE (arg) != ADDR_EXPR || TREE_OPERAND (arg, 0) != target)
> +	return false;
> +    }
> +  return true;
> +}
> +
> +/* If BB is a basic block ending with __builtin_setjmp_setup, return edge
> +   from .ABNORMAL_DISPATCHER basic block to corresponding
> +   __builtin_setjmp_receiver basic block, otherwise return NULL.  */
> +static edge
> +builtin_setjmp_setup_bb (basic_block bb)
> +{
> +  if (EDGE_COUNT (bb->succs) != 2
> +      || ((EDGE_SUCC (bb, 0)->flags
> +	   & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL
> +	  && (EDGE_SUCC (bb, 1)->flags
> +	      & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL))
> +    return NULL;
> +
> +  gimple_stmt_iterator gsi = gsi_last_nondebug_bb (bb);
> +  if (gsi_end_p (gsi)
> +      || !gimple_call_builtin_p (gsi_stmt (gsi), BUILT_IN_SETJMP_SETUP))
> +    return NULL;
> +
> +  tree arg = gimple_call_arg (gsi_stmt (gsi), 1);
> +  if (TREE_CODE (arg) != ADDR_EXPR
> +      || TREE_CODE (TREE_OPERAND (arg, 0)) != LABEL_DECL)
> +    return NULL;
> +
> +  basic_block recv_bb = label_to_block (cfun, TREE_OPERAND (arg, 0));
> +  if (EDGE_COUNT (recv_bb->preds) != 1
> +      || (EDGE_PRED (recv_bb, 0)->flags
> +	  & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL
> +      || (EDGE_SUCC (bb, 0)->dest != EDGE_PRED (recv_bb, 0)->src
> +	  && EDGE_SUCC (bb, 1)->dest != EDGE_PRED (recv_bb, 0)->src))
> +    return NULL;
> +
> +  /* EDGE_PRED (recv_bb, 0)->src should be the .ABNORMAL_DISPATCHER bb.  */
> +  return EDGE_PRED (recv_bb, 0);
> +}
> +
>  /* Do cleanup_control_flow_bb in PRE order.  */
>  
>  static bool
> @@ -736,10 +827,13 @@ cleanup_control_flow_pre ()
>    dom_state saved_state = dom_info_state (CDI_DOMINATORS);
>    set_dom_info_availability (CDI_DOMINATORS, DOM_NONE);
>  
> -  auto_vec<edge_iterator, 20> stack (n_basic_blocks_for_fn (cfun) + 1);
> +  auto_vec<edge_iterator, 20> stack (n_basic_blocks_for_fn (cfun) + 2);
>    auto_sbitmap visited (last_basic_block_for_fn (cfun));
>    bitmap_clear (visited);
>  
> +  vec<edge, va_gc> *setjmp_vec = NULL;
> +  auto_vec<basic_block, 4> abnormal_dispatchers;
> +
>    stack.quick_push (ei_start (ENTRY_BLOCK_PTR_FOR_FN (cfun)->succs));
>  
>    while (! stack.is_empty ())
> @@ -749,12 +843,37 @@ cleanup_control_flow_pre ()
>        basic_block dest = ei_edge (ei)->dest;
>  
>        if (dest != EXIT_BLOCK_PTR_FOR_FN (cfun)
> -	  && ! bitmap_bit_p (visited, dest->index))
> +	  && !bitmap_bit_p (visited, dest->index)
> +	  && (ei_container (ei) == setjmp_vec
> +	      || !maybe_dead_abnormal_edge_p (ei_edge (ei))))
>  	{
>  	  bitmap_set_bit (visited, dest->index);
>  	  /* We only possibly remove edges from DEST here, leaving
>  	     possibly unreachable code in the IL.  */
>  	  retval |= cleanup_control_flow_bb (dest);
> +
> +	  /* Check for __builtin_setjmp_setup.  Edges from .ABNORMAL_DISPATCH
> +	     to __builtin_setjmp_receiver will be normally ignored by
> +	     maybe_dead_abnormal_edge_p.  If DEST is a visited
> +	     __builtin_setjmp_setup, queue edge from .ABNORMAL_DISPATCH
> +	     to __builtin_setjmp_receiver, so that it will be visited too.  */
> +	  if (edge e = builtin_setjmp_setup_bb (dest))
> +	    {
> +	      vec_safe_push (setjmp_vec, e);
> +	      if (vec_safe_length (setjmp_vec) == 1)
> +		stack.quick_push (ei_start (setjmp_vec));
> +	    }
> +
> +	  if ((ei_edge (ei)->flags
> +	       & (EDGE_ABNORMAL | EDGE_EH)) == EDGE_ABNORMAL)
> +	    {
> +	      gimple_stmt_iterator gsi
> +		= gsi_start_nondebug_after_labels_bb (dest);
> +	      gimple *g = gsi_stmt (gsi);
> +	      if (g && gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
> +		abnormal_dispatchers.safe_push (dest);
> +	    }
> +
>  	  if (EDGE_COUNT (dest->succs) > 0)
>  	    stack.quick_push (ei_start (dest->succs));
>  	}
> @@ -763,10 +882,32 @@ cleanup_control_flow_pre ()
>  	  if (!ei_one_before_end_p (ei))
>  	    ei_next (&stack.last ());
>  	  else
> -	    stack.pop ();
> +	    {
> +	      if (ei_container (ei) == setjmp_vec)
> +		vec_safe_truncate (setjmp_vec, 0);
> +	      stack.pop ();
> +	    }
>  	}
>      }
>  
> +  vec_free (setjmp_vec);
> +
> +  /* If we've marked .ABNORMAL_DISPATCHER basic block(s) as visited
> +     above, but haven't marked any of their successors as visited,
> +     unmark them now, so that they can be removed as useless.  */
> +  basic_block dispatcher_bb;
> +  unsigned int k;
> +  FOR_EACH_VEC_ELT (abnormal_dispatchers, k, dispatcher_bb)
> +    {
> +      edge e;
> +      edge_iterator ei;
> +      FOR_EACH_EDGE (e, ei, dispatcher_bb->succs)
> +	if (bitmap_bit_p (visited, e->dest->index))
> +	  break;
> +      if (e == NULL)
> +	bitmap_clear_bit (visited, dispatcher_bb->index);
> +    }
> +
>    set_dom_info_availability (CDI_DOMINATORS, saved_state);
>  
>    /* We are deleting BBs in non-reverse dominator order, make sure
> --- gcc/testsuite/gcc.c-torture/compile/pr89280.c.jj	2019-02-23 01:28:52.633838814 +0100
> +++ gcc/testsuite/gcc.c-torture/compile/pr89280.c	2019-02-23 01:28:52.632838830 +0100
> @@ -0,0 +1,48 @@
> +/* PR tree-optimization/89280 */
> +
> +int a;
> +void foo (void);
> +__attribute__ ((returns_twice)) int bar (void);
> +void baz (int, int);
> +void *buf[5];
> +
> +static inline void
> +inl (int x)
> +{
> +  while (x)
> +    foo ();
> +}
> +
> +void
> +test1 (void)
> +{
> +  for (;;)
> +    foo ();
> +  baz (bar (), a);
> +}
> +
> +void
> +test2 (void)
> +{
> +  for (;;)
> +    foo ();
> +  baz (__builtin_setjmp (buf), a);
> +  if (a)
> +    __builtin_longjmp (buf, 1);
> +}
> +
> +void
> +test3 (void)
> +{
> +  inl (1);
> +  baz (bar (), a);
> +}
> +
> +void
> +test4 (void)
> +{
> +  inl (2);
> +  baz (__builtin_setjmp (buf), a);
> +  if (a)
> +    __builtin_longjmp (buf, 1);
> +}
> --- gcc/testsuite/gcc.dg/torture/pr57147-2.c.jj	2019-02-23 01:14:03.218263169 +0100
> +++ gcc/testsuite/gcc.dg/torture/pr57147-2.c	2019-02-23 01:42:41.529079696 +0100
> @@ -1,6 +1,7 @@
>  /* { dg-do compile } */
>  /* { dg-options "-fdump-tree-optimized" } */
>  /* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } { "" } } */
> +/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
>  /* { dg-require-effective-target indirect_jumps } */
>  
>  struct __jmp_buf_tag {};
> @@ -19,4 +20,4 @@ void TestSyscall(void)
>    _setjmp (g_return_jmp_buf);
>  }
>  
> -/* { dg-final { scan-tree-dump "setjmp" "optimized" } } */
> +/* { dg-final { scan-tree-dump-not "setjmp" "optimized" } } */
> 
> 
> 	Jakub
> 
>
H.J. Lu Feb. 27, 2019, 3:03 p.m. UTC | #7
On Wed, Feb 27, 2019 at 12:20 AM Richard Biener <rguenther@suse.de> wrote:
>
> On Tue, 26 Feb 2019, Jakub Jelinek wrote:
>
> > On Tue, Feb 26, 2019 at 05:18:17PM +0100, Jakub Jelinek wrote:
> > > > > Shall I modify the patch for that?
> > > >
> > > > Might it even simplify the patch?  If not the only comment on the
> > > > original patch is that it would be nice to test it on a SJLJ EH
> > > > target ...
> > >
> > >  1 file changed, 29 insertions(+), 16 deletions(-)
> > > so not really simplify it, but not terrible either.
> > >
> > > Here is the incremental (untested) diff of what handles that.
> > >
> > > I don't have access to any standard SJLJ EH targets, but will try
> > > --enable-sjlj-exceptions on x86_64-linux to see how far I get with that.
> >
> > Full updated patch that passed normal bootstrap/regtest on x86_64-linux and
> > i686-linux.
> >
> > --enable-sjlj-exceptions bootstrap on x86_64-linux failed miserably,
> > some entry-points are removed from libgcc_s.so in that case and
> > make: relocation error: /lib64/libgc.so.1: symbol __gcc_personality_v0 version GCC_3.3.1 not defined in file libgcc_s.so.1 with link time reference
> > On the other side, even without SJLJ EH, the testsuite coverage is quite
> > good, at least during development of the patch I made several mistakes and
> > each time there were dozens to hundreds of failing tests in the testsuite,
> > including __builtin_setjmp, non-local goto, etc.
> >
> > That said, if anybody is able to test this on some SJLJ setup, it would be
> > greatly appreciated.
>
> Patch is OK.  I suppose auto-testers will pick up fallout and we
> can always revert...
>
> Richard.
>
> > 2019-02-26  Jakub Jelinek  <jakub@redhat.com>
> >
> >       PR tree-optimization/89280
> >       * tree-cfgcleanup.c (maybe_dead_abnormal_edge_p,
> >       builtin_setjmp_setup_bb): New functions.
> >       (cleanup_control_flow_pre): Ignore maybe_dead_abnormal_edge_p edges.
> >       When visiting __builtin_setjmp_setup block, queue in special
> >       setjmp_vec vector edges from .ABNORMAL_DISPATCHER to corresponding
> >       __builtin_setjmp_receiver.  Remove .ABNORMAL_DISPATCHER basic blocks
> >       from visited after the loop if they don't have any visited successor
> >       blocks.
> >
> >       * gcc.c-torture/compile/pr89280.c: New test.
> >       * gcc.dg/torture/pr57147-2.c: Don't expect a setjmp after noreturn
> >       function.  Skip the test for -O0.
> >

I got

FAIL: gcc.dg/torture/pr57147-2.c   -O1   scan-tree-dump-not optimized "setjmp"
FAIL: gcc.dg/torture/pr57147-2.c   -O2 -flto -fno-use-linker-plugin
-flto-partition=none   scan-tree-dump-not optimized "setjmp"
FAIL: gcc.dg/torture/pr57147-2.c   -O2   scan-tree-dump-not optimized "setjmp"
FAIL: gcc.dg/torture/pr57147-2.c   -O3 -g   scan-tree-dump-not
optimized "setjmp"
FAIL: gcc.dg/torture/pr57147-2.c   -Os   scan-tree-dump-not optimized "setjmp"

with unix/-fpic on x86.   Should test pass with -fPIC?
Jakub Jelinek Feb. 27, 2019, 3:24 p.m. UTC | #8
On Wed, Feb 27, 2019 at 07:03:06AM -0800, H.J. Lu wrote:
> I got
> 
> FAIL: gcc.dg/torture/pr57147-2.c   -O1   scan-tree-dump-not optimized "setjmp"
> FAIL: gcc.dg/torture/pr57147-2.c   -O2 -flto -fno-use-linker-plugin
> -flto-partition=none   scan-tree-dump-not optimized "setjmp"
> FAIL: gcc.dg/torture/pr57147-2.c   -O2   scan-tree-dump-not optimized "setjmp"
> FAIL: gcc.dg/torture/pr57147-2.c   -O3 -g   scan-tree-dump-not
> optimized "setjmp"
> FAIL: gcc.dg/torture/pr57147-2.c   -Os   scan-tree-dump-not optimized "setjmp"
> 
> with unix/-fpic on x86.   Should test pass with -fPIC?

Fixed thusly, committed as obvious.
Before my patch it was testing that setjmp actually remains, but it was
after noreturn function, I've changed it to test that it is optimized away,
because the new code does optimize unreachable setjmp away.
With -fpic we assume the function can be interposed and don't consider it
noreturn though.

2019-02-27  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/89280
	* gcc.dg/torture/pr57147-2.c (SetNaClSwitchExpectations): Add static
	keyword.

--- gcc/testsuite/gcc.dg/torture/pr57147-2.c.jj	2019-02-27 09:40:01.860502737 +0100
+++ gcc/testsuite/gcc.dg/torture/pr57147-2.c	2019-02-27 16:20:27.957843867 +0100
@@ -10,7 +10,7 @@ extern int _setjmp (struct __jmp_buf_tag
 
 jmp_buf g_return_jmp_buf;
 
-void SetNaClSwitchExpectations (void)
+static void SetNaClSwitchExpectations (void)
 {
   __builtin_longjmp (g_return_jmp_buf, 1);
 }


	Jakub
diff mbox series

Patch

--- gcc/tree-cfgcleanup.c.jj	2019-02-23 01:14:03.032266203 +0100
+++ gcc/tree-cfgcleanup.c	2019-02-23 01:40:03.589681687 +0100
@@ -723,6 +723,111 @@  cleanup_tree_cfg_bb (basic_block bb)
   return false;
 }
 
+/* Return true if E is an EDGE_ABNORMAL edge for returns_twice calls,
+   i.e. one going from .ABNORMAL_DISPATCHER to basic block which doesn't
+   start with a forced or nonlocal label.  Calls which return twice can return
+   the second time only if they are called normally the first time, so basic
+   blocks which can be only entered through these abnormal edges but not
+   normally are effectively unreachable as well.  Additionally ignore
+   __builtin_setjmp_receiver starting blocks, which have one FORCED_LABEL
+   and which are always only reachable through EDGE_ABNORMAL edge.  They are
+   handled in cleanup_control_flow_pre.
+   Similarly, return true for EDGE_ABNORMAL edge from any basic block to
+   .ABNORMAL_DISPATCHER basic block if the latter block has no successors.
+   .ABNORMAL_DISPATCHER basic blocks that don't dispatch to anything are dead,
+   don't really need any EDGE_ABNORMAL edges to them and can be safely
+   removed.  */
+
+static bool
+maybe_dead_abnormal_edge_p (edge e)
+{
+  if ((e->flags & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL)
+    return false;
+
+  gimple_stmt_iterator gsi = gsi_start_nondebug_after_labels_bb (e->src);
+  gimple *g = gsi_stmt (gsi);
+  if (!g || !gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
+    {
+      if (EDGE_COUNT (e->dest->succs) == 0)
+	{
+	  gsi = gsi_start_nondebug_after_labels_bb (e->dest);
+	  g = gsi_stmt (gsi);
+	  if (g && gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
+	    return true;
+	}
+      return false;
+    }
+
+  tree target = NULL_TREE;
+  for (gsi = gsi_start_bb (e->dest); !gsi_end_p (gsi); gsi_next (&gsi))
+    if (glabel *label_stmt = dyn_cast <glabel *> (gsi_stmt (gsi)))
+      {
+	tree this_target = gimple_label_label (label_stmt);
+	if (DECL_NONLOCAL (this_target))
+	  return false;
+	if (FORCED_LABEL (this_target))
+	  {
+	    if (target)
+	      return false;
+	    target = this_target;
+	  }
+      }
+    else
+      break;
+
+  if (target)
+    {
+      /* If there was a single FORCED_LABEL, check for
+	 __builtin_setjmp_receiver with address of that label.  */
+      if (!gsi_end_p (gsi) && is_gimple_debug (gsi_stmt (gsi)))
+	gsi_next_nondebug (&gsi);
+      if (gsi_end_p (gsi))
+	return false;
+      if (!gimple_call_builtin_p (gsi_stmt (gsi), BUILT_IN_SETJMP_RECEIVER))
+	return false;
+
+      tree arg = gimple_call_arg (gsi_stmt (gsi), 0);
+      if (TREE_CODE (arg) != ADDR_EXPR || TREE_OPERAND (arg, 0) != target)
+	return false;
+    }
+  return true;
+}
+
+/* If BB is a basic block ending with __builtin_setjmp_setup, return edge
+   from .ABNORMAL_DISPATCHER basic block to corresponding
+   __builtin_setjmp_receiver basic block, otherwise return NULL.  */
+static edge
+builtin_setjmp_setup_bb (basic_block bb)
+{
+  if (EDGE_COUNT (bb->succs) != 2
+      || ((EDGE_SUCC (bb, 0)->flags
+	   & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL
+	  && (EDGE_SUCC (bb, 1)->flags
+	      & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL))
+    return NULL;
+
+  gimple_stmt_iterator gsi = gsi_last_nondebug_bb (bb);
+  if (gsi_end_p (gsi)
+      || !gimple_call_builtin_p (gsi_stmt (gsi), BUILT_IN_SETJMP_SETUP))
+    return NULL;
+
+  tree arg = gimple_call_arg (gsi_stmt (gsi), 1);
+  if (TREE_CODE (arg) != ADDR_EXPR
+      || TREE_CODE (TREE_OPERAND (arg, 0)) != LABEL_DECL)
+    return NULL;
+
+  basic_block recv_bb = label_to_block (cfun, TREE_OPERAND (arg, 0));
+  if (EDGE_COUNT (recv_bb->preds) != 1
+      || (EDGE_PRED (recv_bb, 0)->flags
+	  & (EDGE_ABNORMAL | EDGE_EH)) != EDGE_ABNORMAL
+      || (EDGE_SUCC (bb, 0)->dest != EDGE_PRED (recv_bb, 0)->src
+	  && EDGE_SUCC (bb, 1)->dest != EDGE_PRED (recv_bb, 0)->src))
+    return NULL;
+
+  /* EDGE_PRED (recv_bb, 0)->src should be the .ABNORMAL_DISPATCHER bb.  */
+  return EDGE_PRED (recv_bb, 0);
+}
+
 /* Do cleanup_control_flow_bb in PRE order.  */
 
 static bool
@@ -736,10 +841,12 @@  cleanup_control_flow_pre ()
   dom_state saved_state = dom_info_state (CDI_DOMINATORS);
   set_dom_info_availability (CDI_DOMINATORS, DOM_NONE);
 
-  auto_vec<edge_iterator, 20> stack (n_basic_blocks_for_fn (cfun) + 1);
+  auto_vec<edge_iterator, 20> stack (n_basic_blocks_for_fn (cfun) + 2);
   auto_sbitmap visited (last_basic_block_for_fn (cfun));
   bitmap_clear (visited);
 
+  vec<edge, va_gc> *setjmp_vec = NULL;
+
   stack.quick_push (ei_start (ENTRY_BLOCK_PTR_FOR_FN (cfun)->succs));
 
   while (! stack.is_empty ())
@@ -749,12 +856,27 @@  cleanup_control_flow_pre ()
       basic_block dest = ei_edge (ei)->dest;
 
       if (dest != EXIT_BLOCK_PTR_FOR_FN (cfun)
-	  && ! bitmap_bit_p (visited, dest->index))
+	  && !bitmap_bit_p (visited, dest->index)
+	  && (ei_container (ei) == setjmp_vec
+	      || !maybe_dead_abnormal_edge_p (ei_edge (ei))))
 	{
 	  bitmap_set_bit (visited, dest->index);
 	  /* We only possibly remove edges from DEST here, leaving
 	     possibly unreachable code in the IL.  */
 	  retval |= cleanup_control_flow_bb (dest);
+
+	  /* Check for __builtin_setjmp_setup.  Edges from .ABNORMAL_DISPATCH
+	     to __builtin_setjmp_receiver will be normally ignored by
+	     maybe_dead_abnormal_edge_p.  If DEST is a visited
+	     __builtin_setjmp_setup, queue edge from .ABNORMAL_DISPATCH
+	     to __builtin_setjmp_receiver, so that it will be visited too.  */
+	  if (edge e = builtin_setjmp_setup_bb (dest))
+	    {
+	      vec_safe_push (setjmp_vec, e);
+	      if (vec_safe_length (setjmp_vec) == 1)
+		stack.quick_push (ei_start (setjmp_vec));
+	    }
+
 	  if (EDGE_COUNT (dest->succs) > 0)
 	    stack.quick_push (ei_start (dest->succs));
 	}
@@ -763,10 +885,16 @@  cleanup_control_flow_pre ()
 	  if (!ei_one_before_end_p (ei))
 	    ei_next (&stack.last ());
 	  else
-	    stack.pop ();
+	    {
+	      if (ei_container (ei) == setjmp_vec)
+		vec_safe_truncate (setjmp_vec, 0);
+	      stack.pop ();
+	    }
 	}
     }
 
+  vec_free (setjmp_vec);
+
   set_dom_info_availability (CDI_DOMINATORS, saved_state);
 
   /* We are deleting BBs in non-reverse dominator order, make sure
--- gcc/testsuite/gcc.c-torture/compile/pr89280.c.jj	2019-02-23 01:28:52.633838814 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr89280.c	2019-02-23 01:28:52.632838830 +0100
@@ -0,0 +1,48 @@ 
+/* PR tree-optimization/89280 */
+
+int a;
+void foo (void);
+__attribute__ ((returns_twice)) int bar (void);
+void baz (int, int);
+void *buf[5];
+
+static inline void
+inl (int x)
+{
+  while (x)
+    foo ();
+}
+
+void
+test1 (void)
+{
+  for (;;)
+    foo ();
+  baz (bar (), a);
+}
+
+void
+test2 (void)
+{
+  for (;;)
+    foo ();
+  baz (__builtin_setjmp (buf), a);
+  if (a)
+    __builtin_longjmp (buf, 1);
+}
+
+void
+test3 (void)
+{
+  inl (1);
+  baz (bar (), a);
+}
+
+void
+test4 (void)
+{
+  inl (2);
+  baz (__builtin_setjmp (buf), a);
+  if (a)
+    __builtin_longjmp (buf, 1);
+}
--- gcc/testsuite/gcc.dg/torture/pr57147-2.c.jj	2019-02-23 01:14:03.218263169 +0100
+++ gcc/testsuite/gcc.dg/torture/pr57147-2.c	2019-02-23 01:42:41.529079696 +0100
@@ -1,6 +1,7 @@ 
 /* { dg-do compile } */
 /* { dg-options "-fdump-tree-optimized" } */
 /* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } { "" } } */
+/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
 /* { dg-require-effective-target indirect_jumps } */
 
 struct __jmp_buf_tag {};
@@ -19,4 +20,4 @@  void TestSyscall(void)
   _setjmp (g_return_jmp_buf);
 }
 
-/* { dg-final { scan-tree-dump "setjmp" "optimized" } } */
+/* { dg-final { scan-tree-dump-not "setjmp" "optimized" } } */