Fix flags for edges from/to entry/exit basic blocks

[dvyukov, 2011-12-12 13:43:13]

Fix flags for edges from/to entry/exit basic blocks.
W/o this patch I hit internal asserts when trying to
split the edge from entry block.

Index: gcc/cgraphunit.c
===================================================================
--- gcc/cgraphunit.c	(revision 182237)
+++ gcc/cgraphunit.c	(working copy)
@@ -1459,8 +1459,8 @@
 
   /* Create BB for body of the function and connect it properly.  */
   bb = create_basic_block (NULL, (void *) 0, ENTRY_BLOCK_PTR);
-  make_edge (ENTRY_BLOCK_PTR, bb, 0);
-  make_edge (bb, EXIT_BLOCK_PTR, 0);
+  make_edge (ENTRY_BLOCK_PTR, bb, EDGE_FALLTHRU);
+  make_edge (bb, EXIT_BLOCK_PTR, EDGE_FALLTHRU);
 
   return bb;
 }
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 182237)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,8 @@
+2011-12-12  Dmitry Vyukov  <dvyukov@google.com>
+
+	* cgraphunit.c (init_lowered_empty_function):
+	Fix flags for new edges.
+
 2011-12-12  Torvald Riegel  <triegel@redhat.com>
 
 	* gimplify.c (voidify_wrapper_expr): Add default handling for

--
This patch is available for review at http://codereview.appspot.com/5486043

[Diego Novillo, 2011-12-12 14:23:31]

On Mon, Dec 12, 2011 at 08:43, Dmitriy Vyukov <dvyukov@google.com> wrote:
> Fix flags for edges from/to entry/exit basic blocks.
> W/o this patch I hit internal asserts when trying to
> split the edge from entry block.

