On Tue, Dec 14, 2021 at 12:10 PM Алексей Нурмухаметов <nurmukhame...@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 > >> <amona...@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; >