[PATCH] Set correct source location for deallocator calls

[PATCH] Set correct source location for deallocator calls

[Dehao Chen]

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

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

[Dehao Chen]

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

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

[Richard Guenther]

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

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

[Richard Henderson]

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~

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

[Dehao Chen]

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~

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

[Richard Henderson]

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~

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

[Jason Merrill]

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

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

[Dehao Chen]

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
>

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

[Richard Henderson]

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~

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

[Dehao Chen]

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~

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

[Richard Henderson]

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~

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

[Jason Merrill]

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

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

[Dehao Chen]

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~

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

[Richard Henderson]

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~

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

[Dehao Chen]

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~

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

[Dehao Chen]

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

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

[Dehao Chen]

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

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

[Richard Henderson]

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~

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

[Dehao Chen]

Hi, Richard,

Thanks for the review. I've addressed most of the issues except the
java unittest (see comments below). The new patch is attached in the
end of this email.

Thanks,
Dehao

On Fri, Aug 17, 2012 at 8:47 AM, Richard Henderson <rth@redhat.com> wrote:
> 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.

Done.
>
> This doesn't belong in guality.  It belongs in g++.dg/debug/.

Done.

> It would be nice if you could add a java testcase to see that it
> doesn't regress.

I spend a whole day working on this, but find it very difficult to add
such a java test because:

* First, libjava testsuits are all runtime tests, i.e., it compiles
the byte code to native code, execute it, and compares the output to
expected output. There is no way to scan the assembly.
* Though there is a way to derive the line number at runtime in java
(using Exception().getStackTrace()), this method only works on VM, and
the gcj generated native code does not get the lineno.

Any suggestions on this?

>
>> ! 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.

Okay, I'll try. The problem is that we have to send mail in plain txt.
And in "plain text mode" gmail wraps each line to 80 characters and
wouldn't allow you change that...

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

Done.

New Patch:
gcc/ChangeLog
	 * 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/debug/dwarf2/deallocator.C: New test.

Index: gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C
===================================================================
--- gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C	(revision 0)
+++ gcc/testsuite/g++.dg/debug/dwarf2/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 -dA" }
+
+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)
@@ -321,6 +321,7 @@
 struct goto_queue_node
 {
   treemple stmt;
+  location_t location;
   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,
+		      location_t location)
 {
   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->location = location;
   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,
+			    location_t location)
 {
   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, location);
 }

 /* 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),
+				  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));
+      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));
+      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);
+      record_in_goto_queue (tf, new_stmt, -1, false, gimple_location (stmt));
       break;

     default:
@@ -866,13 +873,19 @@
    Make sure to record all new labels found.  */

 static gimple_seq
-lower_try_finally_dup_block (gimple_seq seq, struct leh_state *outer_state)
+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);
@@ -967,7 +980,8 @@
       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 = 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
@@ -1184,7 +1198,7 @@

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

@@ -1200,7 +1214,7 @@
       if (eh_else)
 	seq = gimple_eh_else_e_body (eh_else);
       else
-	seq = lower_try_finally_dup_block (finally, state);
+	seq = lower_try_finally_dup_block (finally, state, tf_loc);
       lower_eh_constructs_1 (state, &seq);

       emit_post_landing_pad (&eh_seq, tf->region);
@@ -1250,7 +1264,7 @@
 	  x = gimple_build_label (lab);
           gimple_seq_add_stmt (&new_stmt, x);

-	  seq = lower_try_finally_dup_block (finally, state);
+	  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)
@@ -7434,6 +7434,15 @@
 	    gimple_seq eval, cleanup;
 	    gimple try_;

+	    /* Calls to destructors are generated automatically in FINALL/CATCH
+	       block. They should have location as UNKNOWN_LOCATION. However,
+	       gimplify_call_expr will reset these call stmts to input_location
+	       if it finds stmt's location is unknown. To prevent resetting for
+	       destructors, we set the input_location to unknown.
+	       Note that this only affects the destructor calls in FINALL/CATCH
+	       block, and will automatically reset to its original value by the
+	       end of gimplify_expr.  */
+	    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);

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

[Dehao Chen]

ping....

Thanks,
Dehao

On Sat, Aug 18, 2012 at 6:02 AM, Dehao Chen <dehao@google.com> wrote:
> Hi, Richard,
>
> Thanks for the review. I've addressed most of the issues except the
> java unittest (see comments below). The new patch is attached in the
> end of this email.
>
> Thanks,
> Dehao
>
> On Fri, Aug 17, 2012 at 8:47 AM, Richard Henderson <rth@redhat.com> wrote:
>> 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.
>
> Done.
>>
>> This doesn't belong in guality.  It belongs in g++.dg/debug/.
>
> Done.
>
>> It would be nice if you could add a java testcase to see that it
>> doesn't regress.
>
> I spend a whole day working on this, but find it very difficult to add
> such a java test because:
>
> * First, libjava testsuits are all runtime tests, i.e., it compiles
> the byte code to native code, execute it, and compares the output to
> expected output. There is no way to scan the assembly.
> * Though there is a way to derive the line number at runtime in java
> (using Exception().getStackTrace()), this method only works on VM, and
> the gcj generated native code does not get the lineno.
>
> Any suggestions on this?
>
>>
>>> ! 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.
>
> Okay, I'll try. The problem is that we have to send mail in plain txt.
> And in "plain text mode" gmail wraps each line to 80 characters and
> wouldn't allow you change that...
>
>> 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.
>
> Done.
>
> New Patch:
> gcc/ChangeLog
>          * 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/debug/dwarf2/deallocator.C: New test.
>
> Index: gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C
> ===================================================================
> --- gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C     (revision 0)
> +++ gcc/testsuite/g++.dg/debug/dwarf2/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 -dA" }
> +
> +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)
> @@ -321,6 +321,7 @@
>  struct goto_queue_node
>  {
>    treemple stmt;
> +  location_t location;
>    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,
> +                     location_t location)
>  {
>    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->location = location;
>    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,
> +                           location_t location)
>  {
>    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, location);
>  }
>
>  /* 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),
> +                                 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));
> +      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));
> +      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);
> +      record_in_goto_queue (tf, new_stmt, -1, false, gimple_location (stmt));
>        break;
>
>      default:
> @@ -866,13 +873,19 @@
>     Make sure to record all new labels found.  */
>
>  static gimple_seq
> -lower_try_finally_dup_block (gimple_seq seq, struct leh_state *outer_state)
> +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);
> @@ -967,7 +980,8 @@
>        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 = 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
> @@ -1184,7 +1198,7 @@
>
>    if (tf->may_fallthru)
>      {
> -      seq = lower_try_finally_dup_block (finally, state);
> +      seq = lower_try_finally_dup_block (finally, state, tf_loc);
>        lower_eh_constructs_1 (state, &seq);
>        gimple_seq_add_seq (&new_stmt, seq);
>
> @@ -1200,7 +1214,7 @@
>        if (eh_else)
>         seq = gimple_eh_else_e_body (eh_else);
>        else
> -       seq = lower_try_finally_dup_block (finally, state);
> +       seq = lower_try_finally_dup_block (finally, state, tf_loc);
>        lower_eh_constructs_1 (state, &seq);
>
>        emit_post_landing_pad (&eh_seq, tf->region);
> @@ -1250,7 +1264,7 @@
>           x = gimple_build_label (lab);
>            gimple_seq_add_stmt (&new_stmt, x);
>
> -         seq = lower_try_finally_dup_block (finally, state);
> +         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)
> @@ -7434,6 +7434,15 @@
>             gimple_seq eval, cleanup;
>             gimple try_;
>
> +           /* Calls to destructors are generated automatically in FINALL/CATCH
> +              block. They should have location as UNKNOWN_LOCATION. However,
> +              gimplify_call_expr will reset these call stmts to input_location
> +              if it finds stmt's location is unknown. To prevent resetting for
> +              destructors, we set the input_location to unknown.
> +              Note that this only affects the destructor calls in FINALL/CATCH
> +              block, and will automatically reset to its original value by the
> +              end of gimplify_expr.  */
> +           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);

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

