On Tue, 12 Mar 2024, Jakub Jelinek wrote:
> On Tue, Mar 12, 2024 at 11:42:03AM +0100, Richard Biener wrote:
> > > +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;
> > > + /* Look for a fallthru and possibly a single backedge. */
> >
> > I can't see the backedge handling (I'm not sure it's actually required?)
>
> Oops, that comment line just shouldn't be there. Grepping around, I've
> pasted it from tree-ssa-uninit.cc (warn_uninit_phi_uses) - just wanted
> to grab something with the edge_iterator, edge declaration and
> FOR_EACH_EDGE on bb->preds) and forgot to remove that comment line.
>
> Consider it removed.
>
> > > + FOR_EACH_EDGE (e, ei, bb->preds)
> > > + {
> > > + if ((e->flags & (EDGE_ABNORMAL | EDGE_EH)) == EDGE_ABNORMAL)
> >
> > EDGE_EH is special because of the landing pad?
>
> Yes, though I'd think a landing pad would need to start with a label.
> Anyway, the (x & (EDGE_ABNORMAL | EDGE_EH)) == EDGE_ABNORMAL
> test is what is tested for ABNORMAL_DISPATCHER in other places:
> tree-cfgcleanup.cc- if ((ei_edge (ei)->flags
> tree-cfgcleanup.cc: & (EDGE_ABNORMAL | EDGE_EH)) == EDGE_ABNORMAL)
> tree-cfgcleanup.cc- {
> tree-cfgcleanup.cc- gimple_stmt_iterator gsi
> tree-cfgcleanup.cc- = gsi_start_nondebug_after_labels_bb (dest);
> tree-cfgcleanup.cc- gimple *g = gsi_stmt (gsi);
> tree-cfgcleanup.cc- if (g && gimple_call_internal_p (g,
> IFN_ABNORMAL_DISPATCHER))
> tree-cfgcleanup.cc- abnormal_dispatchers.safe_push (dest);
> (that is where I copied it from),
> tree-cfg.cc- FOR_EACH_EDGE (e, ei, bb->succs)
> tree-cfg.cc: if ((e->flags & (EDGE_ABNORMAL | EDGE_EH)) == EDGE_ABNORMAL)
> tree-cfg.cc- {
> tree-cfg.cc- gimple_stmt_iterator gsi
> tree-cfg.cc- = gsi_start_nondebug_after_labels_bb (e->dest);
> tree-cfg.cc- gimple *g = gsi_stmt (gsi);
> tree-cfg.cc- if (g && gimple_call_internal_p (g, IFN_ABNORMAL_DISPATCHER))
> tree-cfg.cc- return e->dest;
> Though, admittedly, git blame shows both were written by myself, so it
> doesn't count much. The edge from IFN_ABNORMAL_DISPATCHER has EDGE_ABNORMAL
> flag set:
> make_edge (*dispatcher, for_bb, EDGE_ABNORMAL);
> and I bet some EDGE_* flags can be set on that edge later on, but EDGE_EH
> certainly shouldn't be.
> Bet in both of those preexisting cases and in this new one it could be done
> just as flags & EDGE_ABNORMAL test, though perhaps the tree-cfgcleanup.cc
> and tree-cfg.cc cases can see there EDGE_EH edges and would query the first
> non-debug stmt in them uselessly, while perhaps among the returns_twice
> predecessor EDGE_EH is very unlikely and not worth testing.
> So, if you want, I can change that line to
> if ((e->flags & EDGE_ABNORMAL) != 0)
>
> >
> > > + {
> > > + 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);
> >
> > In theory there could be multiple abnormal edges.
>
> Sure, but a function has at most one .ABNORMAL_DISPATCHER block.
> Edge from that block is the only one that needs to be kept pointing to
> the returns_twice function. In the testcases I've even tried to
> get other abnormal edges there, but because of labels we actually have
> labels on the block that falls through to the returns_twice block.
> I bet for EH edges it is the same.
>
> > Did you try
> > to use make_forwarder_block instead of manually doing it?
>
> Looking at it, it doesn't a lot of unnecessary work. We don't need
> to do any redirection of anything but moving the .ABNORMAL_DISPATCHER
> edge after splitting the block. And indeed, it would fail on redirection
> of the EDGE_ABNORMAL edge.
I'm mostly concerned about maintainability here, but let's leave that
aside because of the edge redirection that doesn't work (I don't
see why we couldn't just make it work, but ...)
> > Or
> > does that fail because we end up redirecting the abnormal edges
> > which you avoid by re-making it new and scrapping the old one?
> > I fail to remember why we can't redirect abnormal edges - yes,
> > there's no flow to be redirected but the process of "redirection"
> > still should work.
>
> Seems the redirection always wants to adjust the src blocks of the
> edges. That isn't something that should be done for this particular
> abnormal edge, that one actually can be redirected just by replacing it,
> though for say EH edges we'd need to adjust the on the side EH info etc.
>
> > > + 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;
> > > + if (virtual_operand_p (lhs))
> > > + new_lhs = make_ssa_name (SSA_NAME_VAR (lhs));
> > > + else
> > > + new_lhs = make_ssa_name (TREE_TYPE (lhs));
>
> Guess I should use new_lhs = copy_ssa_name (lhs); instead
>
> > > + 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;
> > > +}
>
> > > + 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;
> > > + }
> > > + }
> >
> > Ick. Doesn't the forwarder keep the PHI defs and new PHI defs
> > are created in e->dest instead, I don't see how we need this?
>
> Admittedly the above is the ugliest part of the patch IMHO.
> It isn't needed in all cases, but e.g. for the pr112709-2.c (qux) case
> we have before ubsan pass
> # _7(ab) = PHI <_20(9), _8(ab)(11)>
> _22 = bar (*_7(ab));
> and want to instrument the *_7(ab) load among the parameters for object
> size.
> So, it wants to add
> _2 = __builtin_dynamic_object_size (_7(ab), 0);
> sequence and later:
> .UBSAN_OBJECT_SIZE (_7(ab), 1024, _2, 0);
> before the call insn. If it isn't a noreturn call, it would just
> add those statements into the same basic block using gsi_insert*before.
> But because it is a returns_twice call, it needs to be done in a different
> block. And having all of ubsan/asan/bitintlower find that out first and
> figure out that it should use _20 instead of _7(ab) seems hard, especially
> that for cases like:
I would have expected the result as
# _7 = PHI <_20(9)>
_2 = __builtin_dynamic_object_size (_7, 0);
.UBSAN_OBJECT_SIZE (_7(ab), 1024, _2, 0);
// fallthru
# _99(ab) = PHI <_7, _8(ab)(11)>
_22 = bar (*_99(ab));
where all uses of _7 in the IL before splitting get replaced via
their immediate uses but the to-be inserted IL can continue using
the defs (that requires the not inserted IL to not have update_stmt
called on them, of course).
I think split_block would have the PHIs organized that way already,
you are adding the _99 defs anyway, right? That is,
+ tree new_lhs = copy_ssa_name (lhs);
+ gimple_phi_set_result (phi, new_lhs);
+ gphi *new_phi = create_phi_node (lhs, other_edge->dest);
here you swap the defs, I'd omit that. And instead essentially do
replace_uses_by (lhs, new_lhs) here (without the folding and stuff).
You have to clear SSA_NAME_OCCURS_IN_ABNORMAL_PHI on the old
and set it on the new LHS if it was set.
I think that should contain the "SSA update" to
edge_before_returns_twice_call for the case it splits the block?
> 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);
> }
> added to pr112709-2.c we'd have:
> # x_5(ab) = PHI <x_3(ab)(7), x_4(ab)(3), x_24(6), x_21(10)>
> # p_8(ab) = PHI <p_16(ab)(7), p_7(ab)(3), p_16(ab)(6), p_16(ab)(10)>
> _26 = bar (*p_8(ab));
> before ubsan and the .ABNORMAL_DISPATCHER block is 3.
> And the caller doesn't know what to replace the p_8(ab) in there with
> until the new bb is created and PHIs are adjusted.
>
> Though, I ran into ICE on the above testcase, not on the second bar call
> with the multiple edges, but on the first one, because unlike everything
> else added in the testcases, the first bar splits the e edge on
> gsi_insert_on_edge_immediate, so the assumption that e is the edge from
> the single non-EDGE_ABNORMAL predecessor of the returns_twice block
> to the returns_twice block is then no longer true. Fixed by adding
> if (new_bb)
> e = single_succ_edge (new_bb);
> twice.
>
> > Btw, gsi_insert_before_returns_twice_call might be usable as
> > gsi_insert_after_labels ()?
>
> It could be called like that too, sure, though still the callers
> would need to check if they want to insert before some statement or
> "after labels" which for the returns_twice case actually isn't after any
> labels, but on an edge and if there isn't one (except for the EDGE_ABNORMAL),
> ensure there is just one.
> In theory even gsi_insert_before and gsi_insert_seq_before could do those,
> but I think most of the half thousand users of those don't need that
> behavior and would be surprised if something like that happened.
>
> > Or alternatively not expose
> > gsi_insert_before_returns_twice_call but instead make
> > split_block () behave "correctly" for abnormals (or add
> > split_block_before_returns_twice_call ()). But that's just bike-shedding.
>
> I don't see how is that possible. The usual case is that no split_block
> is needed, usually the returns_twice block has just a single normal
> predecessor and the .ABNORMAL_EDGE predecessor. And in that case
> we shouldn't be creating further block, just insert on the edge with the
> PHI result to arg replacements. Similarly, even if we do split it, all
> further instrumentation that should go before the call should go to the
> created block, not result in further splits.
>
> Anyway, here is an updated patch, with the bogus comment removed,
> copy_ssa_name used, the ICE fix mentioned above and the additions of freddy
> functions into the tests.
>
> Tested so far 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_insert_before_returns_twice_call,
> gsi_insert_seq_before_returns_twice_call): Declare.
> * gimple-iterator.cc: Include gimplify.h.
> (edge_before_returns_twice_call, gsi_insert_before_returns_twice_call,
> gsi_insert_seq_before_returns_twice_call): New functions.
> * ubsan.cc (ubsan_insert_before, ubsan_insert_seq_before): New
> functions.
> (instrument_mem_ref, instrument_pointer_overflow,
> instrument_nonnull_arg, instrument_nonnull_return): Use
> ubsan_insert_before instead of gsi_insert_before.
> (maybe_instrument_pointer_overflow): Use force_gimple_operand,
> gimple_seq_add_seq_without_update and ubsan_insert_seq_before
> instead of force_gimple_operand_gsi.
> (instrument_object_size): Likewise. Use ubsan_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 12:34:56.026880701 +0100
> @@ -93,6 +93,10 @@ 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 basic_block gsi_insert_before_returns_twice_call (basic_block,
> + gimple *);
> +extern basic_block gsi_insert_seq_before_returns_twice_call (basic_block,
> + 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 12:53:26.174467429 +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,134 @@ gsi_start_phis (basic_block bb)
>
> return i;
> }
> +
> +/* Helper function for gsi_insert_before_returns_twice_call and
> + gsi_insert_seq_before_returns_twice_call. 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;
> +}
> +
> +/* Insert G before BB which starts with an ECF_RETURNS_TWICE call.
> + If BB has a single predecessor edge except for edge from
> + .ABNORMAL_DISPATCHER, insert on that edge, otherwise split
> + BB before the call, adjust PHIs and insert into the new BB.
> + If G refers to any results of BB's PHIs, replace them afterwards
> + with corresponding PHI arg. */
> +
> +basic_block
> +gsi_insert_before_returns_twice_call (basic_block bb, gimple *g)
> +{
> + edge e = edge_before_returns_twice_call (bb);
> + basic_block new_bb = gsi_insert_on_edge_immediate (e, g);
> + if (new_bb)
> + e = single_succ_edge (new_bb);
> + 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);
> + return new_bb;
> +}
> +
> +/* Similiarly for sequence SEQ. */
> +
> +basic_block
> +gsi_insert_seq_before_returns_twice_call (basic_block bb, gimple_seq seq)
> +{
> + if (gimple_seq_empty_p (seq))
> + return NULL;
> + gimple *f = gimple_seq_first_stmt (seq);
> + gimple *l = gimple_seq_last_stmt (seq);
> + edge e = edge_before_returns_twice_call (bb);
> + basic_block new_bb = gsi_insert_seq_on_edge_immediate (e, seq);
> + if (new_bb)
> + e = single_succ_edge (new_bb);
> + use_operand_p use_p;
> + ssa_op_iter iter;
> + for (gimple_stmt_iterator gsi = gsi_for_stmt (f); ; gsi_next (&gsi))
> + {
> + gimple *g = gsi_stmt (gsi);
> + 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);
> + if (g == l)
> + break;
> + }
> + return new_bb;
> +}
> --- gcc/ubsan.cc.jj 2024-03-12 10:15:41.306529121 +0100
> +++ gcc/ubsan.cc 2024-03-12 12:34:56.027880687 +0100
> @@ -1432,6 +1432,37 @@ ubsan_expand_vptr_ifn (gimple_stmt_itera
> return true;
> }
>
> +/* Insert G stmt before ITER. If ITER is a returns_twice call,
> + insert it on an appropriate edge instead. */
> +
> +static void
> +ubsan_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. */
> +
> +static void
> +ubsan_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)
> + gsi_insert_seq_before_returns_twice_call (gsi_bb (*iter), seq);
> + else
> + gsi_insert_seq_before (iter, seq, GSI_SAME_STMT);
> +}
> +
> /* Instrument a memory reference. BASE is the base of MEM, IS_LHS says
> whether the pointer is on the left hand side of the assignment. */
>
> @@ -1458,7 +1489,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);
> + ubsan_insert_before (iter, g);
> }
>
> /* Perform the pointer instrumentation. */
> @@ -1485,7 +1516,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);
> + ubsan_insert_before (gsi, g);
> }
>
> /* Instrument pointer arithmetics if any. */
> @@ -1577,10 +1608,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);
> + ubsan_insert_seq_before (gsi, seq);
> instrument_pointer_overflow (gsi, base_addr, t);
> }
>
> @@ -2035,7 +2067,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);
> + ubsan_insert_before (gsi, g);
> arg = gimple_assign_lhs (g);
> }
>
> @@ -2068,7 +2100,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);
> + ubsan_insert_before (gsi, g);
> ubsan_create_edge (g);
> }
> *gsi = gsi_for_stmt (stmt);
> @@ -2124,7 +2156,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);
> + ubsan_insert_before (gsi, g);
> ubsan_create_edge (g);
> *gsi = gsi_for_stmt (stmt);
> }
> @@ -2231,6 +2263,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 +2277,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 +2324,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);
> + ubsan_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);
> + ubsan_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)
