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

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, Aug 7, 2012 at 12:07 AM, Dehao Chen <dehao@google.com> wrote:
> 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));

given this use it would be nicer if you'd record a location in the queue instead
of a gimple-code.  Also as both lower_eh_constructs_1 and
lower_try_finally_dup_block already walk over all stmts it would be
nice to avoid
the extra walk ... especially as it looks like that other callers of
lower_try_finally_dup_block may also be affected (is re-setting
_every_ stmt location
really ok in all cases?).  So it feels like
lower_try_finally_dup_block should be
the function to re-set the locations.

Other than the above I don't know the code very well.

Thanks,
Richard.

>>            gimple_seq_add_seq (&new_stmt, seq);
>>
>>            gimple_seq_add_stmt (&new_stmt, q->cont_stmt);