[Richard Henderson]

On 08/17/2012 03:02 PM, Dehao Chen wrote:
> I spend a whole day working on this, but find it very difficult to add
> such a java test because:
> 
> * First, libjava testsuits are all runtime tests, i.e., it compiles
> the byte code to native code, execute it, and compares the output to
> expected output. There is no way to scan the assembly.
> * Though there is a way to derive the line number at runtime in java
> (using Exception().getStackTrace()), this method only works on VM, and
> the gcj generated native code does not get the lineno.
> 
> Any suggestions on this?

Hmm, not from me, unfortunately.  Cc'ing the java list for clues.
I won't hang up the main patch for this though.

>> BTW, for the future, please fix your mailer to not wrap lines.
> 
> Okay, I'll try. The problem is that we have to send mail in plain txt.
> And in "plain text mode" gmail wraps each line to 80 characters and
> wouldn't allow you change that...

In that case use a text/plain attachment (which, not having tried it myself,
may require you use a .txt suffix on the patch file).  Most mail readers will
show those inline.  It's certainly better than having actually corrupt data
sent to the list.

> +// { dg-options "-O2 -fno-exceptions -g -dA" }
...
> +// { dg-final { scan-assembler "1 28 0" } }

You're still scanning for the .loc line, not the "test.c:28"
comment added by -dA.

To understand the problem, go back to your build tree, edit
auto-host.h and undefine HAVE_AS_DWARF2_DEBUG_LINE.  Then
rerun the testsuite with RUNTESTFLAGS=dwarf2.exp.

> +	    /* Calls to destructors are generated automatically in FINALL/CATCH
> +	       block. They should have location as UNKNOWN_LOCATION. However,
> +	       gimplify_call_expr will reset these call stmts to input_location
> +	       if it finds stmt's location is unknown. To prevent resetting for
> +	       destructors, we set the input_location to unknown.
> +	       Note that this only affects the destructor calls in FINALL/CATCH
> +	       block, and will automatically reset to its original value by the
> +	       end of gimplify_expr.  */

s/FINALL/FINALLY/g


r~

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

[Bryce McKinlay]

On Thu, Aug 30, 2012 at 3:28 PM, Richard Henderson <rth@redhat.com> wrote:
> On 08/17/2012 03:02 PM, Dehao Chen wrote:
>> I spend a whole day working on this, but find it very difficult to add
>> such a java test because:
>>
>> * First, libjava testsuits are all runtime tests, i.e., it compiles
>> the byte code to native code, execute it, and compares the output to
>> expected output. There is no way to scan the assembly.
>> * Though there is a way to derive the line number at runtime in java
>> (using Exception().getStackTrace()), this method only works on VM, and
>> the gcj generated native code does not get the lineno.
>>
>> Any suggestions on this?
>
> Hmm, not from me, unfortunately.  Cc'ing the java list for clues.
> I won't hang up the main patch for this though.

libjava calls out to addr2line to get the line number and source file
name for stack traces. As long as it can find addr2line you should get
a line number - but some platforms don't have it.

Ref: libjava/stacktrace.cc and libjava/gnu/gcj/runtime/NameFinder.java

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

[Andrew Haley]

On 08/30/2012 03:28 PM, Richard Henderson wrote:
> On 08/17/2012 03:02 PM, Dehao Chen wrote:
>> I spend a whole day working on this, but find it very difficult to add
>> such a java test because:
>>
>> * First, libjava testsuits are all runtime tests, i.e., it compiles
>> the byte code to native code, execute it, and compares the output to
>> expected output. There is no way to scan the assembly.
>> * Though there is a way to derive the line number at runtime in java
>> (using Exception().getStackTrace()), this method only works on VM, and
>> the gcj generated native code does not get the lineno.
>>
>> Any suggestions on this?
> 
> Hmm, not from me, unfortunately.  Cc'ing the java list for clues.
> I won't hang up the main patch for this though.

Fair enough.  As Bryce said, line numbers should work if you have
addr2line installed.

Can't we scan the assembly?  Is the problem simply that the logic to
scan the assembly code isn't present in the libgcj testsuite?

Andrew.

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

[Richard Henderson]

On 08/30/2012 08:20 AM, Andrew Haley wrote:
> Is the problem simply that the logic to
> scan the assembly code isn't present in the libgcj testsuite?

Yes, exactly.


r~

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

[Dehao Chen]

On Thu, Aug 30, 2012 at 9:33 AM, Richard Henderson <rth@redhat.com> wrote:
> On 08/30/2012 08:20 AM, Andrew Haley wrote:
>> Is the problem simply that the logic to
>> scan the assembly code isn't present in the libgcj testsuite?
>
> Yes, exactly.

For this case, I don't think that we want a testcase to rely on
addr2line in the system. So how about that that we add a test when
assembly scan is available in libgcj testsuit?

Thanks,
Dehao

