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