Re: [RFC PATCH] tree-ssa-sink: do not sink to in front of setjmp

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Dec 14, 2021 at 12:10 PM Алексей Нурмухаметов
<nurmukhametov@ispras.ru> wrote:
>
> On 13.12.2021 18:20, Alexander Monakov wrote:
> > On Mon, 13 Dec 2021, Richard Biener wrote:
> >
> >> On December 13, 2021 3:25:47 PM GMT+01:00, Alexander Monakov <amonakov@ispras.ru> wrote:
> >>> Greetings!
> >>>
> >>> While testing our patch that reimplements -Wclobbered on GIMPLE we found
> >>> a case where tree-ssa-sink moves a statement to a basic block in front
> >>> of a setjmp call.
> >>>
> >>> I am confident that this is unintended and should be considered invalid
> >>> GIMPLE.
> >> Does CFG validation not catch this? That is, doesn't setjmp force the start of
> >> a new BB?
> > Oh, good point. There's stmt_start_bb_p which returns true for setjmp, but
> > gimple_verify_flow_info doesn't check it. I guess we can try adding that
> > and collect the fallout on bootstrap/regtest.
>
> Bootstrap looks good, but testsuite has some regression (the applied
> patch is below).
>
> The overall number of unexpected failures and unresolved testcases is
> around 100. The diff is in attachment.
> >> I think sinking relies on dominance and post dominance here but post dominance
> >> may be too fragile with the abnormal cycles which are likely not backwards
> >> reachable from exit.
> >>
> >> That said, checking for abnormal preds is OK, I just want to make sure we
> >> detect the invalid CFG - do we?
> > As above, no, otherwise it would have been caught much earlier than ICE'ing
> > our -Wclobbered patch :)
> >
> > Thank you.
> > Alexander
>
> The patch for CFG validation:
>
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index ebbd894ae03..92b08d1d6d8 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -5663,6 +5663,7 @@ gimple_verify_flow_info (void)
>          }
>
>         /* Verify that body of basic block BB is free of control flow.  */
> +      gimple *prev_stmt = NULL;
>         for (; !gsi_end_p (gsi); gsi_next (&gsi))
>          {
>            gimple *stmt = gsi_stmt (gsi);
> @@ -5674,6 +5675,14 @@ gimple_verify_flow_info (void)
>                err = 1;
>              }
>
> +         if (prev_stmt && stmt_starts_bb_p (stmt, prev_stmt))

stmt_starts_bb_p is really a helper used during CFG build, I'd rather
test explicitely for a GIMPLE call with ECF_RETURNS_TWICE, or maybe,
verify that iff a block has abnormal predecessors it starts with such
a call (because IIRC we in some cases elide abnormal edges and then
it's OK to move "down" the calls).  So yes, if a block has abnormal preds
it should start with a ECF_RETURNS_TWICE call, I think we cannot
verify the reverse reliably - abnormal edges cannot easily be re-built
in late stages (it's a bug that we do so during RTL expansion).


> +           {
> +             error ("setjmp in the middle of basic block %d", bb->index);
> +             err = 1;
> +           }
> +         if (!is_gimple_debug (stmt))
> +           prev_stmt = stmt;
> +
>            if (stmt_ends_bb_p (stmt))
>              found_ctrl_stmt = true;
>