>
>
> r~

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

[Andrew Haley]

On 09/04/2012 05:07 PM, Dehao Chen wrote:
> On Thu, Aug 30, 2012 at 9:33 AM, Richard Henderson <rth@redhat.com> wrote:
>> On 08/30/2012 08:20 AM, Andrew Haley wrote:
>>> Is the problem simply that the logic to
>>> scan the assembly code isn't present in the libgcj testsuite?
>>
>> Yes, exactly.
> 
> For this case, I don't think that we want a testcase to rely on
> addr2line in the system. So how about that that we add a test when
> assembly scan is available in libgcj testsuit?

Fine by me.  I guess you can just copy the scanning code from the gcc
testsuite.

Andrew.

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

[Bryce McKinlay]

On Tue, Sep 4, 2012 at 5:07 PM, Dehao Chen <dehao@google.com> wrote:
> On Thu, Aug 30, 2012 at 9:33 AM, Richard Henderson <rth@redhat.com> wrote:
>> On 08/30/2012 08:20 AM, Andrew Haley wrote:
>>> Is the problem simply that the logic to
>>> scan the assembly code isn't present in the libgcj testsuite?
>>
>> Yes, exactly.
>
> For this case, I don't think that we want a testcase to rely on
> addr2line in the system. So how about that that we add a test when
> assembly scan is available in libgcj testsuit?

Once Ian Lance Taylor's libbacktrace patch is integrated (see:
http://gcc.gnu.org/ml/gcc/2012-08/msg00317.html), we'll be able to get
rid of the code that calls addr2line from libgcj.

So, I think it would be fine to write a Java test case using
Throwable.getStackTrace(). Whichever approach is easiest for you is
fine.

Bryce

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

[Andrew Haley]

On 09/04/2012 05:32 PM, Bryce McKinlay wrote:
> On Tue, Sep 4, 2012 at 5:07 PM, Dehao Chen <dehao@google.com> wrote:
>> On Thu, Aug 30, 2012 at 9:33 AM, Richard Henderson <rth@redhat.com> wrote:
>>> On 08/30/2012 08:20 AM, Andrew Haley wrote:
>>>> Is the problem simply that the logic to
>>>> scan the assembly code isn't present in the libgcj testsuite?
>>>
>>> Yes, exactly.
>>
>> For this case, I don't think that we want a testcase to rely on
>> addr2line in the system. So how about that that we add a test when
>> assembly scan is available in libgcj testsuit?
> 
> Once Ian Lance Taylor's libbacktrace patch is integrated (see:
> http://gcc.gnu.org/ml/gcc/2012-08/msg00317.html), we'll be able to get
> rid of the code that calls addr2line from libgcj.

As I understand it, Ian Taylor's backtrace patch is intended for use in
gcc development, and as he puts it "Since its use in GCC would
be purely for GCC developers, it's not essential that it be fully
portable."  Not for gcj runtime.

Andrew.

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

[Bryce McKinlay]

On Tue, Sep 4, 2012 at 5:39 PM, Andrew Haley <aph@redhat.com> wrote:
> On 09/04/2012 05:32 PM, Bryce McKinlay wrote:
>> On Tue, Sep 4, 2012 at 5:07 PM, Dehao Chen <dehao@google.com> wrote:
>>> On Thu, Aug 30, 2012 at 9:33 AM, Richard Henderson <rth@redhat.com> wrote:
>>>> On 08/30/2012 08:20 AM, Andrew Haley wrote:
>>>>> Is the problem simply that the logic to
>>>>> scan the assembly code isn't present in the libgcj testsuite?
>>>>
>>>> Yes, exactly.
>>>
>>> For this case, I don't think that we want a testcase to rely on
>>> addr2line in the system. So how about that that we add a test when
>>> assembly scan is available in libgcj testsuit?
>>
>> Once Ian Lance Taylor's libbacktrace patch is integrated (see:
>> http://gcc.gnu.org/ml/gcc/2012-08/msg00317.html), we'll be able to get
>> rid of the code that calls addr2line from libgcj.
>
> As I understand it, Ian Taylor's backtrace patch is intended for use in
> gcc development, and as he puts it "Since its use in GCC would
> be purely for GCC developers, it's not essential that it be fully
> portable."  Not for gcj runtime.

He's also planning to use it for libgo, and other gcc runtime libs
have indicated interest. It doesn't have to work on all platforms, and
I can't see how it would be any less portable than addr2line!

Bryce

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

[Andrew Haley]

On 09/04/2012 06:08 PM, Bryce McKinlay wrote:
> On Tue, Sep 4, 2012 at 5:39 PM, Andrew Haley <aph@redhat.com> wrote:
>> On 09/04/2012 05:32 PM, Bryce McKinlay wrote:
>>> On Tue, Sep 4, 2012 at 5:07 PM, Dehao Chen <dehao@google.com> wrote:
>>>> On Thu, Aug 30, 2012 at 9:33 AM, Richard Henderson <rth@redhat.com> wrote:
>>>>> On 08/30/2012 08:20 AM, Andrew Haley wrote:
>>>>>> Is the problem simply that the logic to
>>>>>> scan the assembly code isn't present in the libgcj testsuite?
>>>>>
>>>>> Yes, exactly.
>>>>
>>>> For this case, I don't think that we want a testcase to rely on
>>>> addr2line in the system. So how about that that we add a test when
>>>> assembly scan is available in libgcj testsuit?
>>>
>>> Once Ian Lance Taylor's libbacktrace patch is integrated (see:
>>> http://gcc.gnu.org/ml/gcc/2012-08/msg00317.html), we'll be able to get
>>> rid of the code that calls addr2line from libgcj.
>>
>> As I understand it, Ian Taylor's backtrace patch is intended for use in
>> gcc development, and as he puts it "Since its use in GCC would
>> be purely for GCC developers, it's not essential that it be fully
>> portable."  Not for gcj runtime.
> 
> He's also planning to use it for libgo, and other gcc runtime libs
> have indicated interest. It doesn't have to work on all platforms, and
> I can't see how it would be any less portable than addr2line!

I certainly can.  Maybe once it's shaken-down so it's at least as
robust as what we have now it'll be OK.  I suspect it hasn't had much
testing with, for example, unwinding through signal handlers.

Andrew.

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

[Bryce McKinlay]

On Tue, Sep 4, 2012 at 6:12 PM, Andrew Haley <aph@redhat.com> wrote:

