| Message ID | CAO2gOZXfnETUe4wqjT7p6fd61hXreu9PDfqKxNz+HxpE0E7K0g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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);
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);
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~
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~
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~
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
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 >
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~
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~
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~
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
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~
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~
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~
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);
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);
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~
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;
Hi, This patch fixes the source location for automatically generated calls to deallocator. For example: 19 void foo(int i) 20 { 21 for (int j = 0; j < 10; j++) 22 { 23 t test; 24 test.foo(); 25 if (i + j) 26 { 27 test.bar(); 28 return; 29 } 30 } 31 return; 32 } The deallocator for "23 t test" is called in two places: Line 28 and line 30. However, gcc attributes both callsites to line 30. Bootstrapped and passed gcc regression tests. Is it ok for trunk? Thanks, Dehao gcc/ChangeLog 2012-07-31 Dehao Chen <dehao@google.com> * tree-eh.c (goto_queue_node): New field. (record_in_goto_queue): New parameter. (record_in_goto_queue_label): New parameter. (lower_try_finally_copy): Update source location. gcc/testsuite/ChangeLog 2012-07-31 Dehao Chen <dehao@google.com> * g++.dg/guality/deallocator.C: New test. default: @@ -1234,6 +1241,7 @@ for (index = 0; index < return_index + 1; index++) { tree lab; + gimple_stmt_iterator gsi; q = labels[index].q; if (! q) @@ -1252,6 +1260,11 @@ seq = lower_try_finally_dup_block (finally, state); lower_eh_constructs_1 (state, &seq); + for (gsi = gsi_start (seq); !gsi_end_p (gsi); gsi_next (&gsi)) + gimple_set_location (gsi_stmt (gsi), + q->code == GIMPLE_COND ? + EXPR_LOCATION (*q->stmt.tp) : + gimple_location (q->stmt.g)); gimple_seq_add_seq (&new_stmt, seq); gimple_seq_add_stmt (&new_stmt, q->cont_stmt);