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

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

[Alexander Monakov]

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. 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.

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;
 	}
     }

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

[Richard Biener]

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?

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;
> 	}
>     }

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

[Alexander Monakov]

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.

> 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

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

[Алексей Нурмухаметов]

[-- Attachment #1: Type: text/plain, Size: 2413 bytes --]

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))
+           {
+             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;


[-- Attachment #2: test_summary.diff --]
[-- Type: text/x-patch, Size: 10068 bytes --]

7a8,9
> FAIL: gcc.c-torture/compile/930111-1.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (internal compiler error)
> FAIL: gcc.c-torture/compile/930111-1.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
85a88,148
> FAIL: c-c++-common/tm/cancel-1.c (internal compiler error)
> FAIL: c-c++-common/tm/cancel-1.c (test for excess errors)
> FAIL: gcc.dg/tm/data-2.c (internal compiler error)
> FAIL: gcc.dg/tm/data-2.c (test for excess errors)
> FAIL: gcc.dg/tm/debug-1.c (internal compiler error)
> FAIL: gcc.dg/tm/debug-1.c (test for excess errors)
> FAIL: gcc.dg/tm/memopt-10.c (internal compiler error)
> FAIL: gcc.dg/tm/memopt-10.c (test for excess errors)
> FAIL: gcc.dg/tm/memopt-11.c (internal compiler error)
> FAIL: gcc.dg/tm/memopt-11.c (test for excess errors)
> FAIL: gcc.dg/tm/memopt-12.c (internal compiler error)
> FAIL: gcc.dg/tm/memopt-12.c (test for excess errors)
> FAIL: gcc.dg/tm/memopt-15.c (internal compiler error)
> FAIL: gcc.dg/tm/memopt-15.c (test for excess errors)
> UNRESOLVED: gcc.dg/tm/memopt-15.c scan-assembler _ITM_LM128
> FAIL: gcc.dg/tm/memopt-16.c (internal compiler error)
> FAIL: gcc.dg/tm/memopt-16.c (test for excess errors)
> FAIL: gcc.dg/tm/memopt-3.c (internal compiler error)
> FAIL: gcc.dg/tm/memopt-3.c (test for excess errors)
> FAIL: gcc.dg/tm/memopt-4.c (internal compiler error)
> FAIL: gcc.dg/tm/memopt-4.c (test for excess errors)
> UNRESOLVED: gcc.dg/tm/memopt-4.c scan-tree-dump-times tmedge "tm_save.[0-9_]+ = lala.x\\\\[55\\\\]" 1
> UNRESOLVED: gcc.dg/tm/memopt-4.c scan-tree-dump-times tmedge "lala.x\\\\[55\\\\] = tm_save" 1
> FAIL: gcc.dg/tm/memopt-5.c (internal compiler error)
> FAIL: gcc.dg/tm/memopt-5.c (test for excess errors)
> UNRESOLVED: gcc.dg/tm/memopt-5.c scan-tree-dump-times tmedge "ITM_LU[0-9] \\\\(&lala.x\\\\[55\\\\]" 1
> FAIL: gcc.dg/tm/memopt-6.c (internal compiler error)
> FAIL: gcc.dg/tm/memopt-6.c (test for excess errors)
> UNRESOLVED: gcc.dg/tm/memopt-6.c scan-tree-dump-times tmedge "memcpyRtWn \\\\(.*, &lacopy" 1
> FAIL: gcc.dg/tm/memopt-7.c (internal compiler error)
> FAIL: gcc.dg/tm/memopt-7.c (test for excess errors)
> UNRESOLVED: gcc.dg/tm/memopt-7.c scan-tree-dump-times tmedge "tm_save.[0-9_]+ = lala" 1
> UNRESOLVED: gcc.dg/tm/memopt-7.c scan-tree-dump-times tmedge "lala = tm_save" 1
> FAIL: gcc.dg/tm/opt-1.c (internal compiler error)
> FAIL: gcc.dg/tm/opt-1.c (test for excess errors)
> FAIL: gcc.dg/tm/pr55401.c (internal compiler error)
> FAIL: gcc.dg/tm/pr55401.c (test for excess errors)
> UNRESOLVED: gcc.dg/tm/pr55401.c scan-tree-dump-times optimized "ITM_WU[0-9] \\\\(&george," 2
> FAIL: gcc.dg/tm/pr95569.c (internal compiler error)
> FAIL: gcc.dg/tm/pr95569.c (test for excess errors)
> FAIL: gcc.dg/torture/pr100053.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  (internal compiler error)
> FAIL: gcc.dg/torture/pr100053.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  (test for excess errors)
> UNRESOLVED: gcc.dg/torture/pr100053.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  compilation failed to produce executable
> FAIL: gcc.dg/torture/pr103458.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (internal compiler error)
> FAIL: gcc.dg/torture/pr103458.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
> FAIL: gcc.dg/torture/pr103596.c   -O1  (internal compiler error)
> FAIL: gcc.dg/torture/pr103596.c   -O1  (test for excess errors)
> FAIL: gcc.dg/torture/pr103596.c   -O2  (internal compiler error)
> FAIL: gcc.dg/torture/pr103596.c   -O2  (test for excess errors)
> FAIL: gcc.dg/torture/pr103596.c   -O3 -g  (internal compiler error)
> FAIL: gcc.dg/torture/pr103596.c   -O3 -g  (test for excess errors)
> FAIL: gcc.dg/torture/pr103596.c   -Os  (internal compiler error)
> FAIL: gcc.dg/torture/pr103596.c   -Os  (test for excess errors)
> FAIL: gcc.dg/torture/pr103596.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  (internal compiler error)
> FAIL: gcc.dg/torture/pr103596.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  (test for excess errors)
> FAIL: gcc.dg/torture/pr103596.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  (internal compiler error)
> FAIL: gcc.dg/torture/pr103596.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects  (test for excess errors)
> FAIL: gcc.dg/torture/pr79432.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (internal compiler error)
> FAIL: gcc.dg/torture/pr79432.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
> FAIL: gcc.dg/torture/pr82276.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (internal compiler error)
> FAIL: gcc.dg/torture/pr82276.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
89,90c152,153
< # of expected passes		175813
< # of unexpected failures	64
---
> # of expected passes		175777
> # of unexpected failures	118
92a156
> # of unresolved testcases	9
104,105d167
< FAIL: g++.dg/modules/xtreme-header-3_b.C -std=c++17 (test for excess errors)
< FAIL: g++.dg/modules/xtreme-header-3_c.C -std=c++17 (test for excess errors)
109,110d170
< FAIL: g++.dg/modules/xtreme-header-3_b.C -std=c++2a (test for excess errors)
< FAIL: g++.dg/modules/xtreme-header-3_c.C -std=c++2a (test for excess errors)
113a174,175
> FAIL: g++.dg/modules/xtreme-header-3_b.C -std=c++17 (test for excess errors)
> FAIL: g++.dg/modules/xtreme-header-3_b.C -std=c++2a (test for excess errors)
114a177,178
> FAIL: g++.dg/modules/xtreme-header-3_c.C -std=c++17 (test for excess errors)
> FAIL: g++.dg/modules/xtreme-header-3_c.C -std=c++2a (test for excess errors)
119,120d182
< FAIL: g++.dg/modules/xtreme-header-5_b.C -std=c++2a (test for excess errors)
< FAIL: g++.dg/modules/xtreme-header-5_c.C -std=c++2a (test for excess errors)
123a186
> FAIL: g++.dg/modules/xtreme-header-5_b.C -std=c++2a (test for excess errors)
124a188
> FAIL: g++.dg/modules/xtreme-header-5_c.C -std=c++2a (test for excess errors)
129d192
< FAIL: g++.dg/modules/xtreme-header_b.C -std=c++17 (test for excess errors)
133d195
< FAIL: g++.dg/modules/xtreme-header_b.C -std=c++2a (test for excess errors)
136a199,200
> FAIL: g++.dg/modules/xtreme-header_b.C -std=c++17 (test for excess errors)
> FAIL: g++.dg/modules/xtreme-header_b.C -std=c++2a (test for excess errors)
137a202,233
> FAIL: c-c++-common/tm/cancel-1.c  -std=gnu++98 (internal compiler error)
> FAIL: c-c++-common/tm/cancel-1.c  -std=gnu++98 (test for excess errors)
> FAIL: c-c++-common/tm/cancel-1.c  -std=gnu++14 (internal compiler error)
> FAIL: c-c++-common/tm/cancel-1.c  -std=gnu++14 (test for excess errors)
> FAIL: c-c++-common/tm/cancel-1.c  -std=gnu++17 (internal compiler error)
> FAIL: c-c++-common/tm/cancel-1.c  -std=gnu++17 (test for excess errors)
> FAIL: c-c++-common/tm/cancel-1.c  -std=gnu++2a (internal compiler error)
> FAIL: c-c++-common/tm/cancel-1.c  -std=gnu++2a (test for excess errors)
> FAIL: g++.dg/tm/fatomic-1.C  -std=gnu++98 (internal compiler error)
> FAIL: g++.dg/tm/fatomic-1.C  -std=gnu++98 (test for excess errors)
> FAIL: g++.dg/tm/fatomic-1.C  -std=gnu++14 (internal compiler error)
> FAIL: g++.dg/tm/fatomic-1.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/tm/fatomic-1.C  -std=gnu++17 (internal compiler error)
> FAIL: g++.dg/tm/fatomic-1.C  -std=gnu++17 (test for excess errors)
> FAIL: g++.dg/tm/fatomic-1.C  -std=gnu++2a (internal compiler error)
> FAIL: g++.dg/tm/fatomic-1.C  -std=gnu++2a (test for excess errors)
> FAIL: g++.dg/tm/nested-3.C  -std=gnu++98 (internal compiler error)
> FAIL: g++.dg/tm/nested-3.C  -std=gnu++98 (test for excess errors)
> FAIL: g++.dg/tm/nested-3.C  -std=gnu++14 (internal compiler error)
> FAIL: g++.dg/tm/nested-3.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/tm/nested-3.C  -std=gnu++17 (internal compiler error)
> FAIL: g++.dg/tm/nested-3.C  -std=gnu++17 (test for excess errors)
> FAIL: g++.dg/tm/nested-3.C  -std=gnu++2a (internal compiler error)
> FAIL: g++.dg/tm/nested-3.C  -std=gnu++2a (test for excess errors)
> FAIL: g++.dg/tm/template-1.C  -std=gnu++98 (internal compiler error)
> FAIL: g++.dg/tm/template-1.C  -std=gnu++98 (test for excess errors)
> FAIL: g++.dg/tm/template-1.C  -std=gnu++14 (internal compiler error)
> FAIL: g++.dg/tm/template-1.C  -std=gnu++14 (test for excess errors)
> FAIL: g++.dg/tm/template-1.C  -std=gnu++17 (internal compiler error)
> FAIL: g++.dg/tm/template-1.C  -std=gnu++17 (test for excess errors)
> FAIL: g++.dg/tm/template-1.C  -std=gnu++2a (internal compiler error)
> FAIL: g++.dg/tm/template-1.C  -std=gnu++2a (test for excess errors)
141,142c237,238
< # of expected passes		224294
< # of unexpected failures	38
---
> # of expected passes		224278
> # of unexpected failures	70
168a265,279
> FAIL: libitm.c/cancel.c (internal compiler error)
> FAIL: libitm.c/cancel.c (test for excess errors)
> UNRESOLVED: libitm.c/cancel.c compilation failed to produce executable
> FAIL: libitm.c/dropref-2.c (internal compiler error)
> FAIL: libitm.c/dropref-2.c (test for excess errors)
> UNRESOLVED: libitm.c/dropref-2.c compilation failed to produce executable
> FAIL: libitm.c/priv-1.c (internal compiler error)
> FAIL: libitm.c/priv-1.c (test for excess errors)
> UNRESOLVED: libitm.c/priv-1.c compilation failed to produce executable
> FAIL: libitm.c/reentrant.c (internal compiler error)
> FAIL: libitm.c/reentrant.c (test for excess errors)
> UNRESOLVED: libitm.c/reentrant.c compilation failed to produce executable
> FAIL: libitm.c++/eh-4.C (internal compiler error)
> FAIL: libitm.c++/eh-4.C (test for excess errors)
> UNRESOLVED: libitm.c++/eh-4.C compilation failed to produce executable
172,173c283,286
< # of expected passes		44
< # of expected failures		3
---
> # of expected passes		35
> # of unexpected failures	10
> # of expected failures		2
> # of unresolved testcases	5

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

[Richard Biener]

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;
>

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

[Alexander Monakov]

On Mon, 3 Jan 2022, Richard Biener wrote:

> > @@ -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).

Thanks, I could rewrite the patch along those lines, but I'm not sure where
this is going: the ~100 extra FAILs will still be there. What would the next
steps be for this patch and the initial tree-ssa-sink patch?

Alexander

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

[Richard Biener]

On Mon, Jan 3, 2022 at 5:35 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Mon, 3 Jan 2022, Richard Biener wrote:
>
> > > @@ -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).
>
> Thanks, I could rewrite the patch along those lines, but I'm not sure where
> this is going: the ~100 extra FAILs will still be there. What would the next
> steps be for this patch and the initial tree-ssa-sink patch?

