On Tue, 12 Mar 2024, Jakub Jelinek wrote:
> On Tue, Mar 12, 2024 at 02:31:28PM +0100, Richard Biener wrote:
> > Ah, yeah, I see :/
> >
> > > So, the intention of edge_before_returns_twice_call is just that
> > > it in the common case just finds the non-EDGE_ABNORMAL edge if there is
> > > one,
> > > if there isn't just one, it adjusts the IL such that there is just one.
> > > And then the next step is to handle that case.
> >
> > So I guess the updated patch is OK then.
>
> For the naming thing, another variant would be to export
>
> void
> gsi_safe_insert_before (gimple_stmt_iterator *iter, gimple *g)
> {
> gimple *stmt = gsi_stmt (*iter);
> if (stmt
> && is_gimple_call (stmt)
> && (gimple_call_flags (stmt) & ECF_RETURNS_TWICE) != 0)
> gsi_insert_before_returns_twice_call (gsi_bb (*iter), g);
> else
> gsi_insert_before (iter, g, GSI_SAME_STMT);
> }
>
> /* Similarly for sequence SEQ. */
>
> void
> gsi_safe_insert_seq_before (gimple_stmt_iterator *iter, gimple_seq seq)
> ...
>
> and inline the gsi_insert_*before_returns_twice_call calls by hand
> in there.
> Then asan.cc/ubsan.cc wouldn't need to define those functions.
> I could even outline the updating of SSA_NAMEs on a single statement
> into a helper inline function.
>
> The patch is even shorter then (the asan patch as well).
>
> Tested again with make check-gcc check-g++ RUNTESTFLAGS='ubsan.exp asan.exp'
I like this more, thus OK.
Richard.
> 2024-03-12 Jakub Jelinek <ja...@redhat.com>
>
> PR sanitizer/112709
> * gimple-iterator.h (gsi_safe_insert_before,
> gsi_safe_insert_seq_before): Declare.
> * gimple-iterator.cc: Include gimplify.h.
> (edge_before_returns_twice_call, adjust_before_returns_twice_call,
> gsi_safe_insert_before, gsi_safe_insert_seq_before): New functions.
> * ubsan.cc (instrument_mem_ref, instrument_pointer_overflow,
> instrument_nonnull_arg, instrument_nonnull_return): Use
> gsi_safe_insert_before instead of gsi_insert_before.
> (maybe_instrument_pointer_overflow): Use force_gimple_operand,
> gimple_seq_add_seq_without_update and gsi_safe_insert_seq_before
> instead of force_gimple_operand_gsi.
> (instrument_object_size): Likewise. Use gsi_safe_insert_before
> instead of gsi_insert_before.
>
> * gcc.dg/ubsan/pr112709-1.c: New test.
> * gcc.dg/ubsan/pr112709-2.c: New test.
>
> --- gcc/gimple-iterator.h.jj 2024-03-12 10:15:41.253529859 +0100
> +++ gcc/gimple-iterator.h 2024-03-12 15:10:23.594845422 +0100
> @@ -93,6 +93,8 @@ extern void gsi_insert_on_edge (edge, gi
> extern void gsi_insert_seq_on_edge (edge, gimple_seq);
> extern basic_block gsi_insert_on_edge_immediate (edge, gimple *);
> extern basic_block gsi_insert_seq_on_edge_immediate (edge, gimple_seq);
> +extern void gsi_safe_insert_before (gimple_stmt_iterator *, gimple *);
> +extern void gsi_safe_insert_seq_before (gimple_stmt_iterator *, gimple_seq);
> extern void gsi_commit_edge_inserts (void);
> extern void gsi_commit_one_edge_insert (edge, basic_block *);
> extern gphi_iterator gsi_start_phis (basic_block);
> --- gcc/gimple-iterator.cc.jj 2024-03-12 10:15:41.209530471 +0100
> +++ gcc/gimple-iterator.cc 2024-03-12 15:29:17.814171376 +0100
> @@ -32,6 +32,7 @@ along with GCC; see the file COPYING3.
> #include "tree-cfg.h"
> #include "tree-ssa.h"
> #include "value-prof.h"
> +#include "gimplify.h"
>
>
> /* Mark the statement STMT as modified, and update it. */
> @@ -944,3 +945,137 @@ gsi_start_phis (basic_block bb)
>
> return i;
> }
> +
> +/* Helper function for gsi_safe_insert_before and gsi_safe_insert_seq_before.
> + Find edge to insert statements before returns_twice call at the start of
> BB,
> + if there isn't just one, split the bb and adjust PHIs to ensure that. */
> +
> +static edge
> +edge_before_returns_twice_call (basic_block bb)
> +{
> + gimple_stmt_iterator gsi = gsi_start_nondebug_bb (bb);
> + gcc_checking_assert (is_gimple_call (gsi_stmt (gsi))
> + && (gimple_call_flags (gsi_stmt (gsi))
> + & ECF_RETURNS_TWICE) != 0);
> + edge_iterator ei;
> + edge e, ad_edge = NULL, other_edge = NULL;
> + bool split = false;
> + FOR_EACH_EDGE (e, ei, bb->preds)
> + {
> + if ((e->flags & (EDGE_ABNORMAL | EDGE_EH)) == EDGE_ABNORMAL)
> + {
> + gimple_stmt_iterator gsi
> + = gsi_start_nondebug_after_labels_bb (e->src);
> + gimple *ad = gsi_stmt (gsi);
> + if (ad && gimple_call_internal_p (ad, IFN_ABNORMAL_DISPATCHER))
> + {
> + gcc_checking_assert (ad_edge == NULL);
> + ad_edge = e;
> + continue;
> + }
> + }
> + if (other_edge || e->flags & (EDGE_ABNORMAL | EDGE_EH))
> + split = true;
> + other_edge = e;
> + }
> + gcc_checking_assert (ad_edge);
> + if (other_edge == NULL)
> + split = true;
> + if (split)
> + {
> + other_edge = split_block_after_labels (bb);
> + e = make_edge (ad_edge->src, other_edge->dest, EDGE_ABNORMAL);
> + for (gphi_iterator gsi = gsi_start_phis (other_edge->src);
> + !gsi_end_p (gsi); gsi_next (&gsi))
> + {
> + gphi *phi = gsi.phi ();
> + tree lhs = gimple_phi_result (phi);
> + tree new_lhs = copy_ssa_name (lhs);
> + gimple_phi_set_result (phi, new_lhs);
> + gphi *new_phi = create_phi_node (lhs, other_edge->dest);
> + add_phi_arg (new_phi, new_lhs, other_edge, UNKNOWN_LOCATION);
> + add_phi_arg (new_phi, gimple_phi_arg_def_from_edge (phi, ad_edge),
> + e, gimple_phi_arg_location_from_edge (phi, ad_edge));
> + }
> + remove_edge (ad_edge);
> + }
> + return other_edge;
> +}
> +
> +/* Helper function for gsi_safe_insert_before and gsi_safe_insert_seq_before.
> + Replace SSA_NAME uses in G if they are PHI results of PHIs on E->dest
> + bb with the corresponding PHI argument from E edge. */
> +
> +static void
> +adjust_before_returns_twice_call (edge e, gimple *g)
> +{
> + use_operand_p use_p;
> + ssa_op_iter iter;
> + bool m = false;
> + FOR_EACH_SSA_USE_OPERAND (use_p, g, iter, SSA_OP_USE)
> + {
> + tree s = USE_FROM_PTR (use_p);
> + if (SSA_NAME_DEF_STMT (s)
> + && gimple_code (SSA_NAME_DEF_STMT (s)) == GIMPLE_PHI
> + && gimple_bb (SSA_NAME_DEF_STMT (s)) == e->dest)
> + {
> + tree r = gimple_phi_arg_def_from_edge (SSA_NAME_DEF_STMT (s), e);
> + SET_USE (use_p, unshare_expr (r));
> + m = true;
> + }
> + }
> + if (m)
> + update_stmt (g);
> +}
> +
> +/* Insert G stmt before ITER and keep ITER pointing to the same statement
> + as before. If ITER is a returns_twice call, insert it on an appropriate
> + edge instead. */
> +
> +void
> +gsi_safe_insert_before (gimple_stmt_iterator *iter, gimple *g)
> +{
> + gimple *stmt = gsi_stmt (*iter);
> + if (stmt
> + && is_gimple_call (stmt)
> + && (gimple_call_flags (stmt) & ECF_RETURNS_TWICE) != 0)
> + {
> + edge e = edge_before_returns_twice_call (gsi_bb (*iter));
> + basic_block new_bb = gsi_insert_on_edge_immediate (e, g);
> + if (new_bb)
> + e = single_succ_edge (new_bb);
> + adjust_before_returns_twice_call (e, g);
> + }
> + else
> + gsi_insert_before (iter, g, GSI_SAME_STMT);
> +}
> +
> +/* Similarly for sequence SEQ. */
> +
> +void
> +gsi_safe_insert_seq_before (gimple_stmt_iterator *iter, gimple_seq seq)
> +{
> + if (gimple_seq_empty_p (seq))
> + return;
> + gimple *stmt = gsi_stmt (*iter);
> + if (stmt
> + && is_gimple_call (stmt)
> + && (gimple_call_flags (stmt) & ECF_RETURNS_TWICE) != 0)
> + {
> + edge e = edge_before_returns_twice_call (gsi_bb (*iter));
> + gimple *f = gimple_seq_first_stmt (seq);
> + gimple *l = gimple_seq_last_stmt (seq);
> + basic_block new_bb = gsi_insert_seq_on_edge_immediate (e, seq);
> + if (new_bb)
> + e = single_succ_edge (new_bb);
> + for (gimple_stmt_iterator gsi = gsi_for_stmt (f); ; gsi_next (&gsi))
> + {
> + gimple *g = gsi_stmt (gsi);
> + adjust_before_returns_twice_call (e, g);
> + if (g == l)
> + break;
> + }
> + }
> + else
> + gsi_insert_seq_before (iter, seq, GSI_SAME_STMT);
> +}
> --- gcc/ubsan.cc.jj 2024-03-12 10:15:41.306529121 +0100
> +++ gcc/ubsan.cc 2024-03-12 15:08:17.073594937 +0100
> @@ -1458,7 +1458,7 @@ instrument_mem_ref (tree mem, tree base,
> tree alignt = build_int_cst (pointer_sized_int_node, align);
> gcall *g = gimple_build_call_internal (IFN_UBSAN_NULL, 3, t, kind, alignt);
> gimple_set_location (g, gimple_location (gsi_stmt (*iter)));
> - gsi_insert_before (iter, g, GSI_SAME_STMT);
> + gsi_safe_insert_before (iter, g);
> }
>
> /* Perform the pointer instrumentation. */
> @@ -1485,7 +1485,7 @@ instrument_pointer_overflow (gimple_stmt
> return;
> gcall *g = gimple_build_call_internal (IFN_UBSAN_PTR, 2, ptr, off);
> gimple_set_location (g, gimple_location (gsi_stmt (*gsi)));
> - gsi_insert_before (gsi, g, GSI_SAME_STMT);
> + gsi_safe_insert_before (gsi, g);
> }
>
> /* Instrument pointer arithmetics if any. */
> @@ -1577,10 +1577,11 @@ maybe_instrument_pointer_overflow (gimpl
> else
> t = fold_convert (sizetype, moff);
> }
> - t = force_gimple_operand_gsi (gsi, t, true, NULL_TREE, true,
> - GSI_SAME_STMT);
> - base_addr = force_gimple_operand_gsi (gsi, base_addr, true, NULL_TREE,
> true,
> - GSI_SAME_STMT);
> + gimple_seq seq, this_seq;
> + t = force_gimple_operand (t, &seq, true, NULL_TREE);
> + base_addr = force_gimple_operand (base_addr, &this_seq, true, NULL_TREE);
> + gimple_seq_add_seq_without_update (&seq, this_seq);
> + gsi_safe_insert_seq_before (gsi, seq);
> instrument_pointer_overflow (gsi, base_addr, t);
> }
>
> @@ -2035,7 +2036,7 @@ instrument_nonnull_arg (gimple_stmt_iter
> {
> g = gimple_build_assign (make_ssa_name (TREE_TYPE (arg)), arg);
> gimple_set_location (g, loc[0]);
> - gsi_insert_before (gsi, g, GSI_SAME_STMT);
> + gsi_safe_insert_before (gsi, g);
> arg = gimple_assign_lhs (g);
> }
>
> @@ -2068,7 +2069,7 @@ instrument_nonnull_arg (gimple_stmt_iter
> g = gimple_build_call (fn, 1, data);
> }
> gimple_set_location (g, loc[0]);
> - gsi_insert_before (gsi, g, GSI_SAME_STMT);
> + gsi_safe_insert_before (gsi, g);
> ubsan_create_edge (g);
> }
> *gsi = gsi_for_stmt (stmt);
> @@ -2124,7 +2125,7 @@ instrument_nonnull_return (gimple_stmt_i
> g = gimple_build_call (fn, 2, data, data2);
> }
> gimple_set_location (g, loc[0]);
> - gsi_insert_before (gsi, g, GSI_SAME_STMT);
> + gsi_safe_insert_before (gsi, g);
> ubsan_create_edge (g);
> *gsi = gsi_for_stmt (stmt);
> }
> @@ -2231,6 +2232,7 @@ instrument_object_size (gimple_stmt_iter
> tree sizet;
> tree base_addr = base;
> gimple *bos_stmt = NULL;
> + gimple_seq seq = NULL;
> if (decl_p)
> base_addr = build1 (ADDR_EXPR,
> build_pointer_type (TREE_TYPE (base)), base);
> @@ -2244,19 +2246,12 @@ instrument_object_size (gimple_stmt_iter
> sizet = builtin_decl_explicit (BUILT_IN_DYNAMIC_OBJECT_SIZE);
> sizet = build_call_expr_loc (loc, sizet, 2, base_addr,
> integer_zero_node);
> - sizet = force_gimple_operand_gsi (gsi, sizet, false, NULL_TREE, true,
> - GSI_SAME_STMT);
> + sizet = force_gimple_operand (sizet, &seq, false, NULL_TREE);
> /* If the call above didn't end up being an integer constant, go one
> statement back and get the __builtin_object_size stmt. Save it,
> we might need it later. */
> if (SSA_VAR_P (sizet))
> - {
> - gsi_prev (gsi);
> - bos_stmt = gsi_stmt (*gsi);
> -
> - /* Move on to where we were. */
> - gsi_next (gsi);
> - }
> + bos_stmt = gsi_stmt (gsi_last (seq));
> }
> else
> return;
> @@ -2298,21 +2293,24 @@ instrument_object_size (gimple_stmt_iter
> && !TREE_ADDRESSABLE (base))
> mark_addressable (base);
>
> + /* We have to emit the check. */
> + gimple_seq this_seq;
> + t = force_gimple_operand (t, &this_seq, true, NULL_TREE);
> + gimple_seq_add_seq_without_update (&seq, this_seq);
> + ptr = force_gimple_operand (ptr, &this_seq, true, NULL_TREE);
> + gimple_seq_add_seq_without_update (&seq, this_seq);
> + gsi_safe_insert_seq_before (gsi, seq);
> +
> if (bos_stmt
> && gimple_call_builtin_p (bos_stmt, BUILT_IN_DYNAMIC_OBJECT_SIZE))
> ubsan_create_edge (bos_stmt);
>
> - /* We have to emit the check. */
> - t = force_gimple_operand_gsi (gsi, t, true, NULL_TREE, true,
> - GSI_SAME_STMT);
> - ptr = force_gimple_operand_gsi (gsi, ptr, true, NULL_TREE, true,
> - GSI_SAME_STMT);
> tree ckind = build_int_cst (unsigned_char_type_node,
> is_lhs ? UBSAN_STORE_OF : UBSAN_LOAD_OF);
> gimple *g = gimple_build_call_internal (IFN_UBSAN_OBJECT_SIZE, 4,
> ptr, t, sizet, ckind);
> gimple_set_location (g, loc);
> - gsi_insert_before (gsi, g, GSI_SAME_STMT);
> + gsi_safe_insert_before (gsi, g);
> }
>
> /* Instrument values passed to builtin functions. */
> --- gcc/testsuite/gcc.dg/ubsan/pr112709-1.c.jj 2024-03-12
> 12:34:56.027880687 +0100
> +++ gcc/testsuite/gcc.dg/ubsan/pr112709-1.c 2024-03-12 12:57:58.993687023
> +0100
> @@ -0,0 +1,64 @@
> +/* PR sanitizer/112709 */
> +/* { dg-do compile } */
> +/* { dg-options "-fsanitize=undefined -O2" } */
> +
> +struct S { char c[1024]; };
> +int foo (int);
> +
> +__attribute__((returns_twice, noipa)) struct S
> +bar (int x)
> +{
> + (void) x;
> + struct S s = {};
> + s.c[42] = 42;
> + return s;
> +}
> +
> +void
> +baz (struct S *p)
> +{
> + foo (1);
> + *p = bar (0);
> +}
> +
> +void
> +qux (int x, struct S *p)
> +{
> + if (x == 25)
> + x = foo (2);
> + else if (x == 42)
> + x = foo (foo (3));
> + *p = bar (x);
> +}
> +
> +void
> +corge (int x, struct S *p)
> +{
> + void *q[] = { &&l1, &&l2, &&l3, &&l3 };
> + if (x == 25)
> + {
> + l1:
> + x = foo (2);
> + }
> + else if (x == 42)
> + {
> + l2:
> + x = foo (foo (3));
> + }
> +l3:
> + *p = bar (x);
> + if (x < 4)
> + goto *q[x & 3];
> +}
> +
> +void
> +freddy (int x, struct S *p)
> +{
> + *p = bar (x);
> + ++p;
> + if (x == 25)
> + x = foo (2);
> + else if (x == 42)
> + x = foo (foo (3));
> + *p = bar (x);
> +}
> --- gcc/testsuite/gcc.dg/ubsan/pr112709-2.c.jj 2024-03-12
> 12:34:56.027880687 +0100
> +++ gcc/testsuite/gcc.dg/ubsan/pr112709-2.c 2024-03-12 12:58:13.305488706
> +0100
> @@ -0,0 +1,62 @@
> +/* PR sanitizer/112709 */
> +/* { dg-do compile } */
> +/* { dg-options "-fsanitize=undefined -O2" } */
> +
> +struct S { char c[1024]; } *p;
> +int foo (int);
> +
> +__attribute__((returns_twice, noipa)) int
> +bar (struct S x)
> +{
> + (void) x.c[0];
> + return 0;
> +}
> +
> +void
> +baz (int *y)
> +{
> + foo (1);
> + *y = bar (*p);
> +}
> +
> +void
> +qux (int x, int *y)
> +{
> + if (x == 25)
> + x = foo (2);
> + else if (x == 42)
> + x = foo (foo (3));
> + *y = bar (*p);
> +}
> +
> +void
> +corge (int x, int *y)
> +{
> + void *q[] = { &&l1, &&l2, &&l3, &&l3 };
> + if (x == 25)
> + {
> + l1:
> + x = foo (2);
> + }
> + else if (x == 42)
> + {
> + l2:
> + x = foo (foo (3));
> + }
> +l3:
> + *y = bar (*p);
> + if (x < 4)
> + goto *q[x & 3];
> +}
> +
> +void
> +freddy (int x, int *y, struct S *p)
> +{
> + bar (*p);
> + ++p;
> + if (x == 25)
> + x = foo (2);
> + else if (x == 42)
> + x = foo (foo (3));
> + *y = bar (*p);
> +}
>
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