Please specify how you tested it
(http://gcc.gnu.org/contribute.html#testing).  OK for trunk, if
testing succeeds.


Diego.

[dvyukov, 2011-12-12 16:18:57]

On 2011/12/12 14:23:31, Diego Novillo wrote:
> On Mon, Dec 12, 2011 at 08:43, Dmitriy Vyukov <mailto:dvyukov@google.com>
wrote:
> > Fix flags for edges from/to entry/exit basic blocks.
> > W/o this patch I hit internal asserts when trying to
> > split the edge from entry block.
> 
> Please specify how you tested it
> (http://gcc.gnu.org/contribute.html#testing).  OK for trunk, if
> testing succeeds.


I've done full 3 stage build for all front-ends, then 'make bootstrap', then
diff output of 'make check-gcc -j16 RUNTESTFLAGS="dg.exp"' with non-modified
version. Everything passed successfully. All that on Linux/amd64.
Unfortunately that other script for gcc testing does not work on my machine...

[Diego Novillo, 2011-12-12 16:24:47]

On 11-12-12 11:18 , dvyukov@google.com wrote:

> I've done full 3 stage build for all front-ends, then 'make bootstrap',
> then diff output of 'make check-gcc -j16 RUNTESTFLAGS="dg.exp"' with
> non-modified version. Everything passed successfully. All that on
> Linux/amd64.

OK, thanks.  That's enough.

> Unfortunately that other script for gcc testing does not work on my
> machine...

Other script?  You mean the one we use internally?  Grab me on IM.


Diego.

[pinskia_gmail.com, 2011-12-12 23:05:47]

On Mon, Dec 12, 2011 at 3:04 PM, Hans-Peter Nilsson
<hans-peter.nilsson@axis.com> wrote:
> (Please don't forget to CC gcc-patches on replies.  Thanks.)
>
>> From: Dmitriy Vyukov <dvyukov@google.com>
>> Date: Mon, 12 Dec 2011 14:43:10 +0100
>
>> Fix flags for edges from/to entry/exit basic blocks.
>> W/o this patch I hit internal asserts when trying to
>> split the edge from entry block.
>>
>> Index: gcc/cgraphunit.c
>> ===================================================================
>> --- gcc/cgraphunit.c  (revision 182237)
>> +++ gcc/cgraphunit.c  (working copy)
>> @@ -1459,8 +1459,8 @@
>>
>>    /* Create BB for body of the function and connect it properly.  */
>>    bb = create_basic_block (NULL, (void *) 0, ENTRY_BLOCK_PTR);
>> -  make_edge (ENTRY_BLOCK_PTR, bb, 0);
>> -  make_edge (bb, EXIT_BLOCK_PTR, 0);
>> +  make_edge (ENTRY_BLOCK_PTR, bb, EDGE_FALLTHRU);
>> +  make_edge (bb, EXIT_BLOCK_PTR, EDGE_FALLTHRU);
>>
>>    return bb;
>>  }
>> Index: gcc/ChangeLog
>> ===================================================================
>> --- gcc/ChangeLog     (revision 182237)
>> +++ gcc/ChangeLog     (working copy)
>> @@ -1,3 +1,8 @@
>> +2011-12-12  Dmitry Vyukov  <dvyukov@google.com>
>> +
>> +     * cgraphunit.c (init_lowered_empty_function):
>> +     Fix flags for new edges.
>> +
>
> This caused a build failure for cris-elf (i.e. it is the most
> likely patch in the breaking range r182243:182256):
>
> In member function 'void
> std::istrstream::_ZTv0_n12_NSt10istrstreamD0Ev()':
>
/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libstdc++-v3/include/backward/strstream:134:13:
> error: fallthru to exit from bb 2
>
/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libstdc++-v3/include/backward/strstream:134:13:
> error: fallthru edge after a control statement in bb 2
>
/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libstdc++-v3/include/backward/strstream:134:13:
> error: wrong outgoing edge flags at end of bb 2
>
/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libstdc++-v3/include/backward/strstream:134:13:
> internal compiler error: verify_flow_info failed

And it causes a few failures on x86_64-linux-gnu also.
FAIL: g++.dg/abi/covariant5.C -std=c++98 (internal compiler error)
FAIL: g++.dg/abi/covariant5.C -std=c++98 (test for excess errors)
FAIL: g++.dg/abi/covariant5.C -std=c++11 (internal compiler error)
FAIL: g++.dg/abi/covariant5.C -std=c++11 (test for excess errors)

Thanks,
Andrew Pinski

[dvyukov, 2011-12-13 10:43:18]

On Tue, Dec 13, 2011 at 3:04 AM, Hans-Peter Nilsson
<hans-peter.nilsson@axis.com> wrote:
>
> (Please don't forget to CC gcc-patches on replies.  Thanks.)
>
> > From: Dmitriy Vyukov <dvyukov@google.com>
> > Date: Mon, 12 Dec 2011 14:43:10 +0100
>
> > Fix flags for edges from/to entry/exit basic blocks.
> > W/o this patch I hit internal asserts when trying to
> > split the edge from entry block.
> >
> > Index: gcc/cgraphunit.c
> > ===================================================================
> > --- gcc/cgraphunit.c  (revision 182237)
> > +++ gcc/cgraphunit.c  (working copy)
> > @@ -1459,8 +1459,8 @@
> >
> >    /* Create BB for body of the function and connect it properly.  */
> >    bb = create_basic_block (NULL, (void *) 0, ENTRY_BLOCK_PTR);
> > -  make_edge (ENTRY_BLOCK_PTR, bb, 0);
> > -  make_edge (bb, EXIT_BLOCK_PTR, 0);
> > +  make_edge (ENTRY_BLOCK_PTR, bb, EDGE_FALLTHRU);
> > +  make_edge (bb, EXIT_BLOCK_PTR, EDGE_FALLTHRU);
> >
> >    return bb;
> >  }
> > Index: gcc/ChangeLog
> > ===================================================================
> > --- gcc/ChangeLog     (revision 182237)
> > +++ gcc/ChangeLog     (working copy)
> > @@ -1,3 +1,8 @@
> > +2011-12-12  Dmitry Vyukov  <dvyukov@google.com>
> > +
> > +     * cgraphunit.c (init_lowered_empty_function):
> > +     Fix flags for new edges.
> > +
>
> This caused a build failure for cris-elf (i.e. it is the most
> likely patch in the breaking range r182243:182256):
>
> In member function 'void
> std::istrstream::_ZTv0_n12_NSt10istrstreamD0Ev()':
>
/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libstdc++-v3/include/backward/strstream:134:13:
> error: fallthru to exit from bb 2
>
/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libstdc++-v3/include/backward/strstream:134:13:
> error: fallthru edge after a control statement in bb 2
>
/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libstdc++-v3/include/backward/strstream:134:13:
> error: wrong outgoing edge flags at end of bb 2
>
/tmp/hpautotest-gcc1/cris-elf/gccobj/cris-elf/libstdc++-v3/include/backward/strstream:134:13:
> internal compiler error: verify_flow_info failed
>
> Details in PR51521.
>


Hi,

Sorry for the trouble. What should I do now?
Just in case I've prepared a patch that rolls it back:
http://codereview.appspot.com/5485054

Since it has got the attention, I will describe my initial problem.
I am testing ThreadSanitizer instrumentation pass for gcc. I've
successfully compiled like a million LOCs, but then hit the following
crash:

In file included from foo.cc:4:0:
foo.h: In member function 'bar':
foo.h:92:34: internal compiler
error: in gimple_redirect_edge_and_branch, at tree-cfg.c:5013

My instrumentation code that causes the crash is (split entry edge and
add a new bb at func entry):

static void
instrument_func_entry (void)
{
 gimple_seq seq;
 basic_block entry_bb;
 edge entry_edge;
 gimple_stmt_iterator gsi;

 /* Insert new BB before the first BB.  */
 seq = build_func_entry_instr ();
 gcc_assert (seq != NULL);
 entry_bb = ENTRY_BLOCK_PTR;
 entry_edge = single_succ_edge (entry_bb);
 set_location (seq, cfun->function_start_locus);
 entry_bb = split_edge (entry_edge);
 gsi = gsi_start_bb (entry_bb);
 gsi_insert_seq_after (&gsi, seq, GSI_NEW_STMT);
}

Full code is here:
http://gcc.gnu.org/viewcvs/branches/google/main/gcc/tree-tsan.c?view=markup
The pass is executed after pass_sink_code:
NEXT_PASS (pass_split_crit_edges);
NEXT_PASS (pass_pre);
NEXT_PASS (pass_sink_code);
NEXT_PASS (pass_asan);
NEXT_PASS (pass_tsan); <---- HERE
NEXT_PASS (pass_tree_loop);

Here is the dump of the source function just before my pass:

;; Function foo(foo,
funcdef_no=9728, decl_uid=156387, cgraph_uid=4074)
;; 1 loops found
;;
;; Loop 0
;;  header 0, latch 1
;;  depth 0, outer -1
;;  nodes: 0 1 2 5 3 4
;; 2 succs { 3 5 }
;; 5 succs { 4 }
;; 3 succs { 4 }
;; 4 succs { 1 }
T* Y::foo(int)
(struct Y * const this, int fd)
{
 struct Y * adjusted_this.1221;
<bb 2>:
 retval.1220_6 = *.LTHUNK22 (this_3(D), fd_4(D));
 if (retval.1220_6 != 0B)
   goto <bb 3> (<L0>);
 else
   goto <bb 5>;
<bb 5>:
 goto <bb 4>;
<L0>:
 adjusted_this.1221_8 = retval.1220_6 + 8;
<bb 4>:
 # adjusted_this.1221_1 = PHI <adjusted_this.1221_8(3), 0B(5)>
 return adjusted_this.1221_1;
}


And here is the gdb session:

Breakpoint 2, fancy_abort (file=0x10d53b0 "gcc/tree-cfg.c", line=5013,
   function=0x10d81a0 "gimple_redirect_edge_and_branch")
   at gcc/diagnostic.c:892
892     {
(gdb) bt
#0  fancy_abort (file=0x10d53b0 "gcc/tree-cfg.c", line=5013,
   function=0x10d81a0 "gimple_redirect_edge_and_branch")
   at gcc/diagnostic.c:892
#1  0x0000000000a3af8a in gimple_redirect_edge_and_branch
(e=0x7ffff0b6d640, dest=0x7ffff09ee240)
   at gcc/tree-cfg.c:5013
#2  0x000000000074a47a in redirect_edge_and_branch (e=0x7ffff0b6d640,
dest=<optimized out>)
   at gcc/cfghooks.c:323
#3  0x0000000000a2ae8d in gimple_split_edge (edge_in=0x7ffff0b6d640)
   at gcc/tree-cfg.c:2564
#4  0x000000000074a9d7 in split_edge (e=0x7ffff0b6d640)
   at gcc/cfghooks.c:550
#5  0x0000000000badc27 in instrument_func_entry ()
   at gcc/tree-tsan.c:1020
#6  tsan_pass () at gcc/tree-tsan.c:1073
#7  0x0000000000981918 in execute_one_pass (pass=0x13f66c0)
   at gcc/passes.c:2082
#8  0x0000000000981c85 in execute_pass_list (pass=0x13f66c0)
   at gcc/passes.c:2137
#9  0x0000000000981c97 in execute_pass_list (pass=0x13f4bc0)
   at gcc/passes.c:2138
#10 0x0000000000a8a92e in tree_rest_of_compilation (fndecl=0x7ffff27d7f00)
   at gcc/tree-optimize.c:465
#11 0x0000000000767a96 in cgraph_expand_function (node=0x7ffff16625c8)
   at gcc/cgraphunit.c:1967
#12 0x0000000000767c4f in cgraph_process_new_functions ()
   at gcc/cgraphunit.c:275
#13 0x0000000000769bea in cgraph_expand_all_functions ()
   at gcc/cgraphunit.c:2029
#14 cgraph_optimize () at gcc/cgraphunit.c:2313
#15 0x0000000000769ffa in cgraph_finalize_compilation_unit ()
   at gcc/cgraphunit.c:1342
#16 0x000000000059d143 in cp_write_global_declarations ()
   at gcc/cp/decl2.c:4074
#17 0x0000000000a25c8b in compile_file () at gcc/toplev.c:593
#18 do_compile () at gcc/toplev.c:1953
#19 toplev_main (argc=145, argv=0x7fffffffd388) at gcc/toplev.c:2029
#20 0x00007ffff786ed5d in __libc_start_main () from /usr/grte/v2/lib64/libc.so.6
#21 0x00000000004c15ce in _start () at ../sysdeps/x86_64/elf/start.S:114

(gdb) up
#1  0x0000000000a3af8a in gimple_redirect_edge_and_branch
(e=0x7ffff0b6d640, dest=0x7ffff09ee240)
   at gcc/tree-cfg.c:5013
5013          gcc_assert (e->flags & EDGE_FALLTHRU);
(gdb) print *e
$15 = {src = 0x7ffff0b1f8a0, dest = 0x7ffff0b1f960, insns = {g = 0x0,
r = 0x0}, aux = 0x0, goto_block = 0x0, goto_locus = 0, dest_idx = 0,
flags = 4096, probability = 0, count = 0}
(gdb) print *e->src
$16 = {preds = 0x0, succs = 0x7ffff0b6b6e0, aux = 0x0, loop_father =
0x0, dom = {0x1980cb0, 0x0}, prev_bb = 0x0, next_bb = 0x7ffff09ee240,
il = {gimple = 0x0, rtl = 0x0}, count = 0, index = 0, loop_depth = 0,
frequency = 0,
 flags = 0}
(gdb) print *e->src->succs
$4 = {base = {prefix = {num = 1, alloc = 4}, vec = {0x7ffff0b6d640}}}
(gdb) print *e->dest
$17 = {preds = 0x7ffff0b6b708, succs = 0x7ffff0b6b730, aux = 0x0,
loop_father = 0x0, dom = {0x1980a70, 0x0}, prev_bb = 0x7ffff09ee240,
next_bb = 0x7ffff09ee1e0, il = {gimple = 0x7ffff0b1e9f0, rtl =
0x7ffff0b1e9f0}, count = 0,
 index = 2, loop_depth = 0, frequency = 0, flags = 3}


The assert that fails is:
5013          gcc_assert (e->flags & EDGE_FALLTHRU);
e->flags = 4096 which is
#define EDGE_EXECUTABLE         0x1000  /* Edge is executable.  Only
valid during SSA-CCP.  */

So what does EDGE_FALLTHRU mean? Is it just a non-conditional edge? If
so, the edges that I marked as EDGE_FALLTHRU look like they are indeed
fallthru. Does that mean that the regressions are caused by other
issues? If the edges must not be marked as  EDGE_FALLTHRU, then how to
fix the failing assert?

I am not sure as to whether it is related or not, but on the very same
source function I got another error (after successfully compiling like
a million LOCs):

foo.h: In member function 'T* Y::foo(int)':
foo.h:92:34: internal compiler error: in verify_curr_properties, at
passes.c:1816

Initially I've specified for my pass that it requires PROP_trees, but
it turned out that PROP_gimple_lomp is not calculated. For now I've
just replaced PROP_trees with PROP_ssa. However I think that all
passes in the middle of the middle-end assume that OpenMP is already
lowered. Does it mean that some passes were not executed for the
source function?..

[Diego Novillo, 2011-12-13 14:22:09]

On Tue, Dec 13, 2011 at 05:43, Dmitry Vyukov <dvyukov@google.com> wrote:

> Sorry for the trouble. What should I do now?
> Just in case I've prepared a patch that rolls it back:
> http://codereview.appspot.com/5485054

Dmitry, please roll back the patch for now.  You should be able to see
the failures if you run full checking (I missed the fact that you had
not run full checking in your initial patch, sorry).

When testing the patch, you should just run 'make -jN -k check' from
the toplevel build directory, do not just test a subset of the
testsuite.


Diego.

[Diego Novillo, 2011-12-13 14:29:00]

On 11-12-13 09:22 , Diego Novillo wrote:

> When testing the patch, you should just run 'make -jN -k check' from
> the toplevel build directory, do not just test a subset of the
> testsuite.

Additionally, you will need to configure and build with all the default 
languages.  Your change affects everything.


Diego.