I approved the initial sink patch (maybe not clearly enough).  Can you open
a bugreport about the missing CFG verification and list the set of FAILs
(all errors in some passes similar to the one you fixed in sinking I guess)?
It indeed sounds like something to tackle during next stage1 (unless you
already narrowed down the culprit to a single offender...)

Richard.

>
> Alexander

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

[Alexander Monakov]

> I approved the initial sink patch (maybe not clearly enough).

I wasn't entirely happy with that patch. The new version solves this better.

> Can you open
> a bugreport about the missing CFG verification and list the set of FAILs
> (all errors in some passes similar to the one you fixed in sinking I guess)?
> It indeed sounds like something to tackle during next stage1 (unless you
> already narrowed down the culprit to a single offender...)

Most of the failures were related to transactional memory, and the rest are
seemingly solved by forbidding duplication of returns_twice calls. In reply
to this email I'm sending three patches, the first is a revised patch for
tree-ssa-sink, the second forbids duplication of setjmp-like calls, and the
third implements the checks in verify_flow_info:

  tree-ssa-sink: do not sink to in front of setjmp
  tree-cfg: do not duplicate returns_twice calls
  tree-cfg: check placement of returns_twice calls

 gcc/testsuite/gcc.dg/setjmp-7.c | 13 +++++++++++
 gcc/tree-cfg.c                  | 40 +++++++++++++++++++++++++++++++--
 gcc/tree-ssa-sink.c             |  6 +++++
 3 files changed, 57 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/setjmp-7.c

