Re: [PATCH] tree-optimization/108691 - indirect calls to setjmp

[Richard Biener <rguenther@suse.de>]

On Mon, 13 Feb 2023, Jakub Jelinek wrote:

> On Mon, Feb 13, 2023 at 12:41:48PM +0000, Richard Biener wrote:
> > > Could we e.g. prevent turning such indirect calls into direct calls?
> > 
> > We do exactly have gimple_call_fntype and gimple_call_ctrl_altering_p
> > to not require special-casing indirect to direct call promotion here.
> 
> Ah, so if we make returns_twice apply to function types, then we could
> just compare if gimple_call_fntype has also returns_twice and if not, not
> consider it actually returns_twice.
> 
> > > Anyway, notice_special_calls is called in various spots, not just DCE,
> > > wouldn't it be better to simply not set calls_setjmp flag in there if
> > > the current function already has cfg and the call isn't ctrl altering?
> > 
> > I thought about changing gimple_call_flags instead, filtering out
> > ECF_RETURNS_TWICE.  I just didn't make up my mind on what
> > property to key at (and to require 'cfun' to be set to query it).
> > But sure, changing notice_special_calls also works - the only
> > other relevant caller is the inliner I think, and that could be
> > replaced by caller |= callee of the two flags tracked instead of
> > re-scanning each inlined stmt.
> > 
> > Would you be happy with changing notice_special_calls, dropping the
> > DCE hunk but keeping the cfgexpand/calls.cc hunks?
> 
> I think so.

I'm testing the following and am queueing the second patch below
for next stage1.

Richard.


From d5e62f27489c1e7a8696a85e7bc98cc0a26564d2 Mon Sep 17 00:00:00 2001
From: Richard Biener <rguenther@suse.de>
Date: Mon, 13 Feb 2023 10:41:51 +0100
Subject: [PATCH 1/2] tree-optimization/108691 - indirect calls to setjmp
To: gcc-patches@gcc.gnu.org

DCE now chokes on indirect setjmp calls becoming direct because
that exposes them too late to be subject to abnormal edge creation.
The following patch honors gimple_call_ctrl_altering for those and
_not_ treat formerly indirect calls to setjmp as calls to setjmp
in notice_special_calls.

Unfortunately there's no way to have an indirect call to setjmp
properly annotated (the returns_twice attribute is ignored on types).

RTL expansion late discovers returns-twice for the purpose of
adding REG_SETJMP notes and also sets ->calls_setjmp
(instead of asserting it is set).  There's no good way to
transfer proper knowledge around here so I'm using ->calls_setjmp
as a flag to indicate whether gimple_call_ctrl_altering_p was set.

	PR tree-optimization/108691
	* tree-cfg.cc (notice_special_calls): When the CFG is built
	honor gimple_call_ctrl_altering_p.
	* cfgexpand.cc (expand_call_stmt): Clear cfun->calls_setjmp
	temporarily if the call is not control-altering.
	* calls.cc (emit_call_1): Do not add REG_SETJMP if
	cfun->calls_setjmp is not set.  Do not alter cfun->calls_setjmp.

	* gcc.dg/pr108691.c: New testcase.
---
 gcc/calls.cc                    | 10 +++++-----
 gcc/cfgexpand.cc                |  7 +++++++
 gcc/testsuite/gcc.dg/pr108691.c |  9 +++++++++
 gcc/tree-cfg.cc                 |  4 +++-
 4 files changed, 24 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr108691.c

