Patchwork Set correct source location for deallocator calls

login
register
mail settings
Submitter Dehao Chen
Date July 31, 2012, 3:29 a.m.
Message ID <CAO2gOZXfnETUe4wqjT7p6fd61hXreu9PDfqKxNz+HxpE0E7K0g@mail.gmail.com>
Download mbox | patch
Permalink /patch/174138/
State New
Headers show

Comments

Dehao Chen - July 31, 2012, 3:29 a.m.
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.

     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);
Dehao Chen - Aug. 6, 2012, 10:07 p.m.
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);
Richard Guenther - Aug. 7, 2012, 1:25 p.m.
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);
Richard Henderson - Aug. 8, 2012, 3:56 p.m.
On 08/07/2012 06:25 AM, Richard Guenther wrote:
> (is re-setting _every_ stmt location really ok in all cases?)

I'm certain that it's not, though you can't tell that from C++.

Examine instead a Java test case using try-finally.  In Java the
contents of the finally would be incorrectly relocated from their
original source line to the new line Dehao has decided upon.

I can see the desire for wanting the call to ~t() to appear from 
the return statement.  And for C++ we'll get the correct lines for
the contents of ~t() post inlining (which happens after tree-eh).

But unless the C++ front end uses something like UNKNOWN_LOCATION
on the destructor call, I don't see how we can tell the Java and
C++ cases apart.  And if we can't, then I don't think we can make
this change at all.


r~
Dehao Chen - Aug. 8, 2012, 4:27 p.m.
On Wed, Aug 8, 2012 at 8:56 AM, Richard Henderson <rth@redhat.com> wrote:
> On 08/07/2012 06:25 AM, Richard Guenther wrote:
>> (is re-setting _every_ stmt location really ok in all cases?)
>
> I'm certain that it's not, though you can't tell that from C++.
>
> Examine instead a Java test case using try-finally.  In Java the
> contents of the finally would be incorrectly relocated from their
> original source line to the new line Dehao has decided upon.

This makes sense. I was thinking what else can reside in the finally
block, and apparently I was ignoring Java...

>
> I can see the desire for wanting the call to ~t() to appear from
> the return statement.  And for C++ we'll get the correct lines for
> the contents of ~t() post inlining (which happens after tree-eh).

Even if inline gives it right source position, it'll still have
incorrect inline stack.

>
> But unless the C++ front end uses something like UNKNOWN_LOCATION
> on the destructor call, I don't see how we can tell the Java and
> C++ cases apart.  And if we can't, then I don't think we can make
> this change at all.

Then we should probably assign UNKNOWN_LOCATION for these destructor
calls, what do you guys think?

Thanks,
Dehao

>
>
> r~
Richard Henderson - Aug. 8, 2012, 4:32 p.m.
On 08/08/2012 09:27 AM, Dehao Chen wrote:
> On Wed, Aug 8, 2012 at 8:56 AM, Richard Henderson <rth@redhat.com> wrote:
>> On 08/07/2012 06:25 AM, Richard Guenther wrote:
>>> (is re-setting _every_ stmt location really ok in all cases?)
>>
>> I'm certain that it's not, though you can't tell that from C++.
>>
>> Examine instead a Java test case using try-finally.  In Java the
>> contents of the finally would be incorrectly relocated from their
>> original source line to the new line Dehao has decided upon.
> 
> This makes sense. I was thinking what else can reside in the finally
> block, and apparently I was ignoring Java...
> 
>>
>> I can see the desire for wanting the call to ~t() to appear from
>> the return statement.  And for C++ we'll get the correct lines for
>> the contents of ~t() post inlining (which happens after tree-eh).
> 
> Even if inline gives it right source position, it'll still have
> incorrect inline stack.
> 
>>
>> But unless the C++ front end uses something like UNKNOWN_LOCATION
>> on the destructor call, I don't see how we can tell the Java and
>> C++ cases apart.  And if we can't, then I don't think we can make
>> this change at all.
> 
> Then we should probably assign UNKNOWN_LOCATION for these destructor
> calls, what do you guys think?

I think it's certainly plausible.  I can't think what other problems
such a change would cause.  Jason?


r~
Jason Merrill - Aug. 9, 2012, 10:06 p.m.
On 08/08/2012 12:32 PM, Richard Henderson wrote:
> On 08/08/2012 09:27 AM, Dehao Chen wrote:
>> Then we should probably assign UNKNOWN_LOCATION for these destructor
>> calls, what do you guys think?
>
> I think it's certainly plausible.  I can't think what other problems
> such a change would cause.  Jason?