-- 
2.33.1

[PATCH 1/3] tree-ssa-sink: do not sink to in front of setjmp

[Alexander Monakov]

gcc/ChangeLog:

	* tree-ssa-sink.c (select_best_block): Punt if selected block
	has incoming abnormal edges.

gcc/testsuite/ChangeLog:

	* gcc.dg/setjmp-7.c: New test.
---
 gcc/testsuite/gcc.dg/setjmp-7.c | 13 +++++++++++++
 gcc/tree-ssa-sink.c             |  6 ++++++
 2 files changed, 19 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/setjmp-7.c

diff --git a/gcc/testsuite/gcc.dg/setjmp-7.c b/gcc/testsuite/gcc.dg/setjmp-7.c
new file mode 100644
index 000000000..44b5bcbfa
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/setjmp-7.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-guess-branch-probability -w" } */
+/* { dg-require-effective-target indirect_jumps } */
+
+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 )))
+	;
+}
diff --git a/gcc/tree-ssa-sink.c b/gcc/tree-ssa-sink.c
index 66d7ae89e..016ecbaec 100644
--- a/gcc/tree-ssa-sink.c
+++ b/gcc/tree-ssa-sink.c
@@ -208,6 +208,12 @@ select_best_block (basic_block early_bb,
       temp_bb = get_immediate_dominator (CDI_DOMINATORS, temp_bb);
     }
 
+  /* Placing a statement before a setjmp-like function would be invalid
+     (it cannot be reevaluated when execution follows an abnormal edge).
+     If we selected a block with abnormal predecessors, just punt.  */
+  if (bb_has_abnormal_pred (best_bb))
+    return early_bb;
+
   /* If we found a shallower loop nest, then we always consider that
      a win.  This will always give us the most control dependent block
      within that loop nest.  */
-- 
2.33.1

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

[Richard Biener]

On Fri, Jan 14, 2022 at 7:21 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> gcc/ChangeLog:
>
>         * tree-ssa-sink.c (select_best_block): Punt if selected block
>         has incoming abnormal edges.

OK.

> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/setjmp-7.c: New test.
> ---
>  gcc/testsuite/gcc.dg/setjmp-7.c | 13 +++++++++++++
>  gcc/tree-ssa-sink.c             |  6 ++++++
>  2 files changed, 19 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/setjmp-7.c
>
> diff --git a/gcc/testsuite/gcc.dg/setjmp-7.c b/gcc/testsuite/gcc.dg/setjmp-7.c
> new file mode 100644
> index 000000000..44b5bcbfa
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/setjmp-7.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fno-guess-branch-probability -w" } */
> +/* { dg-require-effective-target indirect_jumps } */
> +
> +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 )))
> +       ;
> +}
> diff --git a/gcc/tree-ssa-sink.c b/gcc/tree-ssa-sink.c
> index 66d7ae89e..016ecbaec 100644
> --- a/gcc/tree-ssa-sink.c
> +++ b/gcc/tree-ssa-sink.c
> @@ -208,6 +208,12 @@ select_best_block (basic_block early_bb,
>        temp_bb = get_immediate_dominator (CDI_DOMINATORS, temp_bb);
>      }
>
> +  /* Placing a statement before a setjmp-like function would be invalid
> +     (it cannot be reevaluated when execution follows an abnormal edge).
> +     If we selected a block with abnormal predecessors, just punt.  */
> +  if (bb_has_abnormal_pred (best_bb))
> +    return early_bb;
> +
>    /* If we found a shallower loop nest, then we always consider that
>       a win.  This will always give us the most control dependent block
>       within that loop nest.  */
> --
> 2.33.1
>

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

[Florian Weimer]

* Alexander Monakov via Gcc-patches:

> diff --git a/gcc/testsuite/gcc.dg/setjmp-7.c b/gcc/testsuite/gcc.dg/setjmp-7.c
> new file mode 100644
> index 000000000..44b5bcbfa
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/setjmp-7.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fno-guess-branch-probability -w" } */
> +/* { dg-require-effective-target indirect_jumps } */
> +
> +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 )))
> +	;
> +}

Is the implicit declaration of _setjmp important to this test?
Could we declare it explicitly instead?

Thanks,
Florian

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

[Richard Biener]

> Am 08.11.2023 um 10:04 schrieb Florian Weimer <fweimer@redhat.com>:
> 
> * Alexander Monakov via Gcc-patches:
> 
>> diff --git a/gcc/testsuite/gcc.dg/setjmp-7.c b/gcc/testsuite/gcc.dg/setjmp-7.c
>> new file mode 100644
>> index 000000000..44b5bcbfa
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/setjmp-7.c
>> @@ -0,0 +1,13 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fno-guess-branch-probability -w" } */
>> +/* { dg-require-effective-target indirect_jumps } */
>> +
>> +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 )))
>> +    ;
>> +}
> 
> Is the implicit declaration of _setjmp important to this test?
> Could we declare it explicitly instead?

It shouldn’t be important.

> Thanks,
> Florian
> 

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

[Alexander Monakov]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]


On Wed, 8 Nov 2023, Richard Biener wrote:

> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/setjmp-7.c
> >> @@ -0,0 +1,13 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O2 -fno-guess-branch-probability -w" } */
> >> +/* { dg-require-effective-target indirect_jumps } */
> >> +
> >> +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 )))
> >> +    ;
> >> +}
> > 
> > Is the implicit declaration of _setjmp important to this test?
> > Could we declare it explicitly instead?
> 
> It shouldn’t be important.

Yes, it's an artifact from testcase minimization, sorry about that.

Florian, I see you've sent a patch to fix this up — thank you!

Alexander

[PATCH 2/3] tree-cfg: do not duplicate returns_twice calls

[Alexander Monakov]

A returns_twice call may have associated abnormal edges that correspond
to the "second return" from the call. If the call is duplicated, the
copies of those edges also need to be abnormal, but e.g. tracer does not
enforce that. Just prohibit the (unlikely to be useful) duplication.

gcc/ChangeLog:

	* tree-cfg.c (gimple_can_duplicate_bb_p): Reject blocks with
	calls that may return twice.