diff --git a/gcc/calls.cc b/gcc/calls.cc
index 4d7f6c3d291..0242d52cfb3 100644
--- a/gcc/calls.cc
+++ b/gcc/calls.cc
@@ -506,11 +506,11 @@ emit_call_1 (rtx funexp, tree fntree ATTRIBUTE_UNUSED, tree fndecl ATTRIBUTE_UNU
   if (ecf_flags & ECF_NORETURN)
     add_reg_note (call_insn, REG_NORETURN, const0_rtx);
 
-  if (ecf_flags & ECF_RETURNS_TWICE)
-    {
-      add_reg_note (call_insn, REG_SETJMP, const0_rtx);
-      cfun->calls_setjmp = 1;
-    }
+  if (ecf_flags & ECF_RETURNS_TWICE
+      /* We rely on GIMPLE setting this flag and here use it to
+	 catch formerly indirect and not control-altering calls.  */
+      && cfun->calls_setjmp)
+    add_reg_note (call_insn, REG_SETJMP, const0_rtx);
 
   SIBLING_CALL_P (call_insn) = ((ecf_flags & ECF_SIBCALL) != 0);
 
diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
index 25b1558dcb9..ab143a6d2d3 100644
--- a/gcc/cfgexpand.cc
+++ b/gcc/cfgexpand.cc
@@ -2808,6 +2808,11 @@ expand_call_stmt (gcall *stmt)
   /* Must come after copying location.  */
   copy_warning (exp, stmt);
 
+  /* For calls that do not alter control flow avoid REG_SETJMP notes.  */
+  bool saved_calls_setjmp = cfun->calls_setjmp;
+  if (!gimple_call_ctrl_altering_p (stmt))
+    cfun->calls_setjmp = false;
+
   /* Ensure RTL is created for debug args.  */
   if (decl && DECL_HAS_DEBUG_ARGS_P (decl))
     {
@@ -2846,6 +2851,8 @@ expand_call_stmt (gcall *stmt)
     }
 
   mark_transaction_restart_calls (stmt);
+
+  cfun->calls_setjmp = saved_calls_setjmp;
 }
 
 
diff --git a/gcc/testsuite/gcc.dg/pr108691.c b/gcc/testsuite/gcc.dg/pr108691.c
new file mode 100644
index 00000000000..e412df10f22
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr108691.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+extern int __attribute__((returns_twice)) setjmp(void*);
+
+void bbb(void) {
+  int (*fnptr)(void*) = setjmp;
+  fnptr(0);
+}
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index a9fcc7fd050..e23293e5cd1 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -2280,7 +2280,9 @@ notice_special_calls (gcall *call)
 
   if (flags & ECF_MAY_BE_ALLOCA)
     cfun->calls_alloca = true;
-  if (flags & ECF_RETURNS_TWICE)
+  if (flags & ECF_RETURNS_TWICE
+      && (!(cfun->curr_properties & PROP_cfg)
+	  || gimple_call_ctrl_altering_p (call)))
     cfun->calls_setjmp = true;
 }
 
-- 
2.35.3


From 01375d8b209c91ea3d5b9e6f6ee6fa3e814c4142 Mon Sep 17 00:00:00 2001
From: Richard Biener <rguenther@suse.de>
Date: Mon, 13 Feb 2023 14:14:02 +0100
Subject: [PATCH 2/2] TLC to notice_special_calls
To: gcc-patches@gcc.gnu.org

The following adds a struct function argument to notice_special_calls,
removes the use from the inliner and make sure to not switch cfun
for each stmt outlined to another function.

	* tree-cfg.h (notice_speical_calls): Add struct function
	argument.
	* tree-cfg.cc (notice_special_calls): Likewise.
	(move_block_to_fn): Do not switch cfun around update_stmt and
	notice_special_calls.
	* gimplify.cc (gimplify_call_expr): Adjust.
	(gimplify_modify_expr): Likewise.
	* tree-ssa-dce.cc (eliminate_unnecessary_stmts): Likewise.
---
 gcc/gimplify.cc     |  4 ++--
 gcc/tree-cfg.cc     | 14 ++++++--------
 gcc/tree-cfg.h      |  2 +-
 gcc/tree-inline.cc  |  5 +++--
 gcc/tree-ssa-dce.cc |  2 +-
 5 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 1b362dd83e3..254ddc3c211 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -3864,7 +3864,7 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool want_value)
 	 have to do is replicate it as a GIMPLE_CALL tuple.  */
       gimple_stmt_iterator gsi;
       call = gimple_build_call_from_tree (*expr_p, fnptrtype);
-      notice_special_calls (call);
+      notice_special_calls (cfun, call);
       gimplify_seq_add_stmt (pre_p, call);
       gsi = gsi_last (*pre_p);
       maybe_fold_stmt (&gsi);
@@ -6298,7 +6298,7 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	      call_stmt = gimple_build_call_from_tree (*from_p, fnptrtype);
 	    }
 	}
