Bug 58123 - [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor
Summary: [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: debug (show other bugs)
Version: 4.8.2
: P2 normal
Target Milestone: 4.8.5
Assignee: Aldy Hernandez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-11 11:16 UTC by Jan Kratochvil
Modified: 2021-04-14 00:20 UTC (History)
5 users (show)

See Also:
Host:
Target: x86_64-unknown-linux-gnu
Build:
Known to work:
Known to fail:
Last reconfirmed: 2015-02-18 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Kratochvil 2013-08-11 11:16:30 UTC
PASS: g++ (GCC) 4.7.3 20130221 (prerelease)
 - it did not generate separate line info for destructors at all
FAIL: g++ (GCC) 4.8.2 20130811 (prerelease)
FAIL: g++ (GCC) 4.9.0 20130811 (experimental)

class C {
public:
  C() {}
  ~C() {}
  int m() { return 0; }
};
/*  7 */ int main() {
/*  8 */   C a;
/*  9 */   C b;
/* 10 */   C c;
/* 11 */   return a.m() + b.m() + c.m();
/* 12 */ }

----------------------------------------------------------------------------
g++-4.7:
(gdb) start
8	/*  8 */   C a;
(gdb) next
9	/*  9 */   C b;
(gdb) next
10	/* 10 */   C c;
(gdb) next
11	/* 11 */   return a.m() + b.m() + c.m();
(gdb) next
12	/* 12 */ }
----------------------------------------------------------------------------
g++-4.8: each destructor has its own .debug_line entry:
(gdb) start
8	/*  8 */   C a;
(gdb) next
9	/*  9 */   C b;
(gdb) next
10	/* 10 */   C c;
(gdb) next
11	/* 11 */   return a.m() + b.m() + c.m();
(gdb) next
10	/* 10 */   C c;
(gdb) next
9	/*  9 */   C b;
(gdb) next
################## Missing: /*  8 */   C a;
11	/* 11 */   return a.m() + b.m() + c.m();
(gdb) next
12	/* 12 */ }
except for the last one:
/home/jkratoch/t/example2.C:10
  4005f7:       48 8d 45 ed             lea    -0x13(%rbp),%rax
  4005fb:       48 89 c7                mov    %rax,%rdi
  4005fe:       e8 2b 00 00 00          callq  40062e <C::~C()>
/home/jkratoch/t/example2.C:9
  400603:       48 8d 45 ee             lea    -0x12(%rbp),%rax
  400607:       48 89 c7                mov    %rax,%rdi
  40060a:       e8 1f 00 00 00          callq  40062e <C::~C()>
/home/jkratoch/t/example2.C:11
############################^^ here should be :8
  40060f:       48 8d 45 ef             lea    -0x11(%rbp),%rax
  400613:       48 89 c7                mov    %rax,%rdi
  400616:       e8 13 00 00 00          callq  40062e <C::~C()>
############################ Missing debug line for :11 or :12 here:
  40061b:       89 d8                   mov    %ebx,%eax
/home/jkratoch/t/example2.C:12
  40061d:       48 83 c4 18             add    $0x18,%rsp
  400621:       5b                      pop    %rbx
  400622:       5d                      pop    %rbp
  400623:       c3                      retq   

Downstream user bugreport: https://bugzilla.redhat.com/show_bug.cgi?id=995804
Comment 1 Jakub Jelinek 2013-08-16 15:28:27 UTC
The dtors contain no lineno, they get it from the corresponding try during eh pass.
Comment 2 Jakub Jelinek 2013-10-16 09:49:37 UTC
GCC 4.8.2 has been released.
Comment 3 asmwarrior 2013-10-28 13:18:10 UTC
@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
Comment 4 Richard Biener 2014-05-22 09:05:41 UTC
GCC 4.8.3 is being released, adjusting target milestone.
Comment 5 Jakub Jelinek 2014-12-19 13:27:13 UTC
GCC 4.8.4 has been released.
Comment 6 Aldy Hernandez 2015-02-18 17:57:01 UTC
I'll take a look.
Comment 7 Aldy Hernandez 2015-02-18 21:13:03 UTC
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?
Comment 8 Jan Kratochvil 2015-02-18 21:40:19 UTC
(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).
Comment 10 asmwarrior 2015-02-20 05:00:47 UTC
(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.
Comment 11 Aldy Hernandez 2015-02-21 00:27:36 UTC
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
Comment 12 Aldy Hernandez 2015-02-21 00:32:24 UTC
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.
Comment 13 CVS Commits 2021-04-14 00:20:11 UTC
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.