>> He's also planning to use it for libgo, and other gcc runtime libs
>> have indicated interest. It doesn't have to work on all platforms, and
>> I can't see how it would be any less portable than addr2line!
>
> I certainly can.  Maybe once it's shaken-down so it's at least as
> robust as what we have now it'll be OK.  I suspect it hasn't had much
> testing with, for example, unwinding through signal handlers.

libgcj wouldn't actually use it for unwinding, we already have all
that. We'd just use it to read DWARF debug info and give us the source
code line numbers.

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

[Andrew Haley]

On 09/04/2012 06:17 PM, Bryce McKinlay wrote:
> On Tue, Sep 4, 2012 at 6:12 PM, Andrew Haley <aph@redhat.com> wrote:
> 
>>> He's also planning to use it for libgo, and other gcc runtime libs
>>> have indicated interest. It doesn't have to work on all platforms, and
>>> I can't see how it would be any less portable than addr2line!
>>
>> I certainly can.  Maybe once it's shaken-down so it's at least as
>> robust as what we have now it'll be OK.  I suspect it hasn't had much
>> testing with, for example, unwinding through signal handlers.
> 
> libgcj wouldn't actually use it for unwinding, we already have all
> that. We'd just use it to read DWARF debug info and give us the source
> code line numbers.

OK, as long as that's all it does.  I think I was perhaps a bit
misled by its description of "a stack backtrace library".  It
certainly looks like a nicer approach than addr2line, but is
going to be much less well-ported.  I guess we'll see how it goes.

Andrew.

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

[Dehao Chen]

Looks like even with addr2line properly installed, the gcj generated
code cannot get the correct source file/lineno. Do I need to pass in
anything to gcj to let it know where addr2line is?

Thanks,
Dehao

#javac stacktrace.java
#java stacktrace
stacktrace.e(stacktrace.java:42)
stacktrace.d(stacktrace.java:38)
stacktrace.c(stacktrace.java:31)
stacktrace.b(stacktrace.java:26)
stacktrace.a(stacktrace.java:19)
stacktrace.main(stacktrace.java:12)
#gcj *.class -o stacktrace.exe
#./stacktrace.exe
stacktrace.e(stacktrace.exe:-1)
stacktrace.d(stacktrace.exe:-1)
stacktrace.c(stacktrace.exe:-1)
stacktrace.b(stacktrace.exe:-1)
stacktrace.a(stacktrace.exe:-1)
stacktrace.main(stacktrace.exe:-1)

The java code is shown below:
stacktrace.java
/* This test should test the stacktrace functionality.
   We only print ClassName and MethName since the other information
   like FileName and LineNumber are not consistent while building
   native or interpreted and we want to test the output inside the dejagnu
   test environment.
   Also, we have to make the methods public since they might be optimized away
   with inline's and then the -O3/-O2 execution might fail.
*/
public class stacktrace {
  public static void main(String args[]) {
    try {
      new stacktrace().a();
    } catch (TopException e) {
    }
  }

  public void a() throws TopException {
    try {
      b();
    } catch (MiddleException e) {
      throw new TopException(e);
    }
  }

  public void b() throws MiddleException {
    c();
  }

  public void c() throws MiddleException {
    try {
      d();
    } catch (BottomException e) {
      throw new MiddleException(e);
    }
  }

  public void d() throws BottomException {
    e();
  }

  public void e() throws BottomException {
    throw new BottomException();
  }
}

class TopException extends Exception {
  TopException(Throwable cause) {
    super(cause);
  }
}

class MiddleException extends Exception {
  MiddleException(Throwable cause) {
    super(cause);
  }
}

class BottomException extends Exception {
  BottomException() {
    StackTraceElement stack[] = this.getStackTrace();
    for (int i = 0; i < stack.length; i++) {
      String className = stack[i].getClassName();
      String methodName = stack[i].getMethodName();
      System.out.println(className + "." + methodName + "(" +
                         stack[i].getFileName() + ":" +
                         stack[i].getLineNumber() +  ")");
    }
  }
}

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

[Dehao Chen]

On Tue, Sep 4, 2012 at 9:22 AM, Andrew Haley <aph@redhat.com> wrote:
> On 09/04/2012 05:07 PM, Dehao Chen wrote:
>> On Thu, Aug 30, 2012 at 9:33 AM, Richard Henderson <rth@redhat.com> wrote:
>>> On 08/30/2012 08:20 AM, Andrew Haley wrote:
>>>> Is the problem simply that the logic to
>>>> scan the assembly code isn't present in the libgcj testsuite?
>>>
>>> Yes, exactly.
>>
>> For this case, I don't think that we want a testcase to rely on
>> addr2line in the system. So how about that that we add a test when
>> assembly scan is available in libgcj testsuit?
>
> Fine by me.  I guess you can just copy the scanning code from the gcc
> testsuite.

I tried that, but it is not trivial, and simply copying "proc
scan-assembler" to libjava seems ugly. Do libjava people really think
it's worth to add scan-assembler and other premitives in gcc testsuite
into libjava testsuite? If yes, I'll leave it to the TODO list.

Thanks,
Dehao
>
> Andrew.
>

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

[Andrew Haley]

On 09/04/2012 09:31 PM, Dehao Chen wrote:
> Looks like even with addr2line properly installed, the gcj generated
> code cannot get the correct source file/lineno. Do I need to pass in
> 
> #javac stacktrace.java
> #java stacktrace
> stacktrace.e(stacktrace.java:42)
> stacktrace.d(stacktrace.java:38)
> stacktrace.c(stacktrace.java:31)
> stacktrace.b(stacktrace.java:26)
> stacktrace.a(stacktrace.java:19)
> stacktrace.main(stacktrace.java:12)
> #gcj *.class -o stacktrace.exe
> #./stacktrace.exe
> stacktrace.e(stacktrace.exe:-1)
> stacktrace.d(stacktrace.exe:-1)
> stacktrace.c(stacktrace.exe:-1)
> stacktrace.b(stacktrace.exe:-1)
> stacktrace.a(stacktrace.exe:-1)
> stacktrace.main(stacktrace.exe:-1)

Works for me:

[aph@nighthawk ~]$ gcj stacktrace.java --main=stacktrace -g
[aph@nighthawk ~]$ ./a.out
stacktrace.e(stacktrace.java:42)
stacktrace.d(stacktrace.java:38)
stacktrace.c(stacktrace.java:31)
stacktrace.b(stacktrace.java:26)
stacktrace.a(stacktrace.java:19)
stacktrace.main(stacktrace.java:12)

