Patchwork fix for segmentation violation in dump_generic_node

login
register
mail settings
Submitter Tom de Vries
Date Aug. 25, 2011, 3:51 p.m.
Message ID <4E566F7A.4080004@codesourcery.com>
Download mbox | patch
Permalink /patch/111601/
State New
Headers show

Comments

Tom de Vries - Aug. 25, 2011, 3:51 p.m.
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.


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  <tom@codesourcery.com>
>>
>>        * tree-pretty-print (dump_generic_node): Test for NULL_TREE before
>>        accessing TREE_TYPE.
>>
Richard Guenther - Aug. 26, 2011, 7:56 a.m.
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.
>>>
>
>

Patch

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