cxx_maybe_build_cleanup is already trying to do that.  If it's missing 
some cases then yes, let's fix them too.

Jason
Dehao Chen - Aug. 10, 2012, 5:21 p.m.
On Thu, Aug 9, 2012 at 3:06 PM, Jason Merrill <jason@redhat.com> wrote:
> On 08/08/2012 12:32 PM, Richard Henderson wrote:
>>
>> On 08/08/2012 09:27 AM, Dehao Chen wrote:
>>>
>>> Then we should probably assign UNKNOWN_LOCATION for these destructor
>>> calls, what do you guys think?
>>
>>
>> I think it's certainly plausible.  I can't think what other problems
>> such a change would cause.  Jason?
>
>
> cxx_maybe_build_cleanup is already trying to do that.  If it's missing some
> cases then yes, let's fix them too.

Yes, cxx_maybe_build_cleanup sets it to UNKNOWN_LOCATION, but during
gimplifying, it's reset to input_location:

gimplify.c (gimplify_call_expr)
2486      /* For reliable diagnostics during inlining, it is necessary that
2487         every call_expr be annotated with file and line.  */
2488      if (! EXPR_HAS_LOCATION (*expr_p))
2489        SET_EXPR_LOCATION (*expr_p, input_location);

Shall we remove this code? Because I don't expect the location to be
unknown in other cases.

Thanks,
Dehao

>
> Jason
>
Richard Henderson - Aug. 10, 2012, 5:52 p.m.
On 2012-08-10 10:21, Dehao Chen wrote:
> Yes, cxx_maybe_build_cleanup sets it to UNKNOWN_LOCATION, but during
> gimplifying, it's reset to input_location:
> 
> gimplify.c (gimplify_call_expr)
> 2486      /* For reliable diagnostics during inlining, it is necessary that
> 2487         every call_expr be annotated with file and line.  */
> 2488      if (! EXPR_HAS_LOCATION (*expr_p))
> 2489        SET_EXPR_LOCATION (*expr_p, input_location);
> 
> Shall we remove this code? Because I don't expect the location to be
> unknown in other cases.

Hmm.  Perhaps something special-cased to TRY_FINALLY/TRY_CATCH
to set input_location itself to UNKNOWN_LOCATION  for the duration
of gimplifying the cleanup?  With a big comment about how we'll be
setting unset locations for cleanups during tree-eh lowering.


r~
Dehao Chen - Aug. 10, 2012, 6:01 p.m.
On Fri, Aug 10, 2012 at 10:52 AM, Richard Henderson <rth@redhat.com> wrote:
> On 2012-08-10 10:21, Dehao Chen wrote:
>> Yes, cxx_maybe_build_cleanup sets it to UNKNOWN_LOCATION, but during
>> gimplifying, it's reset to input_location:
>>
>> gimplify.c (gimplify_call_expr)
>> 2486      /* For reliable diagnostics during inlining, it is necessary that
>> 2487         every call_expr be annotated with file and line.  */
>> 2488      if (! EXPR_HAS_LOCATION (*expr_p))
>> 2489        SET_EXPR_LOCATION (*expr_p, input_location);
>>
>> Shall we remove this code? Because I don't expect the location to be
>> unknown in other cases.
>
> Hmm.  Perhaps something special-cased to TRY_FINALLY/TRY_CATCH
> to set input_location itself to UNKNOWN_LOCATION  for the duration
> of gimplifying the cleanup?  With a big comment about how we'll be
> setting unset locations for cleanups during tree-eh lowering.

Handling TRY_FINALLY/TRY_CATCH case may not work for Java, as it also
shares gimplify.c

Or, shall we create a routine in cp frontend to let gimplify.c know
that a call_expr is auto-generated dtor, so that gimplify will not set
its location to input_location?

Thanks,
Dehao

>
>
> r~
Richard Henderson - Aug. 10, 2012, 7:04 p.m.
On 2012-08-10 11:01, Dehao Chen wrote:
> On Fri, Aug 10, 2012 at 10:52 AM, Richard Henderson <rth@redhat.com> wrote:
>> On 2012-08-10 10:21, Dehao Chen wrote:
>>> Yes, cxx_maybe_build_cleanup sets it to UNKNOWN_LOCATION, but during
>>> gimplifying, it's reset to input_location:
>>>
>>> gimplify.c (gimplify_call_expr)
>>> 2486      /* For reliable diagnostics during inlining, it is necessary that
>>> 2487         every call_expr be annotated with file and line.  */
>>> 2488      if (! EXPR_HAS_LOCATION (*expr_p))
>>> 2489        SET_EXPR_LOCATION (*expr_p, input_location);
>>>
>>> Shall we remove this code? Because I don't expect the location to be
>>> unknown in other cases.
>>
>> Hmm.  Perhaps something special-cased to TRY_FINALLY/TRY_CATCH
>> to set input_location itself to UNKNOWN_LOCATION  for the duration
>> of gimplifying the cleanup?  With a big comment about how we'll be
>> setting unset locations for cleanups during tree-eh lowering.
> 
> Handling TRY_FINALLY/TRY_CATCH case may not work for Java, as it also
> shares gimplify.c

