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'
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