Aren't you just compiling without -g ?  There is no debuginfo.

Andrew.

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

[Andrew Pinski]

On Wed, Sep 5, 2012 at 12:29 AM, Andrew Haley <aph@redhat.com> wrote:
> On 09/04/2012 09:31 PM, Dehao Chen wrote:
>> Looks like even with addr2line properly installed, the gcj generated
>> code cannot get the correct source file/lineno. Do I need to pass in
>>
>> #javac stacktrace.java
>> #java stacktrace
>> stacktrace.e(stacktrace.java:42)
>> stacktrace.d(stacktrace.java:38)
>> stacktrace.c(stacktrace.java:31)
>> stacktrace.b(stacktrace.java:26)
>> stacktrace.a(stacktrace.java:19)
>> stacktrace.main(stacktrace.java:12)
>> #gcj *.class -o stacktrace.exe
>> #./stacktrace.exe
>> stacktrace.e(stacktrace.exe:-1)
>> stacktrace.d(stacktrace.exe:-1)
>> stacktrace.c(stacktrace.exe:-1)
>> stacktrace.b(stacktrace.exe:-1)
>> stacktrace.a(stacktrace.exe:-1)
>> stacktrace.main(stacktrace.exe:-1)
>
> Works for me:
>
> [aph@nighthawk ~]$ gcj stacktrace.java --main=stacktrace -g
> [aph@nighthawk ~]$ ./a.out
> stacktrace.e(stacktrace.java:42)
> stacktrace.d(stacktrace.java:38)
> stacktrace.c(stacktrace.java:31)
> stacktrace.b(stacktrace.java:26)
> stacktrace.a(stacktrace.java:19)
> stacktrace.main(stacktrace.java:12)
>
> Aren't you just compiling without -g ?  There is no debuginfo.

The other thing that might be needed is a newer addr2line which works
correctly with the dwarf2(4) that GCC outputs.

Thanks,
Andrew

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

[Mark Wielaard]

On Tue, 2012-09-04 at 18:17 +0100, Bryce McKinlay wrote:
> libgcj wouldn't actually use it for unwinding, we already have all
> that. We'd just use it to read DWARF debug info and give us the source
> code line numbers.

Casey Marshell did also write that part some time ago, but it was never
finished/integrated.
http://gcc.gnu.org/ml/java-patches/2004-q3/msg00350.html

Cheers,

Mark

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

[Dehao Chen]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

Hi,

I've added a libjava unittest which verifies that this patch will not
break Java debug info. I've also incorporated Richard's review in the
previous mail. Attached is the new patch, which passed bootstrap and
all gcc/libjava testsuites on x86.

Is it ok for trunk?

Thanks,
Dehao

gcc/ChangeLog:
2012-09-08  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-09-08  Dehao Chen  <dehao@google.com>

	* g++.dg/debug/dwarf2/deallocator.C: New test.

libjava/ChangeLog:
2012-09-08  Dehao Chen  <dehao@google.com>

	* testsuite/libjava.lang/sourcelocation.java: New cases.
	* testsuite/libjava.lang/sourcelocation.out: New cases.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 8676 bytes --]

Index: libjava/testsuite/libjava.lang/sourcelocation.java
===================================================================
--- libjava/testsuite/libjava.lang/sourcelocation.java	(revision 0)
+++ libjava/testsuite/libjava.lang/sourcelocation.java	(revision 0)
@@ -0,0 +1,18 @@
+/* This test should test the source location attribution.
+   We print the line number of different parts of the program to make sure
+   that the source code attribution is correct.
+   To make this test pass, one need to have up-to-date addr2line installed
+   to parse the dwarf4 data format.
+*/
+public class sourcelocation {
+  public static void main(String args[]) {
+    try {
+      System.out.println(new Exception().getStackTrace()[0].getLineNumber());
+      throw new Exception();
+    } catch (Exception e) {
+      System.out.println(new Exception().getStackTrace()[0].getLineNumber());
+    } finally {
+      System.out.println(new Exception().getStackTrace()[0].getLineNumber());
+    }
+  }
+}
Index: libjava/testsuite/libjava.lang/sourcelocation.out
===================================================================
--- libjava/testsuite/libjava.lang/sourcelocation.out	(revision 0)
+++ libjava/testsuite/libjava.lang/sourcelocation.out	(revision 0)
@@ -0,0 +1,3 @@
+10
+13
+15
Index: libjava/testsuite/libjava.lang/sourcelocation.jar
===================================================================
Cannot display: file marked as a binary type.
svn:mime-type = application/octet-stream

Property changes on: libjava/testsuite/libjava.lang/sourcelocation.jar
___________________________________________________________________
Added: svn:mime-type
   + application/octet-stream

Index: gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C
===================================================================
--- gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C	(revision 0)
+++ gcc/testsuite/g++.dg/debug/dwarf2/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 -dA" }
+
+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 "deallocator.C:28" } }
Index: gcc/tree-eh.c
===================================================================
--- gcc/tree-eh.c	(revision 191083)
+++ gcc/tree-eh.c	(working copy)
@@ -321,6 +321,7 @@ static bitmap eh_region_may_contain_throw_map;
 struct goto_queue_node
 {
   treemple stmt;
+  location_t location;
   gimple_seq repl_stmt;
   gimple cont_stmt;
   int index;
@@ -560,7 +561,8 @@ static void
 record_in_goto_queue (struct leh_tf_state *tf,
                       treemple new_stmt,
                       int index,
-                      bool is_label)
+                      bool is_label,
+		      location_t location)
 {
   size_t active, size;
   struct goto_queue_node *q;
@@ -583,6 +585,7 @@ record_in_goto_queue (struct leh_tf_state *tf,
   memset (q, 0, sizeof (*q));
   q->stmt = new_stmt;
   q->index = index;
+  q->location = location;
   q->is_label = is_label;
 }
 
@@ -590,7 +593,8 @@ record_in_goto_queue (struct leh_tf_state *tf,
    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,
+			    location_t location)
 {
   int index;
   treemple temp, new_stmt;
@@ -629,7 +633,7 @@ static void
      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, location);
 }
 
 /* For any GIMPLE_GOTO or GIMPLE_RETURN, decide whether it leaves a try_finally
@@ -649,19 +653,22 @@ maybe_record_in_goto_queue (struct leh_state *stat
     {
     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),
+				  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));
+      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));
+      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);
+      record_in_goto_queue (tf, new_stmt, -1, false, gimple_location (stmt));
       break;
 
     default:
@@ -866,13 +873,19 @@ frob_into_branch_around (gimple tp, eh_region regi
    Make sure to record all new labels found.  */
 
 static gimple_seq