For Java, the theory is that the EXPR_LOCATION of the statements in the
catch should already be set by the front end.  So the !EXPR_HAS_LOCATION
bit there would not fire, so we'll not "reset" the location to UNKNOWN.

Then in tree-eh, you would similarly only set the location of gimple
stmts that have UNKNOWN_LOCATION.


r~
Jason Merrill - Aug. 10, 2012, 8:12 p.m.
On 08/10/2012 01:21 PM, Dehao Chen wrote:
> gimplify.c (gimplify_call_expr)
> 2486      /* For reliable diagnostics during inlining, it is necessary that
> 2487         every call_expr be annotated with file and line.  */
> 2488      if (! EXPR_HAS_LOCATION (*expr_p))
> 2489        SET_EXPR_LOCATION (*expr_p, input_location);
>
> Shall we remove this code? Because I don't expect the location to be
> unknown in other cases.

The code above is unnecessary for C++, because build_cxx_call already 
sets the location on CALL_EXPRs:

>   /* Remember roughly where this call is.  */
>   location_t loc = EXPR_LOC_OR_HERE (fn);
>   fn = build_call_a (fn, nargs, argarray);
>   SET_EXPR_LOCATION (fn, loc);

I don't know about other front ends.

Jason
Dehao Chen - Aug. 10, 2012, 9:55 p.m.
On Fri, Aug 10, 2012 at 12:04 PM, Richard Henderson <rth@redhat.com> wrote:
> On 2012-08-10 11:01, Dehao Chen wrote:
>> On Fri, Aug 10, 2012 at 10:52 AM, Richard Henderson <rth@redhat.com> wrote:
>>> On 2012-08-10 10:21, Dehao Chen wrote:
>>>> Yes, cxx_maybe_build_cleanup sets it to UNKNOWN_LOCATION, but during
>>>> gimplifying, it's reset to input_location:
>>>>
>>>> gimplify.c (gimplify_call_expr)
>>>> 2486      /* For reliable diagnostics during inlining, it is necessary that
>>>> 2487         every call_expr be annotated with file and line.  */
>>>> 2488      if (! EXPR_HAS_LOCATION (*expr_p))
>>>> 2489        SET_EXPR_LOCATION (*expr_p, input_location);
>>>>
>>>> Shall we remove this code? Because I don't expect the location to be
>>>> unknown in other cases.
>>>
>>> Hmm.  Perhaps something special-cased to TRY_FINALLY/TRY_CATCH
>>> to set input_location itself to UNKNOWN_LOCATION  for the duration
>>> of gimplifying the cleanup?  With a big comment about how we'll be
>>> setting unset locations for cleanups during tree-eh lowering.
>>
>> Handling TRY_FINALLY/TRY_CATCH case may not work for Java, as it also
>> shares gimplify.c
>
> For Java, the theory is that the EXPR_LOCATION of the statements in the
> catch should already be set by the front end.  So the !EXPR_HAS_LOCATION
> bit there would not fire, so we'll not "reset" the location to UNKNOWN.

I see your point. But the problem is that the above code is in
gimplify_call_expr, in which we have no idea if it is in a
TRY_FINALLY/TRY_CATCH block. However, in the following code that
handles TRY_FINALLY/TRY_CATCH, the EXPR_LOCATION is already set by the
above code to input_location, thus we cannot know if it's from Java or
C++...