---
 gcc/tree-cfg.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index b7fe313b7..a99f1acb4 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -6304,12 +6304,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
     {
       gimple *g = gsi_stmt (gsi);
 
-      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
+      /* Prohibit duplication of returns_twice calls, otherwise associated
+	 abnormal edges also need to be duplicated properly.
+	 An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
 	 duplicated as part of its group, or not at all.
 	 The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
 	 group, so the same holds there.  */
       if (is_gimple_call (g)
-	  && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
+	  && (gimple_call_flags (g) & ECF_RETURNS_TWICE
+	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
 	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
 	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
 	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
-- 
2.33.1

Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls

[Richard Biener]

On Fri, Jan 14, 2022 at 7:21 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> A returns_twice call may have associated abnormal edges that correspond
> to the "second return" from the call. If the call is duplicated, the
> copies of those edges also need to be abnormal, but e.g. tracer does not
> enforce that. Just prohibit the (unlikely to be useful) duplication.

The general CFG copying routines properly duplicate those edges, no?
Tracer uses duplicate_block so it should also get copies of all successor
edges of that block.  It also only traces along normal edges.  What it might
miss is abnormal incoming edges - is that what you are referring to?

That would be a thing we don't handle in duplicate_block on its own but
that callers are expected to do (though I don't see copy_bbs doing that
either).  I wonder if we can trigger this issue for some testcase?

The thing to check would be incoming abnormal edges in
can_duplicate_block_p, not (only) returns twice functions?

Richard.

> gcc/ChangeLog:
>
>         * tree-cfg.c (gimple_can_duplicate_bb_p): Reject blocks with
>         calls that may return twice.
> ---
>  gcc/tree-cfg.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index b7fe313b7..a99f1acb4 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -6304,12 +6304,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
>      {
>        gimple *g = gsi_stmt (gsi);
>
> -      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> +      /* Prohibit duplication of returns_twice calls, otherwise associated
> +        abnormal edges also need to be duplicated properly.
> +        An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
>          duplicated as part of its group, or not at all.
>          The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
>          group, so the same holds there.  */
>        if (is_gimple_call (g)
> -         && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> +         && (gimple_call_flags (g) & ECF_RETURNS_TWICE
> +             || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
> --
> 2.33.1
>

Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls

[Alexander Monakov]

Apologies for the prolonged silence Richard, it is a bit of an obscure topic,
and I was unsure I'd be able to handle any complications in a timely manner.
I'm ready to revisit it now, please see below.

On Mon, 17 Jan 2022, Richard Biener wrote:

> On Fri, Jan 14, 2022 at 7:21 PM Alexander Monakov <amonakov@ispras.ru> wrote:
> >
> > A returns_twice call may have associated abnormal edges that correspond
> > to the "second return" from the call. If the call is duplicated, the
> > copies of those edges also need to be abnormal, but e.g. tracer does not
> > enforce that. Just prohibit the (unlikely to be useful) duplication.
> 
> The general CFG copying routines properly duplicate those edges, no?

No (in fact you say so in the next paragraph). In general I think they cannot,
abnormal edges are a special case, so it should be the responsibility of the
caller.

> Tracer uses duplicate_block so it should also get copies of all successor
> edges of that block.  It also only traces along normal edges.  What it might
> miss is abnormal incoming edges - is that what you are referring to?

Yes (I think its entire point is to build a "trace" of duplicated blocks that
does not have incoming edges in the middle, abnormal or not).

> That would be a thing we don't handle in duplicate_block on its own but
> that callers are expected to do (though I don't see copy_bbs doing that
> either).  I wonder if we can trigger this issue for some testcase?

Oh yes (in fact my desire to find a testcase delayed this quite a bit).
When compiling the following testcase with -O2 -ftracer:

__attribute__((returns_twice))
int rtwice_a(int), rtwice_b(int);

int f(int *x)
{
        volatile unsigned k, i = (*x);

        for (k = 1; (i = rtwice_a(i)) * k; k = 2);

        for (; (i = rtwice_b(i)) * k; k = 4);

        return k;
}

tracer manages to eliminate the ABNORMAL_DISPATCHER block completely, so
the possibility of transferring control back to rtwice_a from rtwice_b
is no longer modeled in the IR. I could spend some time "upgrading" this
to an end-to-end miscompilation, but I hope you agree this is quite broken
already.

> The thing to check would be incoming abnormal edges in
> can_duplicate_block_p, not (only) returns twice functions?

Unfortunately not, abnormal edges are also used for computed gotos, which are
less magic than returns_twice edges and should not block tracer I think.

This implies patch 1/3 [1] unnecessary blocks sinking to computed goto targets.
[1] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588498.html

How would you like to proceed here? Is my initial patch ok?

Alexander

> 
> Richard.
> 
> > gcc/ChangeLog:
> >
> >         * tree-cfg.c (gimple_can_duplicate_bb_p): Reject blocks with
> >         calls that may return twice.
> > ---
> >  gcc/tree-cfg.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> > index b7fe313b7..a99f1acb4 100644
> > --- a/gcc/tree-cfg.c
> > +++ b/gcc/tree-cfg.c
> > @@ -6304,12 +6304,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
> >      {
> >        gimple *g = gsi_stmt (gsi);
> >
> > -      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> > +      /* Prohibit duplication of returns_twice calls, otherwise associated
> > +        abnormal edges also need to be duplicated properly.
> > +        An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> >          duplicated as part of its group, or not at all.
> >          The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
> >          group, so the same holds there.  */
> >        if (is_gimple_call (g)
> > -         && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> > +         && (gimple_call_flags (g) & ECF_RETURNS_TWICE
> > +             || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
> >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
> >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
> > --
> > 2.33.1
> >
> 

Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls

[Richard Biener]

On Tue, Jul 12, 2022 at 10:10 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
> Apologies for the prolonged silence Richard, it is a bit of an obscure topic,
> and I was unsure I'd be able to handle any complications in a timely manner.
> I'm ready to revisit it now, please see below.
>
> On Mon, 17 Jan 2022, Richard Biener wrote:
>
> > On Fri, Jan 14, 2022 at 7:21 PM Alexander Monakov <amonakov@ispras.ru> wrote:
> > >
> > > A returns_twice call may have associated abnormal edges that correspond
> > > to the "second return" from the call. If the call is duplicated, the
> > > copies of those edges also need to be abnormal, but e.g. tracer does not
> > > enforce that. Just prohibit the (unlikely to be useful) duplication.
> >
> > The general CFG copying routines properly duplicate those edges, no?
>
> No (in fact you say so in the next paragraph). In general I think they cannot,
> abnormal edges are a special case, so it should be the responsibility of the
> caller.
>
> > Tracer uses duplicate_block so it should also get copies of all successor
> > edges of that block.  It also only traces along normal edges.  What it might
> > miss is abnormal incoming edges - is that what you are referring to?
>
> Yes (I think its entire point is to build a "trace" of duplicated blocks that
> does not have incoming edges in the middle, abnormal or not).
>
> > That would be a thing we don't handle in duplicate_block on its own but
> > that callers are expected to do (though I don't see copy_bbs doing that
> > either).  I wonder if we can trigger this issue for some testcase?
>
> Oh yes (in fact my desire to find a testcase delayed this quite a bit).
> When compiling the following testcase with -O2 -ftracer:
>
> __attribute__((returns_twice))
> int rtwice_a(int), rtwice_b(int);
>
> int f(int *x)
> {
>         volatile unsigned k, i = (*x);
>
>         for (k = 1; (i = rtwice_a(i)) * k; k = 2);
>
>         for (; (i = rtwice_b(i)) * k; k = 4);
>
>         return k;
> }
>
> tracer manages to eliminate the ABNORMAL_DISPATCHER block completely, so
> the possibility of transferring control back to rtwice_a from rtwice_b
> is no longer modeled in the IR. I could spend some time "upgrading" this
> to an end-to-end miscompilation, but I hope you agree this is quite broken
> already.
>
> > The thing to check would be incoming abnormal edges in
> > can_duplicate_block_p, not (only) returns twice functions?
>
> Unfortunately not, abnormal edges are also used for computed gotos, which are
> less magic than returns_twice edges and should not block tracer I think.

I think computed gotos should use regular edges, only non-local goto should
use abnormals...

I suppose asm goto also uses abnormal edges?

Btw, I don't see how they in general are "less magic".  Sure, we have an
explicit receiver (the destination label), but we can only do edge inserts
if we have a single computed goto edge into a block (we can "move" the
label to the block created when splitting the edge).

> This implies patch 1/3 [1] unnecessary blocks sinking to computed goto targets.
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588498.html
>
> How would you like to proceed here? Is my initial patch ok?

Hmm, so for returns twice calls duplicate_block correctly copies the call
and redirects the provided incoming edge to it.  The API does not
handle adding any further incoming edges - the caller would be responsible
for this.  So I still somewhat fail to see the point here.  If tracer does not
handle extra incoming edges properly then we need to fix tracer?  This
also includes non-local goto (we seem to copy non-local labels just
fine - wasn't there a bugreport about this!?).

So I think can_duplicate_block_p is the wrong place to fix (the RTL side
would need a similar fix anyhow?)

Richard.

> Alexander
>
> >
> > Richard.
> >
> > > gcc/ChangeLog:
> > >
> > >         * tree-cfg.c (gimple_can_duplicate_bb_p): Reject blocks with
> > >         calls that may return twice.
> > > ---
> > >  gcc/tree-cfg.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> > > index b7fe313b7..a99f1acb4 100644
> > > --- a/gcc/tree-cfg.c
> > > +++ b/gcc/tree-cfg.c
> > > @@ -6304,12 +6304,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
> > >      {
> > >        gimple *g = gsi_stmt (gsi);
> > >
> > > -      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> > > +      /* Prohibit duplication of returns_twice calls, otherwise associated
> > > +        abnormal edges also need to be duplicated properly.
> > > +        An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> > >          duplicated as part of its group, or not at all.
> > >          The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
> > >          group, so the same holds there.  */
> > >        if (is_gimple_call (g)
> > > -         && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> > > +         && (gimple_call_flags (g) & ECF_RETURNS_TWICE
> > > +             || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> > >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
> > >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
> > >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
> > > --
> > > 2.33.1
> > >
> >

Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls

[Alexander Monakov]

On Wed, 13 Jul 2022, Richard Biener wrote:

> > > The thing to check would be incoming abnormal edges in
> > > can_duplicate_block_p, not (only) returns twice functions?
> >
> > Unfortunately not, abnormal edges are also used for computed gotos, which are
> > less magic than returns_twice edges and should not block tracer I think.
> 
> I think computed gotos should use regular edges, only non-local goto should
> use abnormals...

Yeah, afaict it's not documented what "abnormal" is supposed to mean :/

> I suppose asm goto also uses abnormal edges?

Heh, no, asm goto appears to use normal edges, but there's an old gap in
their specification: can you use them like computed gotos, i.e. can asm-goto
jump to a computed target? Or must they be similar to plain gotos where the
jump label is redirectable (because it's substitutable in the asm template)?

If you take a restrictive interpretation (asm goto may not jump to a computed
label) then using regular edges looks fine.

> Btw, I don't see how they in general are "less magic".  Sure, we have an
> explicit receiver (the destination label), but we can only do edge inserts
> if we have a single computed goto edge into a block (we can "move" the
> label to the block created when splitting the edge).

Sure, they are a bit magic, but returns_twice edges are even more magic: their
destination looks tied to a label in the IR, but in reality their destination
is inside a call that returns twice (hence GCC must be careful not to insert
anything between the label and the call, like in patch 1/3).

> > This implies patch 1/3 [1] unnecessary blocks sinking to computed goto targets.
> > [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588498.html
> >
> > How would you like to proceed here? Is my initial patch ok?
> 
> Hmm, so for returns twice calls duplicate_block correctly copies the call
> and redirects the provided incoming edge to it.  The API does not
> handle adding any further incoming edges - the caller would be responsible
> for this.  So I still somewhat fail to see the point here.  If tracer does not
> handle extra incoming edges properly then we need to fix tracer?

I think abnormal edges corresponding to computed gotos are fine: we are
attempting to create a chain of blocks with no incoming edges in the middle,
right? Destinations of computed gotos remain at labels of original blocks.

Agreed about correcting this in the tracer.

> This also includes non-local goto (we seem to copy non-local labels just
> fine - wasn't there a bugreport about this!?).

Sorry, no idea about this.

> So I think can_duplicate_block_p is the wrong place to fix (the RTL side
> would need a similar fix anyhow?)

Right. I'm happy to leave both RTL and GIMPLE can_duplicate_block_p as is,
and instead constrain just the tracer. Alternative patch below:

        * tracer.cc (analyze_bb): Disallow duplication of returns_twice calls.

diff --git a/gcc/tracer.cc b/gcc/tracer.cc
index 64517846d..422e2b6a7 100644
--- a/gcc/tracer.cc
+++ b/gcc/tracer.cc
@@ -132,14 +132,19 @@ analyze_bb (basic_block bb, int *count)
   gimple *stmt;
   int n = 0;

+  bool can_dup = can_duplicate_block_p (CONST_CAST_BB (bb));
+
   for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
     {
       stmt = gsi_stmt (gsi);
       n += estimate_num_insns (stmt, &eni_size_weights);
+      if (can_dup && cfun->calls_setjmp && gimple_code (stmt) == GIMPLE_CALL
+         && gimple_call_flags (stmt) & ECF_RETURNS_TWICE)
+       can_dup = false;
     }
   *count = n;

-  cache_can_duplicate_bb_p (bb, can_duplicate_block_p (CONST_CAST_BB (bb)));
+  cache_can_duplicate_bb_p (bb, can_dup);
 }

 /* Return true if E1 is more frequent than E2.  */

Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls

[Richard Biener]

On Wed, Jul 13, 2022 at 4:57 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Wed, 13 Jul 2022, Richard Biener wrote:
>
> > > > The thing to check would be incoming abnormal edges in
> > > > can_duplicate_block_p, not (only) returns twice functions?
> > >
> > > Unfortunately not, abnormal edges are also used for computed gotos, which are
> > > less magic than returns_twice edges and should not block tracer I think.
> >
> > I think computed gotos should use regular edges, only non-local goto should
> > use abnormals...
>
> Yeah, afaict it's not documented what "abnormal" is supposed to mean :/
>
> > I suppose asm goto also uses abnormal edges?
>
> Heh, no, asm goto appears to use normal edges, but there's an old gap in
> their specification: can you use them like computed gotos, i.e. can asm-goto
> jump to a computed target? Or must they be similar to plain gotos where the
> jump label is redirectable (because it's substitutable in the asm template)?
>
> If you take a restrictive interpretation (asm goto may not jump to a computed
> label) then using regular edges looks fine.
>
> > Btw, I don't see how they in general are "less magic".  Sure, we have an
> > explicit receiver (the destination label), but we can only do edge inserts
> > if we have a single computed goto edge into a block (we can "move" the
> > label to the block created when splitting the edge).
>
> Sure, they are a bit magic, but returns_twice edges are even more magic: their
> destination looks tied to a label in the IR, but in reality their destination
> is inside a call that returns twice (hence GCC must be careful not to insert
> anything between the label and the call, like in patch 1/3).

Indeed.  Guess that's what __builtin_setjmp[_receiver] for SJLJ_EH got "right".

When copying a block we do not copy labels so any "jumps" remain to the original
block and thus we are indeed able to isolate normal control flow.  Given that
returns_twice functions _do_ seem to be special, and also special as to how
we handle other abnormal receivers in duplicate_block.  So it might indeed make
sense to special-case them in can_duplicate_block_p ... (sorry for
going back-and-forth ...)

Note that I think this detail of duplicate_block (the function) and the hook
needs to be better documented (the semantics on incoming edges, not duplicating
labels used for incoming control flow).

Can you see as to how to adjust the RTL side for this?  It looks like at least
some places set a REG_SETJMP note on call_insns (emit_call_1), I wonder
if in rtl_verify_flow_info[_1] (or its callees) we can check that such
calls come
first ... they might not since IIRC we do _not_ preserve abnormal edges when
expanding RTL (there's some existing bug about this and how this breaks some
setjmp tests) (but we try to recompute them?).

Sorry about the back-and-forth again ... your original patch looks OK for the
GIMPLE side but can you amend the cfghooks.{cc,h} documentation to
summarize our findings and
the desired semantics of duplicate_block in this respect?

Thanks,
Richard.

> > > This implies patch 1/3 [1] unnecessary blocks sinking to computed goto targets.
> > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588498.html
> > >
> > > How would you like to proceed here? Is my initial patch ok?
> >
> > Hmm, so for returns twice calls duplicate_block correctly copies the call
> > and redirects the provided incoming edge to it.  The API does not
> > handle adding any further incoming edges - the caller would be responsible
> > for this.  So I still somewhat fail to see the point here.  If tracer does not
> > handle extra incoming edges properly then we need to fix tracer?
>
> I think abnormal edges corresponding to computed gotos are fine: we are
> attempting to create a chain of blocks with no incoming edges in the middle,
> right? Destinations of computed gotos remain at labels of original blocks.
>
> Agreed about correcting this in the tracer.
>
> > This also includes non-local goto (we seem to copy non-local labels just
> > fine - wasn't there a bugreport about this!?).
>
> Sorry, no idea about this.
>
> > So I think can_duplicate_block_p is the wrong place to fix (the RTL side
> > would need a similar fix anyhow?)
>
> Right. I'm happy to leave both RTL and GIMPLE can_duplicate_block_p as is,
> and instead constrain just the tracer. Alternative patch below:
>
>         * tracer.cc (analyze_bb): Disallow duplication of returns_twice calls.
>
> diff --git a/gcc/tracer.cc b/gcc/tracer.cc
> index 64517846d..422e2b6a7 100644
> --- a/gcc/tracer.cc
> +++ b/gcc/tracer.cc
> @@ -132,14 +132,19 @@ analyze_bb (basic_block bb, int *count)
>    gimple *stmt;
>    int n = 0;
>
> +  bool can_dup = can_duplicate_block_p (CONST_CAST_BB (bb));
> +
>    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>      {
>        stmt = gsi_stmt (gsi);
>        n += estimate_num_insns (stmt, &eni_size_weights);
> +      if (can_dup && cfun->calls_setjmp && gimple_code (stmt) == GIMPLE_CALL
> +         && gimple_call_flags (stmt) & ECF_RETURNS_TWICE)
> +       can_dup = false;
>      }
>    *count = n;
>
> -  cache_can_duplicate_bb_p (bb, can_duplicate_block_p (CONST_CAST_BB (bb)));
> +  cache_can_duplicate_bb_p (bb, can_dup);
>  }
>
>  /* Return true if E1 is more frequent than E2.  */
>

Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls

[Alexander Monakov]

On Thu, 14 Jul 2022, Richard Biener wrote:

> Indeed.  Guess that's what __builtin_setjmp[_receiver] for SJLJ_EH got "right".
> 
> When copying a block we do not copy labels so any "jumps" remain to the original
> block and thus we are indeed able to isolate normal control flow.  Given that
> returns_twice functions _do_ seem to be special, and also special as to how
> we handle other abnormal receivers in duplicate_block.

We do? Sorry, I don't see what you mean here, can you point me to specific lines?

> So it might indeed make sense to special-case them in can_duplicate_block_p
> ... (sorry for going back-and-forth ...)
> 
> Note that I think this detail of duplicate_block (the function) and the hook
> needs to be better documented (the semantics on incoming edges, not duplicating
> labels used for incoming control flow).
> 
> Can you see as to how to adjust the RTL side for this?  It looks like at least
> some places set a REG_SETJMP note on call_insns (emit_call_1), I wonder
> if in rtl_verify_flow_info[_1] (or its callees) we can check that such
> calls come
> first ... they might not since IIRC we do _not_ preserve abnormal edges when
> expanding RTL (there's some existing bug about this and how this breaks some
> setjmp tests) (but we try to recompute them?).

No, we emit arguments/return value handling before/after a REG_SETJMP call,
and yeah, we don't always properly recompute abnormal edges, so improving
RTL in this respect seems hopeless. For example, it is easy enough to create
a testcase where bb-reordering duplicates a returns_twice call, although it
runs so late that perhaps later passes don't care:

// gcc -O2 --param=max-grow-copy-bb-insns=100
__attribute__((returns_twice))
int rtwice(int);
int g1(int), g2(int);
void f(int i)
{
  do {
    i = i%2 ? g1(i) : g2(i);
  } while (i = rtwice(i));
}

FWIW, I also investigated https://gcc.gnu.org/PR101347

> Sorry about the back-and-forth again ... your original patch looks OK for the
> GIMPLE side but can you amend the cfghooks.{cc,h} documentation to
> summarize our findings and
> the desired semantics of duplicate_block in this respect?

Like below?

---8<---

Subject: [PATCH v3] tree-cfg: do not duplicate returns_twice calls

A returns_twice call may have associated abnormal edges that correspond
to the "second return" from the call. If the call is duplicated, the
copies of those edges also need to be abnormal, but e.g. tracer does not
enforce that. Just prohibit the (unlikely to be useful) duplication.

gcc/ChangeLog:

	* cfghooks.cc (duplicate_block): Expand comment.
	* tree-cfg.cc (gimple_can_duplicate_bb_p): Reject blocks with
	calls that may return twice.
---
 gcc/cfghooks.cc | 13 ++++++++++---
 gcc/tree-cfg.cc |  7 +++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/gcc/cfghooks.cc b/gcc/cfghooks.cc
index e435891fa..c6ac9532c 100644
--- a/gcc/cfghooks.cc
+++ b/gcc/cfghooks.cc
@@ -1086,9 +1086,16 @@ can_duplicate_block_p (const_basic_block bb)
   return cfg_hooks->can_duplicate_block_p (bb);
 }
 
-/* Duplicates basic block BB and redirects edge E to it.  Returns the
-   new basic block.  The new basic block is placed after the basic block
-   AFTER.  */
+/* Duplicate basic block BB, place it after AFTER (if non-null) and redirect
+   edge E to it (if non-null).  Return the new basic block.
+
+   If BB contains a returns_twice call, the caller is responsible for recreating
+   incoming abnormal edges corresponding to the "second return" for the copy.
+   gimple_can_duplicate_bb_p rejects such blocks, while RTL likes to live
+   dangerously.
+
+   If BB has incoming abnormal edges for some other reason, their destinations
+   should be tied to label(s) of the original BB and not the copy.  */
 
 basic_block
 duplicate_block (basic_block bb, edge e, basic_block after, copy_bb_data *id)
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index f846dc2d8..5bcf78198 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -6346,12 +6346,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
     {
       gimple *g = gsi_stmt (gsi);
 
-      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
+      /* Prohibit duplication of returns_twice calls, otherwise associated
+	 abnormal edges also need to be duplicated properly.
+	 An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
 	 duplicated as part of its group, or not at all.
 	 The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
 	 group, so the same holds there.  */
       if (is_gimple_call (g)
-	  && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
+	  && (gimple_call_flags (g) & ECF_RETURNS_TWICE
+	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
 	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
 	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
 	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
-- 
2.35.1

Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls

[Richard Biener]

On Thu, Jul 14, 2022 at 10:12 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
> On Thu, 14 Jul 2022, Richard Biener wrote:
>
> > Indeed.  Guess that's what __builtin_setjmp[_receiver] for SJLJ_EH got "right".
> >
> > When copying a block we do not copy labels so any "jumps" remain to the original
> > block and thus we are indeed able to isolate normal control flow.  Given that
> > returns_twice functions _do_ seem to be special, and also special as to how
> > we handle other abnormal receivers in duplicate_block.
>
> We do? Sorry, I don't see what you mean here, can you point me to specific lines?

I'm referring to

      stmt = gsi_stmt (gsi);
      if (gimple_code (stmt) == GIMPLE_LABEL)
        continue;

but indeed, looking again we do _not_ skip a __builtin_setjmp_receiver
(but I don't
exactly remember the CFG setup with SJLJ EH and setjmp_{receiver,setup}.

> > So it might indeed make sense to special-case them in can_duplicate_block_p
> > ... (sorry for going back-and-forth ...)
> >
> > Note that I think this detail of duplicate_block (the function) and the hook
> > needs to be better documented (the semantics on incoming edges, not duplicating
> > labels used for incoming control flow).
> >
> > Can you see as to how to adjust the RTL side for this?  It looks like at least
> > some places set a REG_SETJMP note on call_insns (emit_call_1), I wonder
> > if in rtl_verify_flow_info[_1] (or its callees) we can check that such
> > calls come
> > first ... they might not since IIRC we do _not_ preserve abnormal edges when
> > expanding RTL (there's some existing bug about this and how this breaks some
> > setjmp tests) (but we try to recompute them?).
>
> No, we emit arguments/return value handling before/after a REG_SETJMP call,
> and yeah, we don't always properly recompute abnormal edges, so improving
> RTL in this respect seems hopeless.

:/  (but yes, nobody got to fixing PR57067 in the last 10 years)

> For example, it is easy enough to create
> a testcase where bb-reordering duplicates a returns_twice call, although it
> runs so late that perhaps later passes don't care:
>
> // gcc -O2 --param=max-grow-copy-bb-insns=100
> __attribute__((returns_twice))
> int rtwice(int);
> int g1(int), g2(int);
> void f(int i)
> {
>   do {
>     i = i%2 ? g1(i) : g2(i);
>   } while (i = rtwice(i));
> }
>
> FWIW, I also investigated https://gcc.gnu.org/PR101347
>
> > Sorry about the back-and-forth again ... your original patch looks OK for the
> > GIMPLE side but can you amend the cfghooks.{cc,h} documentation to
> > summarize our findings and
> > the desired semantics of duplicate_block in this respect?
>
> Like below?

Yes.

Thanks and sorry for the back and forth - this _is_ a mightly
complicated area ...

Richard.

> ---8<---
>
> Subject: [PATCH v3] tree-cfg: do not duplicate returns_twice calls
>
> A returns_twice call may have associated abnormal edges that correspond
> to the "second return" from the call. If the call is duplicated, the
> copies of those edges also need to be abnormal, but e.g. tracer does not
> enforce that. Just prohibit the (unlikely to be useful) duplication.
>
> gcc/ChangeLog:
>
>         * cfghooks.cc (duplicate_block): Expand comment.
>         * tree-cfg.cc (gimple_can_duplicate_bb_p): Reject blocks with
>         calls that may return twice.
> ---
>  gcc/cfghooks.cc | 13 ++++++++++---
>  gcc/tree-cfg.cc |  7 +++++--
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/cfghooks.cc b/gcc/cfghooks.cc
> index e435891fa..c6ac9532c 100644
> --- a/gcc/cfghooks.cc
> +++ b/gcc/cfghooks.cc
> @@ -1086,9 +1086,16 @@ can_duplicate_block_p (const_basic_block bb)
>    return cfg_hooks->can_duplicate_block_p (bb);
>  }
>
> -/* Duplicates basic block BB and redirects edge E to it.  Returns the
> -   new basic block.  The new basic block is placed after the basic block
> -   AFTER.  */
> +/* Duplicate basic block BB, place it after AFTER (if non-null) and redirect
> +   edge E to it (if non-null).  Return the new basic block.
> +
> +   If BB contains a returns_twice call, the caller is responsible for recreating
> +   incoming abnormal edges corresponding to the "second return" for the copy.
> +   gimple_can_duplicate_bb_p rejects such blocks, while RTL likes to live
> +   dangerously.
> +
> +   If BB has incoming abnormal edges for some other reason, their destinations
> +   should be tied to label(s) of the original BB and not the copy.  */
>
>  basic_block
>  duplicate_block (basic_block bb, edge e, basic_block after, copy_bb_data *id)
> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> index f846dc2d8..5bcf78198 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -6346,12 +6346,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
>      {
>        gimple *g = gsi_stmt (gsi);
>
> -      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> +      /* Prohibit duplication of returns_twice calls, otherwise associated
> +        abnormal edges also need to be duplicated properly.
> +        An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
>          duplicated as part of its group, or not at all.
>          The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
>          group, so the same holds there.  */
>        if (is_gimple_call (g)
> -         && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> +         && (gimple_call_flags (g) & ECF_RETURNS_TWICE
> +             || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
> --
> 2.35.1
>

Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls

[Alexander Monakov]

On Tue, 19 Jul 2022, Richard Biener wrote:

> > Like below?
> 
> Yes.
> 
> Thanks and sorry for the back and forth - this _is_ a mightly
> complicated area ...

No problem!  This is the good, healthy kind of back-and-forth, and
I am grateful.

Pushed, including the tree-cfg validator enhancement in patch 3/3.

Alexander

Re: [PATCH 2/3] tree-cfg: do not duplicate returns_twice calls

[Jeff Law]

On 7/13/2022 1:13 AM, Richard Biener via Gcc-patches wrote:
> On Tue, Jul 12, 2022 at 10:10 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>>
>> Apologies for the prolonged silence Richard, it is a bit of an obscure topic,
>> and I was unsure I'd be able to handle any complications in a timely manner.
>> I'm ready to revisit it now, please see below.
>>
>> On Mon, 17 Jan 2022, Richard Biener wrote:
>>
>>> On Fri, Jan 14, 2022 at 7:21 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>>>> A returns_twice call may have associated abnormal edges that correspond
>>>> to the "second return" from the call. If the call is duplicated, the
>>>> copies of those edges also need to be abnormal, but e.g. tracer does not
>>>> enforce that. Just prohibit the (unlikely to be useful) duplication.
>>> The general CFG copying routines properly duplicate those edges, no?
>> No (in fact you say so in the next paragraph). In general I think they cannot,
>> abnormal edges are a special case, so it should be the responsibility of the
>> caller.
>>
>>> Tracer uses duplicate_block so it should also get copies of all successor
>>> edges of that block.  It also only traces along normal edges.  What it might
>>> miss is abnormal incoming edges - is that what you are referring to?
>> Yes (I think its entire point is to build a "trace" of duplicated blocks that
>> does not have incoming edges in the middle, abnormal or not).
>>
>>> That would be a thing we don't handle in duplicate_block on its own but
>>> that callers are expected to do (though I don't see copy_bbs doing that
>>> either).  I wonder if we can trigger this issue for some testcase?
>> Oh yes (in fact my desire to find a testcase delayed this quite a bit).
>> When compiling the following testcase with -O2 -ftracer:
>>
>> __attribute__((returns_twice))
>> int rtwice_a(int), rtwice_b(int);
>>
>> int f(int *x)
>> {
>>          volatile unsigned k, i = (*x);
>>
>>          for (k = 1; (i = rtwice_a(i)) * k; k = 2);
>>
>>          for (; (i = rtwice_b(i)) * k; k = 4);
>>
>>          return k;
>> }
>>
>> tracer manages to eliminate the ABNORMAL_DISPATCHER block completely, so
>> the possibility of transferring control back to rtwice_a from rtwice_b
>> is no longer modeled in the IR. I could spend some time "upgrading" this
>> to an end-to-end miscompilation, but I hope you agree this is quite broken
>> already.
>>
>>> The thing to check would be incoming abnormal edges in
>>> can_duplicate_block_p, not (only) returns twice functions?
>> Unfortunately not, abnormal edges are also used for computed gotos, which are
>> less magic than returns_twice edges and should not block tracer I think.
> I think computed gotos should use regular edges, only non-local goto should
> use abnormals...
>
> I suppose asm goto also uses abnormal edges?
>
> Btw, I don't see how they in general are "less magic".  Sure, we have an
> explicit receiver (the destination label), but we can only do edge inserts
> if we have a single computed goto edge into a block (we can "move" the
> label to the block created when splitting the edge).
I suspect treating them like abnormals probably came from the inability 
to reliably split them way back when we introduced RTL GCSE and the like.

Jeff

[PATCH 3/3] tree-cfg: check placement of returns_twice calls

[Alexander Monakov]

When a returns_twice call has an associated abnormal edge, the edge
corresponds to the "second return" from the call. It wouldn't make sense
if any executable statements appeared between the call and the
destination of the edge (they wouldn't be re-executed upon the "second
return"), so verify that.

gcc/ChangeLog:

	* tree-cfg.c (gimple_verify_flow_info): Check placement of
	returns_twice calls.
---
 gcc/tree-cfg.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index a99f1acb4..70bd31d3d 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -5644,6 +5644,7 @@ gimple_verify_flow_info (void)
 	}
 
       /* Verify that body of basic block BB is free of control flow.  */
+      bool seen_nondebug_stmt = false;
       for (; !gsi_end_p (gsi); gsi_next (&gsi))
 	{
 	  gimple *stmt = gsi_stmt (gsi);
@@ -5664,6 +5665,38 @@ gimple_verify_flow_info (void)
 		     gimple_label_label (label_stmt), bb->index);
 	      err = 1;
 	    }
+
+	  /* Check that no statements appear between a returns_twice call
+	     and its associated abnormal edge.  */
+	  if (gimple_code (stmt) == GIMPLE_CALL
+	      && gimple_call_flags (stmt) & ECF_RETURNS_TWICE)
+	    {
+	      const char *misplaced = NULL;
+	      /* TM is an exception: it points abnormal edges just after the
+		 call that starts a transaction, i.e. it must end the BB.  */
+	      if (gimple_call_builtin_p (stmt, BUILT_IN_TM_START))
+		{
+		  if (single_succ_p (bb)
+		      && bb_has_abnormal_pred (single_succ (bb))
+		      && !gsi_one_nondebug_before_end_p (gsi))
+		    misplaced = "not last";
+		}
+	      else
+		{
+		  if (seen_nondebug_stmt
+		      && bb_has_abnormal_pred (bb))
+		    misplaced = "not first";
+		}
+	      if (misplaced)
+		{
+		  error ("returns_twice call is %s in basic block %d",
+			 misplaced, bb->index);
+		  print_gimple_stmt (stderr, stmt, 0, TDF_SLIM);
+		  err = 1;
+		}
+	    }
+	  if (!is_gimple_debug (stmt))
+	    seen_nondebug_stmt = true;
 	}
 
       gsi = gsi_last_nondebug_bb (bb);
-- 
2.33.1

Re: [PATCH 3/3] tree-cfg: check placement of returns_twice calls

[Richard Biener]

On Fri, Jan 14, 2022 at 7:21 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> When a returns_twice call has an associated abnormal edge, the edge
> corresponds to the "second return" from the call. It wouldn't make sense
> if any executable statements appeared between the call and the
> destination of the edge (they wouldn't be re-executed upon the "second
> return"), so verify that.

OK for next stage1.

Thanks,
Richard.

> gcc/ChangeLog:
>
>         * tree-cfg.c (gimple_verify_flow_info): Check placement of
>         returns_twice calls.
> ---
>  gcc/tree-cfg.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index a99f1acb4..70bd31d3d 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -5644,6 +5644,7 @@ gimple_verify_flow_info (void)
>         }
>
>        /* Verify that body of basic block BB is free of control flow.  */
> +      bool seen_nondebug_stmt = false;
>        for (; !gsi_end_p (gsi); gsi_next (&gsi))
>         {
>           gimple *stmt = gsi_stmt (gsi);
> @@ -5664,6 +5665,38 @@ gimple_verify_flow_info (void)
>                      gimple_label_label (label_stmt), bb->index);
>               err = 1;
>             }
> +
> +         /* Check that no statements appear between a returns_twice call
> +            and its associated abnormal edge.  */
> +         if (gimple_code (stmt) == GIMPLE_CALL
> +             && gimple_call_flags (stmt) & ECF_RETURNS_TWICE)
> +           {
> +             const char *misplaced = NULL;
> +             /* TM is an exception: it points abnormal edges just after the
> +                call that starts a transaction, i.e. it must end the BB.  */
> +             if (gimple_call_builtin_p (stmt, BUILT_IN_TM_START))
> +               {
> +                 if (single_succ_p (bb)
> +                     && bb_has_abnormal_pred (single_succ (bb))
> +                     && !gsi_one_nondebug_before_end_p (gsi))
> +                   misplaced = "not last";
> +               }
> +             else
> +               {
> +                 if (seen_nondebug_stmt
> +                     && bb_has_abnormal_pred (bb))
> +                   misplaced = "not first";
> +               }
> +             if (misplaced)
> +               {
> +                 error ("returns_twice call is %s in basic block %d",
> +                        misplaced, bb->index);
> +                 print_gimple_stmt (stderr, stmt, 0, TDF_SLIM);
> +                 err = 1;
> +               }
> +           }
> +         if (!is_gimple_debug (stmt))
> +           seen_nondebug_stmt = true;
>         }
>
>        gsi = gsi_last_nondebug_bb (bb);
> --
> 2.33.1
>