| Message ID | 54E61DE2.5010401@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/19/15 10:31, Aldy Hernandez wrote: > [Ughh...I'm apparently incapable of CCing gcc-patches on the first try. > My apologies again.] > > Please see the long explanation in the PR: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123 > > But the short explanation for this patch is that, at the very least, we > should fix the location of the GIMPLE_TRY_FINALLY. Currently we're > preferring the input_location, instead of the location of the > TRY_FINALLY_EXPR. > > An incorrect GIMPLE_TRY_FINALLY location causes the location of > location-less FINALLY statements to also have an incorrect location, > causing an inconsistent debugging experience. > > As explained in the PR, I would ideally like to get rid of the kludge > where we set the location of location-less FINALLY statements to be that > of the TRY statement, but that may be by design, and/or beyond the scope > of this fix. > > Tested on x86-64 Linux. > > OK for mainline? > > > > > curr > > > commit 119b6f5c628831706ac8fa26a906d80cf996bbe3 > Author: Aldy Hernandez<aldyh@redhat.com> > Date: Wed Feb 18 13:31:41 2015 -0800 > > PR debug/58123 > * gimplify.c (gimplify_expr): Prefer location of TRY_FINALLY_EXPR > over input_location. > testsuite/ > * g++.dg/debug/dwarf2/deallocator.C: Adjust for correct try > location. > * g++.dg/gcov/gcov-2.C: Likewise. OK. Jeff
On 02/19/2015 12:31 PM, Aldy Hernandez wrote: > As explained in the PR, I would ideally like to get rid of the kludge > where we set the location of location-less FINALLY statements to be that > of the TRY statement, but that may be by design, and/or beyond the scope > of this fix. This reminds me of the semi-recent change to disassociate variable cleanups with the location of the declaration. I think Jakub added a pass specifically for this purpose? I think making the change you suggest would make sense, but probably as a separate patch. Jason
On 02/23/2015 10:37 AM, Jason Merrill wrote: > On 02/19/2015 12:31 PM, Aldy Hernandez wrote: >> As explained in the PR, I would ideally like to get rid of the kludge >> where we set the location of location-less FINALLY statements to be that >> of the TRY statement, but that may be by design, and/or beyond the scope >> of this fix. > > This reminds me of the semi-recent change to disassociate variable > cleanups with the location of the declaration. I think Jakub added a > pass specifically for this purpose? I think making the change you > suggest would make sense, but probably as a separate patch. Absolutely, and that's why I closed the PR with the fix to the TRY_FINALLY gimple code. That at least fixes the regression. thanks. Aldy
diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 1353ada..d822913 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -8244,10 +8244,10 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p, TREE_CODE (*expr_p) == TRY_FINALLY_EXPR ? GIMPLE_TRY_FINALLY : GIMPLE_TRY_CATCH); - if (LOCATION_LOCUS (saved_location) != UNKNOWN_LOCATION) - gimple_set_location (try_, saved_location); - else + if (EXPR_HAS_LOCATION (save_expr)) gimple_set_location (try_, EXPR_LOCATION (save_expr)); + else if (LOCATION_LOCUS (saved_location) != UNKNOWN_LOCATION) + gimple_set_location (try_, saved_location); if (TREE_CODE (*expr_p) == TRY_CATCH_EXPR) gimple_try_set_catch_is_cleanup (try_, TRY_CATCH_IS_CLEANUP (*expr_p)); diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C b/gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C index 4057393..0fcd08e 100644 --- a/gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C +++ b/gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C @@ -41,6 +41,6 @@ void foo(int i) return; } // { dg-final { scan-assembler "deallocator.C:29" } } -// { dg-final { scan-assembler "deallocator.C:31" } } -// { dg-final { scan-assembler "deallocator.C:38" } } -// { dg-final { scan-assembler "deallocator.C:41" } } +// { dg-final { scan-assembler "deallocator.C:24" } } +// { dg-final { scan-assembler "deallocator.C:34" } } +// { dg-final { scan-assembler "deallocator.C:21" } } diff --git a/gcc/testsuite/g++.dg/gcov/gcov-2.C b/gcc/testsuite/g++.dg/gcov/gcov-2.C index 66d8af3..6d002f5 100644 --- a/gcc/testsuite/g++.dg/gcov/gcov-2.C +++ b/gcc/testsuite/g++.dg/gcov/gcov-2.C @@ -20,7 +20,7 @@ private: void foo() { - C c; /* count(1) */ + C c; /* count(2) */ c.seti (1); /* count(1) */ } diff --git a/gcc/testsuite/g++.dg/pr58123.C b/gcc/testsuite/g++.dg/pr58123.C new file mode 100644 index 0000000..7fe1a27 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr58123.C @@ -0,0 +1,18 @@ +// { dg-do compile } +// { dg-options "-fdump-tree-gimple-lineno" } + +// Test that the TRY block's location is the definition of "C a". + +class C { +public: + C() {} + ~C() {} + int m() { return 0; } +}; +int main() { + C a; + return a.m(); +} + +// { dg-final { scan-tree-dump-times "pr58123.C:13\.6\] try" 1 "gimple" } } +// { dg-final { cleanup-tree-dump "gimple" } }
[Ughh...I'm apparently incapable of CCing gcc-patches on the first try. My apologies again.] Please see the long explanation in the PR: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123 But the short explanation for this patch is that, at the very least, we should fix the location of the GIMPLE_TRY_FINALLY. Currently we're preferring the input_location, instead of the location of the TRY_FINALLY_EXPR. An incorrect GIMPLE_TRY_FINALLY location causes the location of location-less FINALLY statements to also have an incorrect location, causing an inconsistent debugging experience. As explained in the PR, I would ideally like to get rid of the kludge where we set the location of location-less FINALLY statements to be that of the TRY statement, but that may be by design, and/or beyond the scope of this fix. Tested on x86-64 Linux. OK for mainline? commit 119b6f5c628831706ac8fa26a906d80cf996bbe3 Author: Aldy Hernandez <aldyh@redhat.com> Date: Wed Feb 18 13:31:41 2015 -0800 PR debug/58123 * gimplify.c (gimplify_expr): Prefer location of TRY_FINALLY_EXPR over input_location. testsuite/ * g++.dg/debug/dwarf2/deallocator.C: Adjust for correct try location. * g++.dg/gcov/gcov-2.C: Likewise.