gimplify.c (gimplify_expr)
....
7431            case TRY_FINALLY_EXPR:
7432            case TRY_CATCH_EXPR:
7433              {
......

Thanks,
Dehao

>
> Then in tree-eh, you would similarly only set the location of gimple
> stmts that have UNKNOWN_LOCATION.
>
>
> r~
Richard Henderson - Aug. 10, 2012, 10:11 p.m.
On 2012-08-10 14:55, Dehao Chen wrote:
> I see your point. But the problem is that the above code is in
> gimplify_call_expr, in which we have no idea if it is in a
> TRY_FINALLY/TRY_CATCH block.

That's why I suggested saving and restoring input_location
while gimplifying try_finally.

i.e.

	location_t save_loc = input_location;
	input_location = UNKNOWN_LOCATION;

	gimplify_and_add (TREE_OPERAND (*expr_p, 1), &cleanup);

	input_location = save_loc;

You may not even need the save_loc, since gimplify_expr already
has an outer saved_location.


r~
Dehao Chen - Aug. 10, 2012, 10:23 p.m.
On Fri, Aug 10, 2012 at 3:11 PM, Richard Henderson <rth@redhat.com> wrote:
> On 2012-08-10 14:55, Dehao Chen wrote:
>> I see your point. But the problem is that the above code is in
>> gimplify_call_expr, in which we have no idea if it is in a
>> TRY_FINALLY/TRY_CATCH block.
>
> That's why I suggested saving and restoring input_location
> while gimplifying try_finally.
>
> i.e.
>
>         location_t save_loc = input_location;
>         input_location = UNKNOWN_LOCATION;
>
>         gimplify_and_add (TREE_OPERAND (*expr_p, 1), &cleanup);
>
>         input_location = save_loc;
>
> You may not even need the save_loc, since gimplify_expr already
> has an outer saved_location.

Emm, that's clever.

Thanks,
Dehao

>
>
> r~
Dehao Chen - Aug. 11, 2012, 3:38 a.m.
New patch attached.

Bootstrapped and passed GCC regression tests.

Ok for trunk?

Thanks,
Dehao

gcc/ChangeLog
2012-08-07  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_dup_block): New parameter.
 	(maybe_record_in_goto_queue): Update source location.
 	(lower_try_finally_copy): Likewise.
 	(honor_protect_cleanup_actions): Likewise.
 	* gimplify.c (gimplify_expr): Reset the location to unknown.

