Patchwork Fix debug info for expr and jump stmt

login
register
mail settings
Submitter Richard Guenther
Date Oct. 27, 2012, 6:07 p.m.
Message ID <CAFiYyc0f6=z6qCMKkbF+dfF8p8adqb-rqyge439WR=NxGmECWg@mail.gmail.com>
Download mbox | patch
Permalink /patch/194627/
State New
Headers show

Comments

Richard Guenther - Oct. 27, 2012, 6:07 p.m.
On Sat, Oct 27, 2012 at 12:53 AM, Dehao Chen <dehao@google.com> wrote:
> Hi,
>
> I've updated the patch:
>
> 1. abandon the changes in cfgexpand.c
> 2. set the block for trees when lowering gimple stmt.
> 3. add a unittest.



why do you need this?  The stmt location is taken from the operands and in fact
they may have more precise locations.  So it seems completely pointless to me
(wasting location entries where a NULL block is just fine (take the block from
the stmt)).

So, what is the issue you try to fix?  Yes, the stmt operands will
keep the BLOCK
live, but it is live if the frontend assigned it.

Richard.

> However, this patch will trigger two lto bug when asserting
> LTO_NO_PREVAIL for TREE_CHAIN. After debugging for a while, I found
> that the problem was also there even without the patch. This patch
> just reveal the problem by moving a decl into cache so that it will be
> checked. As I'm not familiar with LTO, not quite sure what the root
> problem is. Can anyone help take a look?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog:
> 2012-10-25  Dehao Chen  <dehao@google.com>
>
>         * tree-eh.c (do_return_redirection): Set location for jump statement.
>         (do_goto_redirection): Likewise.
>         (frob_into_branch_around): Likewise.
>         (lower_try_finally_nofallthru): Likewise.
>         (lower_try_finally_copy): Likewise.
>         (lower_try_finally_switch): Likewise.
>         * gimple-low.c (tree_set_block_r): New callback function.
>         (lower_stmt): Set block for tested expr.
>
> gcc/testsuite/ChangeLog:
> 2012-10-25  Dehao Chen  <dehao@google.com>
>
>         * g++.dg/debug/dwarf2/block.C: New testcase.
Dehao Chen - Oct. 27, 2012, 6:22 p.m.
On Sat, Oct 27, 2012 at 11:07 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Sat, Oct 27, 2012 at 12:53 AM, Dehao Chen <dehao@google.com> wrote:
>> Hi,
>>
>> I've updated the patch:
>>
>> 1. abandon the changes in cfgexpand.c
>> 2. set the block for trees when lowering gimple stmt.
>> 3. add a unittest.
>
> Index: gcc/gimple-low.c
> ===================================================================
> --- gcc/gimple-low.c    (revision 192809)
> +++ gcc/gimple-low.c    (working copy)
> @@ -331,7 +331,18 @@ lower_omp_directive (gimple_stmt_iterator *gsi, st
>    gsi_next (gsi);
>  }
>
> +/* Call back function to set the block for expr.  */
>
> +static tree
> +tree_set_block_r (tree *tp, int *walk_subtrees ATTRIBUTE_UNUSED,
> +                     void *data)
> +{
> +  tree block = (tree) data;
> +  if (CAN_HAVE_LOCATION_P (*tp))
> +    TREE_SET_BLOCK (*tp, block);
> +  return NULL_TREE;
> +}
> +
>  /* Lower statement GSI.  DATA is passed through the recursion.  We try to
>     track the fallthruness of statements and get rid of unreachable return
>     statements in order to prevent the EH lowering pass from adding useless
> @@ -343,8 +354,11 @@ static void
>  lower_stmt (gimple_stmt_iterator *gsi, struct lower_data *data)
>  {
>    gimple stmt = gsi_stmt (*gsi);
> +  unsigned i;
>
>    gimple_set_block (stmt, data->block);
> +  for (i = 0; i < gimple_num_ops (stmt); i++)
> +    walk_tree (gimple_op_ptr (stmt, i), tree_set_block_r, data->block, NULL);
>
>    switch (gimple_code (stmt))
>      {
>
>
> why do you need this?  The stmt location is taken from the operands and in fact
> they may have more precise locations.  So it seems completely pointless to me
> (wasting location entries where a NULL block is just fine (take the block from
> the stmt)).
>
> So, what is the issue you try to fix?  Yes, the stmt operands will
> keep the BLOCK
> live, but it is live if the frontend assigned it.

The issue I was trying to fix is that when expanding to rtl, the
operands's location is still gonna be used by set_curr_insn_location.
Thus we need to set the block info when we update the block info for
stmt. The unittest I added will have this problem.

Thanks,
Dehao

>
> Richard.
>
>> However, this patch will trigger two lto bug when asserting
>> LTO_NO_PREVAIL for TREE_CHAIN. After debugging for a while, I found
>> that the problem was also there even without the patch. This patch
>> just reveal the problem by moving a decl into cache so that it will be
>> checked. As I'm not familiar with LTO, not quite sure what the root
>> problem is. Can anyone help take a look?
>>
>> Thanks,
>> Dehao
>>
>> gcc/ChangeLog:
>> 2012-10-25  Dehao Chen  <dehao@google.com>
>>
>>         * tree-eh.c (do_return_redirection): Set location for jump statement.
>>         (do_goto_redirection): Likewise.
>>         (frob_into_branch_around): Likewise.
>>         (lower_try_finally_nofallthru): Likewise.
>>         (lower_try_finally_copy): Likewise.
>>         (lower_try_finally_switch): Likewise.
>>         * gimple-low.c (tree_set_block_r): New callback function.
>>         (lower_stmt): Set block for tested expr.
>>
>> gcc/testsuite/ChangeLog:
>> 2012-10-25  Dehao Chen  <dehao@google.com>
>>
>>         * g++.dg/debug/dwarf2/block.C: New testcase.

Patch

Index: gcc/gimple-low.c
===================================================================
--- gcc/gimple-low.c	(revision 192809)
+++ gcc/gimple-low.c	(working copy)
@@ -331,7 +331,18 @@  lower_omp_directive (gimple_stmt_iterator *gsi, st
   gsi_next (gsi);
 }

+/* Call back function to set the block for expr.  */

+static tree
+tree_set_block_r (tree *tp, int *walk_subtrees ATTRIBUTE_UNUSED,
+                     void *data)
+{
+  tree block = (tree) data;
+  if (CAN_HAVE_LOCATION_P (*tp))
+    TREE_SET_BLOCK (*tp, block);
+  return NULL_TREE;
+}
+
 /* Lower statement GSI.  DATA is passed through the recursion.  We try to
    track the fallthruness of statements and get rid of unreachable return
    statements in order to prevent the EH lowering pass from adding useless
@@ -343,8 +354,11 @@  static void
 lower_stmt (gimple_stmt_iterator *gsi, struct lower_data *data)
 {
   gimple stmt = gsi_stmt (*gsi);
+  unsigned i;

   gimple_set_block (stmt, data->block);
+  for (i = 0; i < gimple_num_ops (stmt); i++)
+    walk_tree (gimple_op_ptr (stmt, i), tree_set_block_r, data->block, NULL);

   switch (gimple_code (stmt))
     {