| Message ID | 4E566F7A.4080004@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 25, 2011 at 5:51 PM, Tom de Vries <vries@codesourcery.com> wrote: > Hi Richard, > > thanks for the review. > > On 08/25/2011 12:45 PM, Richard Guenther wrote: >> On Thu, Aug 25, 2011 at 12:32 PM, Tom de Vries <vries@codesourcery.com> wrote: >>> Jakub, >>> >>> This patch fixes a segmentation violation, which occurs when printing a MEM_REF >>> or COMPONENT_REF containing a released ssa name. This can happen when we print >>> basic blocks upon removal, enabled by -ftree-dump-tree-*-details (see >>> remove_bb:tree-cfg.c). >> >> Where do we dump stmts there? >> > > In dump_bb: > > static void > remove_bb (basic_block bb) > { > gimple_stmt_iterator i; > > if (dump_file) > { > fprintf (dump_file, "Removing basic block %d\n", bb->index); > if (dump_flags & TDF_DETAILS) > { > dump_bb (bb, dump_file, 0); > fprintf (dump_file, "\n"); > } > } > >>> Bootstrapped and reg-tested on x86_64. >>> >>> OK for trunk? >> >> At least >> >> TREE_TYPE (TREE_OPERAND (node, 1)) != NULL_TREE >> >> is always true. >> > > Right. > >> The comment before the new lines is now in the wrong place and this >> check at least needs a comment as well. >> > > Ok, fixed that. > >> But - it's broken to dump freed stuff, why and where do we do this? >> > > Sorry, I did not realize that. > > The scenario is as follows: fnsplit splits a function, and as todo > cleanup_tree_cfg is called and unreachable blocks are removed, among which > blocks 12 and 13. > > Block 12 contains a use of 45: > > # BLOCK 12 freq:9100 > # PRED: 13 > D.13888_46 = *sD.13886_45; > > Block 13 contains a def of 45: > > Block 13 > # BLOCK 13 > # PRED: 11 12 > ... > # sD.13886_45 = PHI <sD.13886_44(11), sD.13886_49(12)> > ... > if (sizeD.8479_2 > iD.13887_50) > goto <bb 12>; > else > goto <bb 14>; > # SUCC: 12 14 > > > First block 13 is removed, and remove_phi_nodes_and_edges_for_unreachable_block > in remove_bb removes the phi def and releases version 45. Then block 12 is > removed, and before removal it is dumped by dump_bb in remove_bb, triggering the > segv. > > The order of removal is determined by the 2nd loop in delete_unreachable_blocks, > which is chosen because there is no dominator info present: > > for (b = EXIT_BLOCK_PTR->prev_bb; b != ENTRY_BLOCK_PTR; b = prev_bb) > { > prev_bb = b->prev_bb; > > if (!(b->flags & BB_REACHABLE)) > { > delete_basic_block (b); > changed = true; > } > } > > I'm not sure how to fix this. Hm, it's probably easiest to fixup the dumper here indeed. > > Another occurance of the same segv is in remove_dead_inserted_code: > > EXECUTE_IF_SET_IN_BITMAP (inserted_exprs, 0, i, bi) > { > t = SSA_NAME_DEF_STMT (ssa_name (i)); > if (!gimple_plf (t, NECESSARY)) > { > gimple_stmt_iterator gsi; > > if (dump_file && (dump_flags & TDF_DETAILS)) > { > fprintf (dump_file, "Removing unnecessary insertion:"); > print_gimple_stmt (dump_file, t, 0, 0); > } > > gsi = gsi_for_stmt (t); > if (gimple_code (t) == GIMPLE_PHI) > remove_phi_node (&gsi, true); > else > { > gsi_remove (&gsi, true); > release_defs (t); > } > } > } > > Here a version is released, while it's used in the defining statement of > version+1, which is subsequently printed. This is easy to fix by splitting the > loop, I'll make a patch for this. Probably also not worth "fixing". I guess we can simply go with your patch, which in it's updated form is ok for trunk. Thanks, Richard. > > There might be other occurrences (I triggered these 2 doing a gcc build), but I > cannot trigger others until delete_unreachable_blocks does not trigger anymore. > >> Richard. >> > > Updated untested patch attached, I'll test this patch together with the > remove_dead_inserted_code patch. > > Thanks, > - Tom > >>> 2011-08-25 Tom de Vries <tom@codesourcery.com> >>> >>> * tree-pretty-print (dump_generic_node): Test for NULL_TREE before >>> accessing TREE_TYPE. >>> > >
Index: gcc/tree-pretty-print.c =================================================================== --- gcc/tree-pretty-print.c (revision 176554) +++ gcc/tree-pretty-print.c (working copy) @@ -809,6 +809,8 @@ dump_generic_node (pretty_printer *buffe infer them and MEM_ATTR caching will share MEM_REFs with differently-typed op0s. */ && TREE_CODE (TREE_OPERAND (node, 0)) != INTEGER_CST + /* Released SSA_NAMES have no TREE_TYPE. */ + && TREE_TYPE (TREE_OPERAND (node, 0)) != NULL_TREE /* Same pointer types, but ignoring POINTER_TYPE vs. REFERENCE_TYPE. */ && (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 0))) @@ -1175,6 +1177,8 @@ dump_generic_node (pretty_printer *buffe can't infer them and MEM_ATTR caching will share MEM_REFs with differently-typed op0s. */ && TREE_CODE (TREE_OPERAND (op0, 0)) != INTEGER_CST + /* Released SSA_NAMES have no TREE_TYPE. */ + && TREE_TYPE (TREE_OPERAND (op0, 0)) != NULL_TREE /* Same pointer types, but ignoring POINTER_TYPE vs. REFERENCE_TYPE. */ && (TREE_TYPE (TREE_TYPE (TREE_OPERAND (op0, 0)))