gcc/testsuite/ChangeLog
2012-08-07  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 ****
--- 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 190209)
--- gcc/tree-eh.c	(working copy)
*************** static bitmap eh_region_may_contain_thro
*** 321,326 ****
--- 321,327 ----
  struct goto_queue_node
  {
    treemple stmt;
+   location_t location;
    gimple_seq repl_stmt;
    gimple cont_stmt;
    int index;
*************** static void
*** 560,566 ****
  record_in_goto_queue (struct leh_tf_state *tf,
                        treemple new_stmt,
                        int index,
!                       bool is_label)
  {
    size_t active, size;
    struct goto_queue_node *q;
--- 561,568 ----
  record_in_goto_queue (struct leh_tf_state *tf,
                        treemple new_stmt,
                        int index,
!                       bool is_label,
! 		      location_t location)
  {
    size_t active, size;
    struct goto_queue_node *q;
*************** record_in_goto_queue (struct leh_tf_stat
*** 583,588 ****
--- 585,591 ----
    memset (q, 0, sizeof (*q));
    q->stmt = new_stmt;
    q->index = index;
+   q->location = location;
    q->is_label = is_label;
  }

*************** record_in_goto_queue (struct leh_tf_stat
*** 590,596 ****
     TF is not null.  */

  static void
! record_in_goto_queue_label (struct leh_tf_state *tf, treemple stmt,
tree label)
  {
    int index;
    treemple temp, new_stmt;
--- 593,600 ----
     TF is not null.  */

  static void
! record_in_goto_queue_label (struct leh_tf_state *tf, treemple stmt,
tree label,
! 			    location_t location)
  {
    int index;
    treemple temp, new_stmt;
*************** record_in_goto_queue_label (struct leh_t
*** 629,635 ****
       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);
  }

  /* For any GIMPLE_GOTO or GIMPLE_RETURN, decide whether it leaves a
try_finally
--- 633,639 ----
       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, location);
  }

  /* For any GIMPLE_GOTO or GIMPLE_RETURN, decide whether it leaves a
try_finally
*************** maybe_record_in_goto_queue (struct leh_s
*** 649,667 ****
      {
      case GIMPLE_COND:
        new_stmt.tp = gimple_op_ptr (stmt, 2);
!       record_in_goto_queue_label (tf, new_stmt,
gimple_cond_true_label (stmt));
        new_stmt.tp = gimple_op_ptr (stmt, 3);
!       record_in_goto_queue_label (tf, new_stmt,
gimple_cond_false_label (stmt));
        break;
      case GIMPLE_GOTO:
        new_stmt.g = stmt;
!       record_in_goto_queue_label (tf, new_stmt, gimple_goto_dest (stmt));
        break;

      case GIMPLE_RETURN:
        tf->may_return = true;
        new_stmt.g = stmt;
!       record_in_goto_queue (tf, new_stmt, -1, false);
        break;

      default:
--- 653,674 ----
      {
      case GIMPLE_COND:
        new_stmt.tp = gimple_op_ptr (stmt, 2);
!       record_in_goto_queue_label (tf, new_stmt, gimple_cond_true_label (stmt),
! 				  EXPR_LOCATION (*new_stmt.tp));
        new_stmt.tp = gimple_op_ptr (stmt, 3);
!       record_in_goto_queue_label (tf, new_stmt,
gimple_cond_false_label (stmt),
! 				  EXPR_LOCATION (*new_stmt.tp));
        break;
      case GIMPLE_GOTO:
        new_stmt.g = stmt;
!       record_in_goto_queue_label (tf, new_stmt, gimple_goto_dest (stmt),
! 				  gimple_location (stmt));
        break;

      case GIMPLE_RETURN:
        tf->may_return = true;
        new_stmt.g = stmt;
!       record_in_goto_queue (tf, new_stmt, -1, false, gimple_location (stmt));
        break;

      default:
*************** frob_into_branch_around (gimple tp, eh_r
*** 866,878 ****
     Make sure to record all new labels found.  */

  static gimple_seq
! lower_try_finally_dup_block (gimple_seq seq, struct leh_state *outer_state)
  {
    gimple region = NULL;
    gimple_seq new_seq;

    new_seq = copy_gimple_seq_and_replace_locals (seq);

    if (outer_state->tf)
      region = outer_state->tf->try_finally_expr;
    collect_finally_tree_1 (new_seq, region);
--- 873,891 ----
     Make sure to record all new labels found.  */

  static gimple_seq
! lower_try_finally_dup_block (gimple_seq seq, struct leh_state *outer_state,
! 			     location_t loc)
  {
    gimple region = NULL;
    gimple_seq new_seq;
+   gimple_stmt_iterator gsi;

    new_seq = copy_gimple_seq_and_replace_locals (seq);

+   for (gsi = gsi_start (new_seq); !gsi_end_p (gsi); gsi_next (&gsi))
+     if (gimple_location (gsi_stmt (gsi)) == UNKNOWN_LOCATION)
+       gimple_set_location (gsi_stmt (gsi), loc);
+
    if (outer_state->tf)
      region = outer_state->tf->try_finally_expr;
    collect_finally_tree_1 (new_seq, region);
*************** honor_protect_cleanup_actions (struct le
*** 967,973 ****
        gimple_try_set_cleanup (tf->top_p, gimple_eh_else_n_body (eh_else));
      }
    else if (this_state)
!     finally = lower_try_finally_dup_block (finally, outer_state);
    finally_may_fallthru = gimple_seq_may_fallthru (finally);

    /* If this cleanup consists of a TRY_CATCH_EXPR with TRY_CATCH_IS_CLEANUP
--- 980,987 ----
        gimple_try_set_cleanup (tf->top_p, gimple_eh_else_n_body (eh_else));
      }
    else if (this_state)
!     finally = lower_try_finally_dup_block (finally, outer_state,
! 					   UNKNOWN_LOCATION);
    finally_may_fallthru = gimple_seq_may_fallthru (finally);

    /* If this cleanup consists of a TRY_CATCH_EXPR with TRY_CATCH_IS_CLEANUP
*************** lower_try_finally_copy (struct leh_state
*** 1184,1190 ****

    if (tf->may_fallthru)
      {
!       seq = lower_try_finally_dup_block (finally, state);
        lower_eh_constructs_1 (state, &seq);
        gimple_seq_add_seq (&new_stmt, seq);

--- 1198,1204 ----

    if (tf->may_fallthru)
      {
!       seq = lower_try_finally_dup_block (finally, state, tf_loc);
        lower_eh_constructs_1 (state, &seq);
        gimple_seq_add_seq (&new_stmt, seq);

*************** lower_try_finally_copy (struct leh_state
*** 1200,1206 ****
        if (eh_else)
  	seq = gimple_eh_else_e_body (eh_else);
        else
! 	seq = lower_try_finally_dup_block (finally, state);
        lower_eh_constructs_1 (state, &seq);

        emit_post_landing_pad (&eh_seq, tf->region);
--- 1214,1220 ----
        if (eh_else)
  	seq = gimple_eh_else_e_body (eh_else);
        else
! 	seq = lower_try_finally_dup_block (finally, state, tf_loc);
        lower_eh_constructs_1 (state, &seq);

        emit_post_landing_pad (&eh_seq, tf->region);
*************** lower_try_finally_copy (struct leh_state
*** 1250,1256 ****
  	  x = gimple_build_label (lab);
            gimple_seq_add_stmt (&new_stmt, x);

! 	  seq = lower_try_finally_dup_block (finally, state);
  	  lower_eh_constructs_1 (state, &seq);
            gimple_seq_add_seq (&new_stmt, seq);

--- 1264,1270 ----
  	  x = gimple_build_label (lab);
            gimple_seq_add_stmt (&new_stmt, x);

! 	  seq = lower_try_finally_dup_block (finally, state, q->location);
  	  lower_eh_constructs_1 (state, &seq);
            gimple_seq_add_seq (&new_stmt, seq);

Index: gcc/gimplify.c
===================================================================
*** gcc/gimplify.c	(revision 190209)
--- gcc/gimplify.c	(working copy)
*************** gimplify_expr (tree *expr_p, gimple_seq
*** 7434,7439 ****
--- 7434,7447 ----
  	    gimple_seq eval, cleanup;
  	    gimple try_;

+ 	    /* For call expressions inside FINALL/CATCH block, if its location
+ 	       is unknown, gimplify_call_expr will set it to input_location.
+ 	       However, these calls are automatically generated to destructors.
+ 	       And they may be cloned to many places. In this case, we will
+ 	       set the location for them in tree-eh.c. But to ensure that EH
+ 	       does the right job, we first need mark their location as
+ 	       UNKNOWN_LOCATION.  */
+ 	    input_location = UNKNOWN_LOCATION;
  	    eval = cleanup = NULL;
  	    gimplify_and_add (TREE_OPERAND (*expr_p, 0), &eval);
  	    gimplify_and_add (TREE_OPERAND (*expr_p, 1), &cleanup);
Dehao Chen - Aug. 16, 2012, 10:34 p.m.
ping.....

Thanks,
Dehao

On Fri, Aug 10, 2012 at 8:38 PM, Dehao Chen <dehao@google.com> wrote:
> New patch attached.
>
> Bootstrapped and passed GCC regression tests.
>
> Ok for trunk?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog
> 2012-08-07  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_dup_block): New parameter.
>         (maybe_record_in_goto_queue): Update source location.
>         (lower_try_finally_copy): Likewise.
>         (honor_protect_cleanup_actions): Likewise.
>         * gimplify.c (gimplify_expr): Reset the location to unknown.
>
> gcc/testsuite/ChangeLog
> 2012-08-07  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 ****
> --- 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 190209)
> --- gcc/tree-eh.c       (working copy)
> *************** static bitmap eh_region_may_contain_thro
> *** 321,326 ****
> --- 321,327 ----
>   struct goto_queue_node
>   {
>     treemple stmt;
> +   location_t location;
>     gimple_seq repl_stmt;
>     gimple cont_stmt;
>     int index;
> *************** static void
> *** 560,566 ****
>   record_in_goto_queue (struct leh_tf_state *tf,
>                         treemple new_stmt,
>                         int index,
> !                       bool is_label)
>   {
>     size_t active, size;
>     struct goto_queue_node *q;
> --- 561,568 ----
>   record_in_goto_queue (struct leh_tf_state *tf,
>                         treemple new_stmt,
>                         int index,
> !                       bool is_label,
> !                     location_t location)
>   {
>     size_t active, size;
>     struct goto_queue_node *q;
> *************** record_in_goto_queue (struct leh_tf_stat
> *** 583,588 ****
> --- 585,591 ----
>     memset (q, 0, sizeof (*q));
>     q->stmt = new_stmt;
>     q->index = index;
> +   q->location = location;
>     q->is_label = is_label;
>   }
>
> *************** record_in_goto_queue (struct leh_tf_stat
> *** 590,596 ****
>      TF is not null.  */
>
>   static void
> ! record_in_goto_queue_label (struct leh_tf_state *tf, treemple stmt,
> tree label)
>   {
>     int index;
>     treemple temp, new_stmt;
> --- 593,600 ----
>      TF is not null.  */
>
>   static void
> ! record_in_goto_queue_label (struct leh_tf_state *tf, treemple stmt,
> tree label,
> !                           location_t location)
>   {
>     int index;
>     treemple temp, new_stmt;
> *************** record_in_goto_queue_label (struct leh_t
> *** 629,635 ****
>        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);
>   }
>
>   /* For any GIMPLE_GOTO or GIMPLE_RETURN, decide whether it leaves a
> try_finally
> --- 633,639 ----
>        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, location);
>   }
>
>   /* For any GIMPLE_GOTO or GIMPLE_RETURN, decide whether it leaves a
> try_finally
> *************** maybe_record_in_goto_queue (struct leh_s
> *** 649,667 ****
>       {
>       case GIMPLE_COND:
>         new_stmt.tp = gimple_op_ptr (stmt, 2);
> !       record_in_goto_queue_label (tf, new_stmt,
> gimple_cond_true_label (stmt));
>         new_stmt.tp = gimple_op_ptr (stmt, 3);
> !       record_in_goto_queue_label (tf, new_stmt,
> gimple_cond_false_label (stmt));
>         break;
>       case GIMPLE_GOTO:
>         new_stmt.g = stmt;
> !       record_in_goto_queue_label (tf, new_stmt, gimple_goto_dest (stmt));
>         break;
>
>       case GIMPLE_RETURN:
>         tf->may_return = true;
>         new_stmt.g = stmt;
> !       record_in_goto_queue (tf, new_stmt, -1, false);
>         break;
>
>       default:
> --- 653,674 ----
>       {
>       case GIMPLE_COND:
>         new_stmt.tp = gimple_op_ptr (stmt, 2);
> !       record_in_goto_queue_label (tf, new_stmt, gimple_cond_true_label (stmt),
> !                                 EXPR_LOCATION (*new_stmt.tp));
>         new_stmt.tp = gimple_op_ptr (stmt, 3);
> !       record_in_goto_queue_label (tf, new_stmt,
> gimple_cond_false_label (stmt),
> !                                 EXPR_LOCATION (*new_stmt.tp));
>         break;
>       case GIMPLE_GOTO:
>         new_stmt.g = stmt;
> !       record_in_goto_queue_label (tf, new_stmt, gimple_goto_dest (stmt),
> !                                 gimple_location (stmt));
>         break;
>
>       case GIMPLE_RETURN:
>         tf->may_return = true;
>         new_stmt.g = stmt;
> !       record_in_goto_queue (tf, new_stmt, -1, false, gimple_location (stmt));
>         break;
>
>       default:
> *************** frob_into_branch_around (gimple tp, eh_r
> *** 866,878 ****
>      Make sure to record all new labels found.  */
>
>   static gimple_seq
> ! lower_try_finally_dup_block (gimple_seq seq, struct leh_state *outer_state)
>   {
>     gimple region = NULL;
>     gimple_seq new_seq;
>
>     new_seq = copy_gimple_seq_and_replace_locals (seq);
>
>     if (outer_state->tf)
>       region = outer_state->tf->try_finally_expr;
>     collect_finally_tree_1 (new_seq, region);
> --- 873,891 ----
>      Make sure to record all new labels found.  */
>
>   static gimple_seq
> ! lower_try_finally_dup_block (gimple_seq seq, struct leh_state *outer_state,
> !                            location_t loc)
>   {
>     gimple region = NULL;
>     gimple_seq new_seq;
> +   gimple_stmt_iterator gsi;
>
>     new_seq = copy_gimple_seq_and_replace_locals (seq);
>
> +   for (gsi = gsi_start (new_seq); !gsi_end_p (gsi); gsi_next (&gsi))
> +     if (gimple_location (gsi_stmt (gsi)) == UNKNOWN_LOCATION)
> +       gimple_set_location (gsi_stmt (gsi), loc);
> +
>     if (outer_state->tf)
>       region = outer_state->tf->try_finally_expr;
>     collect_finally_tree_1 (new_seq, region);
> *************** honor_protect_cleanup_actions (struct le
> *** 967,973 ****
>         gimple_try_set_cleanup (tf->top_p, gimple_eh_else_n_body (eh_else));
>       }
>     else if (this_state)
> !     finally = lower_try_finally_dup_block (finally, outer_state);
>     finally_may_fallthru = gimple_seq_may_fallthru (finally);
>
>     /* If this cleanup consists of a TRY_CATCH_EXPR with TRY_CATCH_IS_CLEANUP
> --- 980,987 ----
>         gimple_try_set_cleanup (tf->top_p, gimple_eh_else_n_body (eh_else));
>       }
>     else if (this_state)
> !     finally = lower_try_finally_dup_block (finally, outer_state,
> !                                          UNKNOWN_LOCATION);
>     finally_may_fallthru = gimple_seq_may_fallthru (finally);
>
>     /* If this cleanup consists of a TRY_CATCH_EXPR with TRY_CATCH_IS_CLEANUP
> *************** lower_try_finally_copy (struct leh_state
> *** 1184,1190 ****
>
>     if (tf->may_fallthru)
>       {
> !       seq = lower_try_finally_dup_block (finally, state);
>         lower_eh_constructs_1 (state, &seq);
>         gimple_seq_add_seq (&new_stmt, seq);
>
> --- 1198,1204 ----
>
>     if (tf->may_fallthru)
>       {
> !       seq = lower_try_finally_dup_block (finally, state, tf_loc);
>         lower_eh_constructs_1 (state, &seq);
>         gimple_seq_add_seq (&new_stmt, seq);
>
> *************** lower_try_finally_copy (struct leh_state
> *** 1200,1206 ****
>         if (eh_else)
>         seq = gimple_eh_else_e_body (eh_else);
>         else
> !       seq = lower_try_finally_dup_block (finally, state);
>         lower_eh_constructs_1 (state, &seq);
>
>         emit_post_landing_pad (&eh_seq, tf->region);
> --- 1214,1220 ----
>         if (eh_else)
>         seq = gimple_eh_else_e_body (eh_else);
>         else
> !       seq = lower_try_finally_dup_block (finally, state, tf_loc);
>         lower_eh_constructs_1 (state, &seq);
>
>         emit_post_landing_pad (&eh_seq, tf->region);
> *************** lower_try_finally_copy (struct leh_state
> *** 1250,1256 ****
>           x = gimple_build_label (lab);
>             gimple_seq_add_stmt (&new_stmt, x);
>
> !         seq = lower_try_finally_dup_block (finally, state);
>           lower_eh_constructs_1 (state, &seq);
>             gimple_seq_add_seq (&new_stmt, seq);
>
> --- 1264,1270 ----
>           x = gimple_build_label (lab);
>             gimple_seq_add_stmt (&new_stmt, x);
>
> !         seq = lower_try_finally_dup_block (finally, state, q->location);
>           lower_eh_constructs_1 (state, &seq);
>             gimple_seq_add_seq (&new_stmt, seq);
>
> Index: gcc/gimplify.c
> ===================================================================
> *** gcc/gimplify.c      (revision 190209)
> --- gcc/gimplify.c      (working copy)
> *************** gimplify_expr (tree *expr_p, gimple_seq
> *** 7434,7439 ****
> --- 7434,7447 ----
>             gimple_seq eval, cleanup;
>             gimple try_;
>
> +           /* For call expressions inside FINALL/CATCH block, if its location
> +              is unknown, gimplify_call_expr will set it to input_location.
> +              However, these calls are automatically generated to destructors.
> +              And they may be cloned to many places. In this case, we will
> +              set the location for them in tree-eh.c. But to ensure that EH
> +              does the right job, we first need mark their location as
> +              UNKNOWN_LOCATION.  */
> +           input_location = UNKNOWN_LOCATION;
>             eval = cleanup = NULL;
>             gimplify_and_add (TREE_OPERAND (*expr_p, 0), &eval);
>             gimplify_and_add (TREE_OPERAND (*expr_p, 1), &cleanup);
Richard Henderson - Aug. 17, 2012, 3:47 p.m.
On 2012-08-10 20:38, Dehao Chen wrote:
> + // { dg-final { scan-assembler "1 28 0" } }

This test case isn't going to work except with dwarf2, and with gas.
You can use -dA so that you can scan for file.c:line.  There are 
other examples in the testsuite.

This doesn't belong in guality.  It belongs in g++.dg/debug/.
It would be nice if you could add a java testcase to see that it
doesn't regress.

> ! record_in_goto_queue_label (struct leh_tf_state *tf, treemple stmt,
> tree label,
> ! 			    location_t location)

BTW, for the future, please fix your mailer to not wrap lines.

> + 	    /* For call expressions inside FINALL/CATCH block, if its location
> + 	       is unknown, gimplify_call_expr will set it to input_location.
> + 	       However, these calls are automatically generated to destructors.
> + 	       And they may be cloned to many places. In this case, we will
> + 	       set the location for them in tree-eh.c. But to ensure that EH
> + 	       does the right job, we first need mark their location as
> + 	       UNKNOWN_LOCATION.  */
> + 	    input_location = UNKNOWN_LOCATION;

I'll quibble with the wording here.  It reads as if we want to force 
all calls to have UNKNOWN_LOC, whereas all we want is to prevent any
calls that already have UNKNOWN_LOC from gaining input_loc via gimplify_call_expr.


r~

Patch

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;