-      notice_special_calls (call_stmt);
+      notice_special_calls (cfun, call_stmt);
       if (!gimple_call_noreturn_p (call_stmt) || !should_remove_lhs_p (*to_p))
 	gimple_call_set_lhs (call_stmt, *to_p);
       else if (TREE_CODE (*to_p) == SSA_NAME)
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index e23293e5cd1..1dffcc6a693 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -2274,16 +2274,16 @@ single_noncomplex_succ (basic_block bb)
 /* T is CALL_EXPR.  Set current_function_calls_* flags.  */
 
 void
-notice_special_calls (gcall *call)
+notice_special_calls (function *fn, gcall *call)
 {
   int flags = gimple_call_flags (call);
 
   if (flags & ECF_MAY_BE_ALLOCA)
-    cfun->calls_alloca = true;
+    fn->calls_alloca = true;
   if (flags & ECF_RETURNS_TWICE
-      && (!(cfun->curr_properties & PROP_cfg)
+      && (!(fn->curr_properties & PROP_cfg)
 	  || gimple_call_ctrl_altering_p (call)))
-    cfun->calls_setjmp = true;
+    fn->calls_setjmp = true;
 }
 
 
@@ -7446,11 +7446,9 @@ move_block_to_fn (struct function *dest_cfun, basic_block bb,
       /* We cannot leave any operands allocated from the operand caches of
 	 the current function.  */
       free_stmt_operands (cfun, stmt);
-      push_cfun (dest_cfun);
-      update_stmt (stmt);
+      update_stmt_fn (dest_cfun, stmt);
       if (is_gimple_call (stmt))
-	notice_special_calls (as_a <gcall *> (stmt));
-      pop_cfun ();
+	notice_special_calls (dest_cfun, as_a <gcall *> (stmt));
     }
 
   FOR_EACH_EDGE (e, ei, bb->succs)
diff --git a/gcc/tree-cfg.h b/gcc/tree-cfg.h
index 9b56a68fe9d..f845fcb9699 100644
--- a/gcc/tree-cfg.h
+++ b/gcc/tree-cfg.h
@@ -40,7 +40,7 @@ extern bool group_case_labels_stmt (gswitch *);
 extern bool group_case_labels (void);
 extern void replace_uses_by (tree, tree);
 extern basic_block single_noncomplex_succ (basic_block bb);
-extern void notice_special_calls (gcall *);
+extern void notice_special_calls (struct function *, gcall *);
 extern void clear_special_calls (void);
 extern edge find_taken_edge (basic_block, tree);
 extern void gimple_debug_bb (basic_block);
diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc
index 7fd08310acf..af5b99fbaa3 100644
--- a/gcc/tree-inline.cc
+++ b/gcc/tree-inline.cc
@@ -2353,8 +2353,6 @@ copy_bb (copy_body_data *id, basic_block bb,
 			       dest->dump_name ());
 		    }
 		}
-
-	      notice_special_calls (as_a <gcall *> (stmt));
 	    }
 
 	  maybe_duplicate_eh_stmt_fn (cfun, stmt, id->src_cfun, orig_stmt,
@@ -3076,6 +3074,9 @@ copy_cfg_body (copy_body_data * id,
 	new_bb->loop_father = entry_block_map->loop_father;
       }
 
+  cfun->calls_alloca |= src_cfun->calls_alloca;
+  cfun->calls_setjmp |= src_cfun->calls_setjmp;
+
   last = last_basic_block_for_fn (cfun);
 
   /* Now that we've duplicated the blocks, duplicate their edges.  */
diff --git a/gcc/tree-ssa-dce.cc b/gcc/tree-ssa-dce.cc
index ceeb0ad5ab3..e9404c55bf9 100644
--- a/gcc/tree-ssa-dce.cc
+++ b/gcc/tree-ssa-dce.cc
@@ -1420,7 +1420,7 @@ eliminate_unnecessary_stmts (bool aggressive)
 	    {
 	      tree name = gimple_call_lhs (stmt);
 
-	      notice_special_calls (as_a <gcall *> (stmt));
+	      notice_special_calls (cfun, as_a <gcall *> (stmt));
 
 	      /* When LHS of var = call (); is dead, simplify it into
 		 call (); saving one operand.  */
-- 
2.35.3