Bug 58123

The dtors contain no lineno, they get it from the corresponding try during eh pass.

GCC 4.8.2 has been released.

@Jan Kratochvil
This bug is also exists in GCC 4.8.1.

BTW: Your test code can also reproduce the GCC bug http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49951, I have add a comment there.

Yuanhui Zhang

GCC 4.8.3 is being released, adjusting target milestone.

GCC 4.8.4 has been released.

I'll take a look.

Confirmed.

There are a few curious things with this bug.

First, the reason we jump to line 11 is because the EH code, as it expands try/finally blocks, can't find the location of the destructor calls, so it uses the location of the enclosing TRY block.  So for something like this:

try {
  foo();
} finally {
  C::~C (&var);
}

...lower_try_finally_onedest(), will set the location of the destructor call to the try location:

      if (LOCATION_LOCUS (gimple_location (stmt)) == UNKNOWN_LOCATION)
	{
	  tree block = gimple_block (stmt);
	  gimple_set_location (stmt, gimple_location (tf->try_finally_expr));
	  gimple_set_block (stmt, block);
	}

Putting this aside for a second, my question is, do we really want a debugging experience where we jump from back from the end of scope, back to the declaration of the class?  Because one way of fixing this would be to get rid of the above code that pushes the try location onto finally statements that are locationless.  Then we could hit "next" on the debugger and go from line 8 -> 9 -> 10 -> 11 -> 12, without backtracking to some definition.

Second, the reason we don't have a location for the destructor call is apparently by design, so we don't do this jumping around I am arguing against.   The patch for PR debug/49951 specifically removes the location of the destructor so we don't jump around:

  /* build_delete sets the location of the destructor call to the
     current location, even though the destructor is going to be
     called later, at the end of the current scope.  This can lead to
     a "jumpy" behaviour for users of debuggers when they step around
     the end of the block.  So let's unset the location of the
     destructor call instead.  */
  if (cleanup != NULL && EXPR_P (cleanup))
    SET_EXPR_LOCATION (cleanup, UNKNOWN_LOCATION);

But alas, as I explain above, the try/finally code fills in the missing location for the destructor with the location of the try.

Third, be that as it may with the above two points, the TRY location is actually wrong in the trees:

      [a.cc:11:40] try      <-- ***THIS SHOULD BE LINE 8***
        {
          [a.cc:9:14] C::C ([a.cc:9:14] &b);
          [a.cc:9:14] try
            {
              [a.cc:10:14] C::C ([a.cc:10:14] &c);
              [a.cc:10:14] try
                {
                  [a.cc:11:22] D.2371 = C::m ([a.cc:11:22] &a);
                  [a.cc:11:30] D.2372 = C::m ([a.cc:11:30] &b);
                  [a.cc:11:25] D.2373 = D.2371 + D.2372;
                  [a.cc:11:38] D.2374 = C::m ([a.cc:11:38] &c);
                  [a.cc:11:39] D.2370 = D.2373 + D.2374;
                  [a.cc:11:39] goto <D.2375>;
                }
              finally
                {
                  C::~C (&c);
                }
            }
          finally
            {
              C::~C (&b);
            }
        }
      finally
        {
          C::~C (&a);
        }

This is actually a problem in the gimplifier.  The code gimplifying the TRY_FINALLY_EXPR has:

	    if (LOCATION_LOCUS (saved_location) != UNKNOWN_LOCATION)
	      gimple_set_location (try_, saved_location);
	    else
	      gimple_set_location (try_, EXPR_LOCATION (save_expr));

which means we are preferring the saved_location (gathered generically from input_location) over the TRY_FINALLY_EXPR's location.

IMO, this should be (untested):

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

This prefers the TRY_FINALLY_EXPR's location if available, else it uses the input_location upon entry.

With this untested patch, we do the jumpy thing Jan was expecting-- going back to line 8.  But I question whether we want this at all.  Instead, do we even want to fill in unknown location finally statements with the location of the try block?

