Re: [PATCH] gimple-iterator, ubsan, v3: Fix ICE during instrumentation of returns_twice calls [PR112709]
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
>
> 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.cc2024-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);
[PATCH] gimple-iterator, ubsan, v3: Fix ICE during instrumentation of returns_twice calls [PR112709]
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
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.jj2024-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 ())
+ {
+ gphi *phi = gsi.phi ();
+ tree lhs = gimple_phi_result (phi);
+