-lower_try_finally_dup_block (gimple_seq seq, struct leh_state *outer_state)
+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);
@@ -967,7 +980,8 @@ honor_protect_cleanup_actions (struct leh_state *o
       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 = 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
@@ -1184,7 +1198,7 @@ lower_try_finally_copy (struct leh_state *state, s
 
   if (tf->may_fallthru)
     {
-      seq = lower_try_finally_dup_block (finally, state);
+      seq = lower_try_finally_dup_block (finally, state, tf_loc);
       lower_eh_constructs_1 (state, &seq);
       gimple_seq_add_seq (&new_stmt, seq);
 
@@ -1200,7 +1214,7 @@ lower_try_finally_copy (struct leh_state *state, s
       if (eh_else)
 	seq = gimple_eh_else_e_body (eh_else);
       else
-	seq = lower_try_finally_dup_block (finally, state);
+	seq = lower_try_finally_dup_block (finally, state, tf_loc);
       lower_eh_constructs_1 (state, &seq);
 
       emit_post_landing_pad (&eh_seq, tf->region);
@@ -1250,7 +1264,7 @@ lower_try_finally_copy (struct leh_state *state, s
 	  x = gimple_build_label (lab);
           gimple_seq_add_stmt (&new_stmt, x);
 
-	  seq = lower_try_finally_dup_block (finally, state);
+	  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 191083)
+++ gcc/gimplify.c	(working copy)
@@ -7429,6 +7429,15 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gi
 	    gimple_seq eval, cleanup;
 	    gimple try_;
 
+	    /* Calls to destructors are generated automatically in FINALLY/CATCH
+	       block. They should have location as UNKNOWN_LOCATION. However,
+	       gimplify_call_expr will reset these call stmts to input_location
+	       if it finds stmt's location is unknown. To prevent resetting for
+	       destructors, we set the input_location to unknown.
+	       Note that this only affects the destructor calls in FINALLY/CATCH
+	       block, and will automatically reset to its original value by the
+	       end of gimplify_expr.  */
+	    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);

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

[Dehao Chen]

ping...

Thanks,
Dehao

On Sun, Sep 9, 2012 at 5:42 AM, Dehao Chen <dehao@google.com> wrote:
> Hi,
>
> I've added a libjava unittest which verifies that this patch will not
> break Java debug info. I've also incorporated Richard's review in the
> previous mail. Attached is the new patch, which passed bootstrap and
> all gcc/libjava testsuites on x86.
>
> Is it ok for trunk?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog:
> 2012-09-08  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-09-08  Dehao Chen  <dehao@google.com>
>
>         * g++.dg/debug/dwarf2/deallocator.C: New test.
>
> libjava/ChangeLog:
> 2012-09-08  Dehao Chen  <dehao@google.com>
>
>         * testsuite/libjava.lang/sourcelocation.java: New cases.
>         * testsuite/libjava.lang/sourcelocation.out: New cases.

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

[Andrew Haley]

On 09/08/2012 10:42 PM, Dehao Chen wrote:
> I've added a libjava unittest which verifies that this patch will not
> break Java debug info. I've also incorporated Richard's review in the
> previous mail. Attached is the new patch, which passed bootstrap and
> all gcc/libjava testsuites on x86.
> 
> Is it ok for trunk?

Yes, thanks.

Andrew.

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

[H.J. Lu]

On Sat, Sep 8, 2012 at 2:42 PM, Dehao Chen <dehao@google.com> wrote:
> Hi,
>
> I've added a libjava unittest which verifies that this patch will not
> break Java debug info. I've also incorporated Richard's review in the
> previous mail. Attached is the new patch, which passed bootstrap and
> all gcc/libjava testsuites on x86.
>
> Is it ok for trunk?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog:
> 2012-09-08  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-09-08  Dehao Chen  <dehao@google.com>
>
>         * g++.dg/debug/dwarf2/deallocator.C: New test.
>
> libjava/ChangeLog:
> 2012-09-08  Dehao Chen  <dehao@google.com>
>
>         * testsuite/libjava.lang/sourcelocation.java: New cases.
>         * testsuite/libjava.lang/sourcelocation.out: New cases.

On Linux/x86, I got

FAIL: sourcelocation -O3 -findirect-dispatch output - source compiled test
FAIL: sourcelocation -O3 output - source compiled test
FAIL: sourcelocation -findirect-dispatch output - source compiled test
FAIL: sourcelocation output - source compiled test

spawn [open ...]^M
-1
-1
-1
PASS: sourcelocation -findirect-dispatch execution - source compiled test
FAIL: sourcelocation -findirect-dispatch output - source compiled test


-- 
H.J.

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

[H.J. Lu]

On Fri, Sep 14, 2012 at 9:25 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sat, Sep 8, 2012 at 2:42 PM, Dehao Chen <dehao@google.com> wrote:
>> Hi,
>>
>> I've added a libjava unittest which verifies that this patch will not
>> break Java debug info. I've also incorporated Richard's review in the
>> previous mail. Attached is the new patch, which passed bootstrap and
>> all gcc/libjava testsuites on x86.
>>
>> Is it ok for trunk?
>>
>> Thanks,
>> Dehao
>>
>> gcc/ChangeLog:
>> 2012-09-08  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-09-08  Dehao Chen  <dehao@google.com>
>>
>>         * g++.dg/debug/dwarf2/deallocator.C: New test.
>>
>> libjava/ChangeLog:
>> 2012-09-08  Dehao Chen  <dehao@google.com>
>>
>>         * testsuite/libjava.lang/sourcelocation.java: New cases.
>>         * testsuite/libjava.lang/sourcelocation.out: New cases.
>
> On Linux/x86, I got
>
> FAIL: sourcelocation -O3 -findirect-dispatch output - source compiled test
> FAIL: sourcelocation -O3 output - source compiled test
> FAIL: sourcelocation -findirect-dispatch output - source compiled test
> FAIL: sourcelocation output - source compiled test
>
> spawn [open ...]^M
> -1
> -1
> -1
> PASS: sourcelocation -findirect-dispatch execution - source compiled test
> FAIL: sourcelocation -findirect-dispatch output - source compiled test
>
>

I am using binutils mainline 20120914 from CVS.

-- 
H.J.

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

[Andrew Pinski]

On Fri, Sep 14, 2012 at 9:25 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sat, Sep 8, 2012 at 2:42 PM, Dehao Chen <dehao@google.com> wrote:
>> Hi,
>>
>> I've added a libjava unittest which verifies that this patch will not
>> break Java debug info. I've also incorporated Richard's review in the
>> previous mail. Attached is the new patch, which passed bootstrap and
>> all gcc/libjava testsuites on x86.
>>
>> Is it ok for trunk?
>>
>> Thanks,
>> Dehao
>>
>> gcc/ChangeLog:
>> 2012-09-08  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-09-08  Dehao Chen  <dehao@google.com>
>>
>>         * g++.dg/debug/dwarf2/deallocator.C: New test.
>>
>> libjava/ChangeLog:
>> 2012-09-08  Dehao Chen  <dehao@google.com>
>>
>>         * testsuite/libjava.lang/sourcelocation.java: New cases.
>>         * testsuite/libjava.lang/sourcelocation.out: New cases.
>
> On Linux/x86, I got
>
> FAIL: sourcelocation -O3 -findirect-dispatch output - source compiled test
> FAIL: sourcelocation -O3 output - source compiled test
> FAIL: sourcelocation -findirect-dispatch output - source compiled test
> FAIL: sourcelocation output - source compiled test
>
> spawn [open ...]^M
> -1
> -1
> -1
> PASS: sourcelocation -findirect-dispatch execution - source compiled test
> FAIL: sourcelocation -findirect-dispatch output - source compiled test

I bet you have an older addr2line installed.

Thanks,
Andrew Pinski

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

[H.J. Lu]

On Fri, Sep 14, 2012 at 9:27 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Fri, Sep 14, 2012 at 9:25 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Sat, Sep 8, 2012 at 2:42 PM, Dehao Chen <dehao@google.com> wrote:
>>> Hi,
>>>
>>> I've added a libjava unittest which verifies that this patch will not
>>> break Java debug info. I've also incorporated Richard's review in the
>>> previous mail. Attached is the new patch, which passed bootstrap and
>>> all gcc/libjava testsuites on x86.
>>>
>>> Is it ok for trunk?
>>>
>>> Thanks,
>>> Dehao
>>>
>>> gcc/ChangeLog:
>>> 2012-09-08  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-09-08  Dehao Chen  <dehao@google.com>
>>>
>>>         * g++.dg/debug/dwarf2/deallocator.C: New test.
>>>
>>> libjava/ChangeLog:
>>> 2012-09-08  Dehao Chen  <dehao@google.com>
>>>
>>>         * testsuite/libjava.lang/sourcelocation.java: New cases.
>>>         * testsuite/libjava.lang/sourcelocation.out: New cases.
>>
>> On Linux/x86, I got
>>
>> FAIL: sourcelocation -O3 -findirect-dispatch output - source compiled test
>> FAIL: sourcelocation -O3 output - source compiled test
>> FAIL: sourcelocation -findirect-dispatch output - source compiled test
>> FAIL: sourcelocation output - source compiled test
>>
>> spawn [open ...]^M
>> -1
>> -1
>> -1
>> PASS: sourcelocation -findirect-dispatch execution - source compiled test
>> FAIL: sourcelocation -findirect-dispatch output - source compiled test
>
> I bet you have an older addr2line installed.
>

I am using  addr2line from binutils 20120914.


-- 
H.J.

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

[Dehao Chen]

I tried the up-to-date addr2line on any "gcc -g" generated code, it
does not work either. This is because in the new dwarf, the
DW_AT_high_pc now actually means the size. e.g.

 <1><9b>: Abbrev Number: 2 (DW_TAG_subprogram)
    <9c>   DW_AT_external    : 1	
    <9c>   DW_AT_name        : bar	
    <a0>   DW_AT_decl_file   : 1	
    <a1>   DW_AT_decl_line   : 8	
    <a2>   DW_AT_linkage_name: (indirect string, offset: 0x7b): _Z3barv	
    <a6>   DW_AT_type        : <0x8d>	
    <aa>   DW_AT_low_pc      : 0x400583	
    <b2>   DW_AT_high_pc     : 0x37 0x0	
    <ba>   DW_AT_frame_base  : 1 byte block: 9c 	(DW_OP_call_frame_cfa)
    <bc>   DW_AT_GNU_all_call_sites: 1	
    <bc>   DW_AT_sibling     : <0xff>	

However, addr2line still thinks DW_AT_high_pc means "high_pc". I think
we should wait for binutil to catch up with gcc.

Thanks,
Dehao

On Sat, Sep 15, 2012 at 8:55 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Sep 14, 2012 at 9:27 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Fri, Sep 14, 2012 at 9:25 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Sat, Sep 8, 2012 at 2:42 PM, Dehao Chen <dehao@google.com> wrote:
>>>> Hi,
>>>>
>>>> I've added a libjava unittest which verifies that this patch will not
>>>> break Java debug info. I've also incorporated Richard's review in the
>>>> previous mail. Attached is the new patch, which passed bootstrap and
>>>> all gcc/libjava testsuites on x86.
>>>>
>>>> Is it ok for trunk?
>>>>
>>>> Thanks,
>>>> Dehao
>>>>
>>>> gcc/ChangeLog:
>>>> 2012-09-08  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-09-08  Dehao Chen  <dehao@google.com>
>>>>
>>>>         * g++.dg/debug/dwarf2/deallocator.C: New test.
>>>>
>>>> libjava/ChangeLog:
>>>> 2012-09-08  Dehao Chen  <dehao@google.com>
>>>>
>>>>         * testsuite/libjava.lang/sourcelocation.java: New cases.
>>>>         * testsuite/libjava.lang/sourcelocation.out: New cases.
>>>
>>> On Linux/x86, I got
>>>
>>> FAIL: sourcelocation -O3 -findirect-dispatch output - source compiled test
>>> FAIL: sourcelocation -O3 output - source compiled test
>>> FAIL: sourcelocation -findirect-dispatch output - source compiled test
>>> FAIL: sourcelocation output - source compiled test
>>>
>>> spawn [open ...]^M
>>> -1
>>> -1
>>> -1
>>> PASS: sourcelocation -findirect-dispatch execution - source compiled test
>>> FAIL: sourcelocation -findirect-dispatch output - source compiled test
>>
>> I bet you have an older addr2line installed.
>>
>
> I am using  addr2line from binutils 20120914.
>
>
> --
> H.J.

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

[H.J. Lu]

On Sat, Sep 15, 2012 at 9:09 AM, Dehao Chen <dehao@google.com> wrote:
> I tried the up-to-date addr2line on any "gcc -g" generated code, it
> does not work either. This is because in the new dwarf, the
> DW_AT_high_pc now actually means the size. e.g.
>
>  <1><9b>: Abbrev Number: 2 (DW_TAG_subprogram)
>     <9c>   DW_AT_external    : 1
>     <9c>   DW_AT_name        : bar
>     <a0>   DW_AT_decl_file   : 1
>     <a1>   DW_AT_decl_line   : 8
>     <a2>   DW_AT_linkage_name: (indirect string, offset: 0x7b): _Z3barv
>     <a6>   DW_AT_type        : <0x8d>
>     <aa>   DW_AT_low_pc      : 0x400583
>     <b2>   DW_AT_high_pc     : 0x37 0x0
>     <ba>   DW_AT_frame_base  : 1 byte block: 9c         (DW_OP_call_frame_cfa)
>     <bc>   DW_AT_GNU_all_call_sites: 1
>     <bc>   DW_AT_sibling     : <0xff>
>
> However, addr2line still thinks DW_AT_high_pc means "high_pc". I think
> we should wait for binutil to catch up with gcc.
>

So, the meaning of  DW_AT_high_pc in DWARF4 is different
from DWARF3?

-- 
H.J.

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

[Dehao Chen]

Yeah, in dwarf2out.c:

 4590 add_AT_low_high_pc (dw_die_ref die, const char *lbl_low, const
char *lbl_high,
......
 4604   if (dwarf_version < 4)
 4605     attr.dw_attr_val.val_class = dw_val_class_lbl_id;
 4606   else
 4607     attr.dw_attr_val.val_class = dw_val_class_high_pc;
.
dw_val_class_lbl_id is handled:
 7984         case dw_val_class_lbl_id:
 7985           dw2_asm_output_addr (DWARF2_ADDR_SIZE, AT_lbl (a), "%s", name);
 7986           break;

dw_val_class_high_pc is handled:
 8027         case dw_val_class_high_pc:
 8028           dw2_asm_output_delta (DWARF2_ADDR_SIZE, AT_lbl (a),
 8029                                 get_AT_low_pc (die), "DW_AT_high_pc");
 8030           break;

The dwarf4 specification says:

If the value of the DW_AT_high_pc is of class address, it is the
relocated address of the first location past the last instruction
associated with the entity; if it is of class constant, the value is
an unsigned integer offset which when added to the low PC gives the
address of the first location past the last instruction associated
with the entity.

However, I'm not sure how to tell how the DW_AT_high_pc's class is
represented...

Maybe it's the bug in the gcc dwarf implementation?

Dehao

On Sun, Sep 16, 2012 at 2:06 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sat, Sep 15, 2012 at 9:09 AM, Dehao Chen <dehao@google.com> wrote:
>> I tried the up-to-date addr2line on any "gcc -g" generated code, it
>> does not work either. This is because in the new dwarf, the
>> DW_AT_high_pc now actually means the size. e.g.
>>
>>  <1><9b>: Abbrev Number: 2 (DW_TAG_subprogram)
>>     <9c>   DW_AT_external    : 1
>>     <9c>   DW_AT_name        : bar
>>     <a0>   DW_AT_decl_file   : 1
>>     <a1>   DW_AT_decl_line   : 8
>>     <a2>   DW_AT_linkage_name: (indirect string, offset: 0x7b): _Z3barv
>>     <a6>   DW_AT_type        : <0x8d>
>>     <aa>   DW_AT_low_pc      : 0x400583
>>     <b2>   DW_AT_high_pc     : 0x37 0x0
>>     <ba>   DW_AT_frame_base  : 1 byte block: 9c         (DW_OP_call_frame_cfa)
>>     <bc>   DW_AT_GNU_all_call_sites: 1
>>     <bc>   DW_AT_sibling     : <0xff>
>>
>> However, addr2line still thinks DW_AT_high_pc means "high_pc". I think
>> we should wait for binutil to catch up with gcc.
>>
>
> So, the meaning of  DW_AT_high_pc in DWARF4 is different
> from DWARF3?
>
> --
> H.J.

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

[Mark Wielaard]

On Sun, Sep 16, 2012 at 06:03:24AM +0800, Dehao Chen wrote:
> The dwarf4 specification says:
> 
> If the value of the DW_AT_high_pc is of class address, it is the
> relocated address of the first location past the last instruction
> associated with the entity; if it is of class constant, the value is
> an unsigned integer offset which when added to the low PC gives the
> address of the first location past the last instruction associated
> with the entity.
> 
> However, I'm not sure how to tell how the DW_AT_high_pc's class is
> represented...

You look at the form in which the attribute is encoded. If it is
DW_FORM_addr then it is of class address, otherwise (DW_FORM_data1,
DW_FORM_data2, DW_FORM_data4, DW_FORM_data8, DW_FORM_sdata or
DW_FORM_udata) it is of class constant.

Cheers,

Mark

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

[Mark Wielaard]

On Sun, Sep 16, 2012 at 12:09:04AM +0800, Dehao Chen wrote:
> I tried the up-to-date addr2line on any "gcc -g" generated code, it
> does not work either. This is because in the new dwarf, the
> DW_AT_high_pc now actually means the size. e.g.
> 
>  <1><9b>: Abbrev Number: 2 (DW_TAG_subprogram)
>     <9c>   DW_AT_external    : 1	
>     <9c>   DW_AT_name        : bar	
>     <a0>   DW_AT_decl_file   : 1	
>     <a1>   DW_AT_decl_line   : 8	
>     <a2>   DW_AT_linkage_name: (indirect string, offset: 0x7b): _Z3barv	
>     <a6>   DW_AT_type        : <0x8d>	
>     <aa>   DW_AT_low_pc      : 0x400583	
>     <b2>   DW_AT_high_pc     : 0x37 0x0	
>     <ba>   DW_AT_frame_base  : 1 byte block: 9c 	(DW_OP_call_frame_cfa)
>     <bc>   DW_AT_GNU_all_call_sites: 1	
>     <bc>   DW_AT_sibling     : <0xff>	
> 
> However, addr2line still thinks DW_AT_high_pc means "high_pc". I think
> we should wait for binutil to catch up with gcc.

I thought it had some months ago:
http://sourceware.org/ml/binutils/2012-04/msg00447.html

If that patch is present in your binutils and addr2line still doesn't
work as intented there might be a bug or some other place to update.

Thanks,

Mark