Re: [PATCH] Set correct source location for deallocator calls

[Dehao Chen <dehao at google dot com>]

Ping...

Richard, could you shed some lights on this?

Thanks,
Dehao

On Mon, Jul 30, 2012 at 8:29 PM, Dehao Chen <dehao@google.com> wrote:
> Hi,
>
> This patch fixes the source location for automatically generated calls
> to deallocator. For example:
>
>  19 void foo(int i)
>  20 {
>  21   for (int j = 0; j < 10; j++)
>  22     {
>  23       t test;
>  24       test.foo();
>  25       if (i + j)
>  26         {
>  27           test.bar();
>  28           return;
>  29         }
>  30     }
>  31   return;
>  32 }
>
> The deallocator for "23  t test" is called in two places: Line 28 and
> line 30. However, gcc attributes both callsites to line 30.
>
> Bootstrapped and passed gcc regression tests.
>
> Is it ok for trunk?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog
>
> 2012-07-31  Dehao Chen  <dehao@google.com>
>
>         * tree-eh.c (goto_queue_node): New field.
>         (record_in_goto_queue): New parameter.
>         (record_in_goto_queue_label): New parameter.
>         (lower_try_finally_copy): Update source location.
>
> gcc/testsuite/ChangeLog
>
> 2012-07-31  Dehao Chen  <dehao@google.com>
>
>         * g++.dg/guality/deallocator.C: New test.
>
> Index: gcc/testsuite/g++.dg/guality/deallocator.C
> ===================================================================
> --- gcc/testsuite/g++.dg/guality/deallocator.C  (revision 0)
> +++ gcc/testsuite/g++.dg/guality/deallocator.C  (revision 0)
> @@ -0,0 +1,33 @@
> +// Test that debug info generated for auto-inserted deallocator is
> +// correctly attributed.
> +// This patch scans for the lineno directly from assembly, which may
> +// differ between different architectures. Because it mainly tests
> +// FE generated debug info, without losing generality, only x86
> +// assembly is scanned in this test.
> +// { dg-do compile { target { i?86-*-* x86_64-*-* } } }
> +// { dg-options "-O2 -fno-exceptions -g" }
> +
> +struct t {
> +  t ();
> +  ~t ();
> +  void foo();
> +  void bar();
> +};
> +
> +int bar();
> +
> +void foo(int i)
> +{
> +  for (int j = 0; j < 10; j++)
> +    {
> +      t test;
> +      test.foo();
> +      if (i + j)
> +       {
> +         test.bar();
> +         return;
> +       }
> +    }
> +  return;
> +}
> +// { dg-final { scan-assembler "1 28 0" } }
> Index: gcc/tree-eh.c
> ===================================================================
> --- gcc/tree-eh.c       (revision 189835)
> +++ gcc/tree-eh.c       (working copy)
> @@ -321,6 +321,7 @@
>  struct goto_queue_node
>  {
>    treemple stmt;
> +  enum gimple_code code;
>    gimple_seq repl_stmt;
>    gimple cont_stmt;
>    int index;
> @@ -560,7 +561,8 @@
>  record_in_goto_queue (struct leh_tf_state *tf,
>                        treemple new_stmt,
>                        int index,
> -                      bool is_label)
> +                      bool is_label,
> +                     enum gimple_code code)
>  {
>    size_t active, size;
>    struct goto_queue_node *q;
> @@ -583,6 +585,7 @@
>    memset (q, 0, sizeof (*q));
>    q->stmt = new_stmt;
>    q->index = index;
> +  q->code = code;
>    q->is_label = is_label;
>  }
>
> @@ -590,7 +593,8 @@
>     TF is not null.  */
>
>  static void
> -record_in_goto_queue_label (struct leh_tf_state *tf, treemple stmt, tree label)
> +record_in_goto_queue_label (struct leh_tf_state *tf, treemple stmt, tree label,
> +                           enum gimple_code code)
>  {
>    int index;
>    treemple temp, new_stmt;
> @@ -629,7 +633,7 @@
>       since with a GIMPLE_COND we have an easy access to the then/else
>       labels. */
>    new_stmt = stmt;
> -  record_in_goto_queue (tf, new_stmt, index, true);
> +  record_in_goto_queue (tf, new_stmt, index, true, code);
>  }
>
>  /* For any GIMPLE_GOTO or GIMPLE_RETURN, decide whether it leaves a try_finally
> @@ -649,19 +653,22 @@
>      {
>      case GIMPLE_COND:
>        new_stmt.tp = gimple_op_ptr (stmt, 2);
> -      record_in_goto_queue_label (tf, new_stmt, gimple_cond_true_label (stmt));
> +      record_in_goto_queue_label (tf, new_stmt, gimple_cond_true_label (stmt),
> +                                 gimple_code (stmt));
>        new_stmt.tp = gimple_op_ptr (stmt, 3);
> -      record_in_goto_queue_label (tf, new_stmt,
> gimple_cond_false_label (stmt));
> +      record_in_goto_queue_label (tf, new_stmt, gimple_cond_false_label (stmt),
> +                                 gimple_code (stmt));
>        break;
>      case GIMPLE_GOTO:
>        new_stmt.g = stmt;
> -      record_in_goto_queue_label (tf, new_stmt, gimple_goto_dest (stmt));
> +      record_in_goto_queue_label (tf, new_stmt, gimple_goto_dest (stmt),
> +                                 gimple_code (stmt));
>        break;
>
>      case GIMPLE_RETURN:
>        tf->may_return = true;
>        new_stmt.g = stmt;
> -      record_in_goto_queue (tf, new_stmt, -1, false);
> +      record_in_goto_queue (tf, new_stmt, -1, false, gimple_code (stmt));
>        break;
>
>      default:
> @@ -1234,6 +1241,7 @@
>        for (index = 0; index < return_index + 1; index++)
>         {
>           tree lab;
> +         gimple_stmt_iterator gsi;
>
>           q = labels[index].q;
>           if (! q)
> @@ -1252,6 +1260,11 @@
>
>           seq = lower_try_finally_dup_block (finally, state);
>           lower_eh_constructs_1 (state, &seq);
> +         for (gsi = gsi_start (seq); !gsi_end_p (gsi); gsi_next (&gsi))
> +           gimple_set_location (gsi_stmt (gsi),
> +                                q->code == GIMPLE_COND ?
> +                                    EXPR_LOCATION (*q->stmt.tp) :
> +                                    gimple_location (q->stmt.g));
>            gimple_seq_add_seq (&new_stmt, seq);
>
>            gimple_seq_add_stmt (&new_stmt, q->cont_stmt);