PR debug/58123: Set correct location for TRY blocks
diff mbox

Message ID 54E61DE2.5010401@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Feb. 19, 2015, 5:31 p.m. UTC
[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.

Comments

Jeff Law Feb. 20, 2015, 7:04 p.m. UTC | #1
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
Jason Merrill Feb. 23, 2015, 6:37 p.m. UTC | #2
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
Aldy Hernandez Feb. 23, 2015, 7:37 p.m. UTC | #3
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

Patch
diff mbox

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" } }