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?
One of the edges incoming to a setjmp BB will be an edge from
>the ABNORMAL_DISPATCHER block, corresponding to transfer of control flow
>from a longjmp-like function and resulting in a "second return" from
>setjmp. When that happens, it is not possible for GIMPLE statements in
>front of setjmp to be somehow re-executed after longjmp.
>
>I am unsure how this could be prevented "once and for all" so the
>following patch just attacks one place (out of three) in
>tree-ssa-sink by checking 'bb_has_abnormal_pred (sinkbb)'. Alexey
>(Cc'ed) bootstrapped and regtested the patch on trunk.
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?
>The testcase is just
>
>struct __jmp_buf_tag { };
>typedef struct __jmp_buf_tag jmp_buf[1];
>struct globals { jmp_buf listingbuf; };
>extern struct globals *const ptr_to_globals;
>void foo()
>{
> if ( _setjmp ( ((*ptr_to_globals).listingbuf )))
> ;
>}
>
>Before tree-ssa-sink we have
>
>void foo ()
>{
> struct globals * ptr_to_globals.0_1;
> struct __jmp_buf_tag[1] * _2;
>
> <bb 2> :
> ptr_to_globals.0_1 = ptr_to_globals;
> _2 = &ptr_to_globals.0_1->listingbuf;
>
> <bb 3> :
> _setjmp (_2);
> goto <bb 5>; [INV]
>
> <bb 4> :
> .ABNORMAL_DISPATCHER (0);
>
> <bb 5> :
> return;
>
>}
>
>And tree-ssa-sink yields (see BB 3)
>
>Sinking _2 = &ptr_to_globals.0_1->listingbuf;
> from bb 2 to bb 3
>void foo ()
>{
> struct globals * ptr_to_globals.0_1;
> struct __jmp_buf_tag[1] * _2;
>
> <bb 2> :
> ptr_to_globals.0_1 = ptr_to_globals;
>
> <bb 3> :
> _2 = &ptr_to_globals.0_1->listingbuf;
> _setjmp (_2);
> goto <bb 5>; [INV]
>
> <bb 4> :
> .ABNORMAL_DISPATCHER (0);
>
> <bb 5> :
> return;
>
>}
>
>The patch:
>
>diff --git a/gcc/tree-ssa-sink.c b/gcc/tree-ssa-sink.c
>index c5d67840be3..317e2563114 100644
>--- a/gcc/tree-ssa-sink.c
>+++ b/gcc/tree-ssa-sink.c
>@@ -461,6 +461,9 @@ statement_sink_location (gimple *stmt, basic_block frombb,
> else
> *togsi = gsi_after_labels (sinkbb);
>
>+ if (bb_has_abnormal_pred (sinkbb))
>+ return false;
>+
> return true;
> }
> }
