On Thu, Aug 25, 2011 at 5:51 PM, Tom de Vries <vr...@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 <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.
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 <t...@codesourcery.com>
>>>
>>> * tree-pretty-print (dump_generic_node): Test for NULL_TREE before
>>> accessing TREE_TYPE.
>>>
>
>