It seems like there are two possible solutions (a) avoid filling in missing locations with the try block's location (b) fix the gimplifier to use the TRY_FINALLY_EXPR's location if available.

Either way, I think the gimplifier is wrong and should be fixed with (b) independently of this problem.

Jason, Jakub?

(In reply to Aldy Hernandez from comment #7)
> Putting this aside for a second, my question is, do we really want a
> debugging experience where we jump from back from the end of scope, back to
> the declaration of the class?

I do not think so.

Or if so then the destructor call needs to be marked with is_stmt=false; but GDB currently ignores is_stmt.  It would then show the declaration line during backtrace from crashed destructor (I am not sure it would be better than the 4.7 last statement of the block).

https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01216.html

(In reply to Jan Kratochvil from comment #8)
> (In reply to Aldy Hernandez from comment #7)
> > Putting this aside for a second, my question is, do we really want a
> > debugging experience where we jump from back from the end of scope, back to
> > the declaration of the class?
> 
> I do not think so.
> 
I agree with Jan. 
Jump back to declaration of a variable may be OK for command line debugging, but it is very annoying especially you use an IDE, which means the whole view window get refreshed.

Author: aldyh
Date: Sat Feb 21 00:27:05 2015
New Revision: 220886

URL: https://gcc.gnu.org/viewcvs?rev=220886&root=gcc&view=rev
Log:
	PR debug/58123
	* gimplify.c (gimplify_expr): Prefer location of TRY_FINALLY_EXPR
	over input_location.

Added:
    trunk/gcc/testsuite/g++.dg/pr58123.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/gimplify.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/debug/dwarf2/deallocator.C
    trunk/gcc/testsuite/g++.dg/gcov/gcov-2.C

Fix committed to trunk.  With this patch, we have a consistent debugging experience, as was originally expected/reported in the PR.

I agree, that eventually we should stop jumping back to the definition of the class instance.  Perhaps that can be filed as an enhancement request.  If that is the case, then what we want to revisit is whether the destructor call should be marked with is_stmt=false or whether we should avoid marking locationless FINALLY statements with the location of the TRY.

The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:006783f4b165dff25aae3697920fcf54754dddd4

commit r11-8164-g006783f4b165dff25aae3697920fcf54754dddd4
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Apr 13 16:18:54 2021 -0400

    c++: debug location of variable cleanups [PR88742]
    
    PR49951 complained about the debugger jumping back to the declaration of a
    local variable when we run its destructor.  That was fixed in 4.7, but broke
    again in 4.8.  PR58123 fixed an inconsistency in the behavior, but not the
    jumping around.  This patch addresses the issue by setting EXPR_LOCATION on
    a cleanup destructor call to the location of the closing brace of the
    compound-statement, or whatever token ends the scope of the variable.
    
    The change to cp_parser_compound_statement is so input_location is the }
    rather than the ; of the last substatement.
    
    gcc/cp/ChangeLog:
    
            PR c++/88742
            PR c++/49951
            PR c++/58123
            * semantics.c (set_cleanup_locs): New.
            (do_poplevel): Call it.
            * parser.c (cp_parser_compound_statement): Consume the }
            before finish_compound_stmt.
    
    gcc/testsuite/ChangeLog:
    
            PR c++/88742
            * g++.dg/debug/cleanup1.C: New test.
            * c-c++-common/Wimplicit-fallthrough-6.c: Adjust diagnostic line.
            * c-c++-common/Wimplicit-fallthrough-7.c: Likewise.
            * g++.dg/cpp2a/constexpr-dtor3.C: Likewise.
            * g++.dg/ext/constexpr-attr-cleanup1.C: Likewise.
            * g++.dg/tm/inherit2.C: Likewise.
            * g++.dg/tm/unsafe1.C: Likewise.
            * g++.dg/warn/Wimplicit-fallthrough-1.C: Likewise.
            * g++.dg/gcov/gcov-2.C: Adjust coverage counts.