[Bug debug/58123] New: [4.8 Regression] debug line not tracked for last autovariable dtor

[Bug debug/58123] New: [4.8 Regression] debug line not tracked for last autovariable dtor

[jan.kratochvil at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

            Bug ID: 58123
           Summary: [4.8 Regression] debug line not tracked for last
                    autovariable dtor
           Product: gcc
           Version: 4.8.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: debug
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jan.kratochvil at redhat dot com
            Target: x86_64-unknown-linux-gnu

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

[Bug debug/58123] [4.8/4.9 Regression] debug line not tracked for last autovariable dtor

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org
   Target Milestone|---                         |4.8.2
            Summary|[4.8 Regression] debug line |[4.8/4.9 Regression] debug
                   |not tracked for last        |line not tracked for last
                   |autovariable dtor           |autovariable dtor

[Bug debug/58123] [4.8/4.9 Regression] debug line not tracked for last autovariable dtor

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.2                       |4.8.3

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.8.2 has been released.

[Bug debug/58123] [4.8/4.9 Regression] debug line not tracked for last autovariable dtor

[asmwarrior at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

asmwarrior <asmwarrior at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |asmwarrior at gmail dot com

--- Comment #3 from asmwarrior <asmwarrior at gmail dot com> ---
@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

[Bug debug/58123] [4.8/4.9 Regression] debug line not tracked for last autovariable dtor

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug debug/58123] [4.8/4.9/4.10 Regression] debug line not tracked for last autovariable dtor

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.3                       |4.8.4

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 4.8.3 is being released, adjusting target milestone.

[Bug debug/58123] [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.8.4                       |4.8.5

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 4.8.4 has been released.

[Bug debug/58123] [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aldyh at gcc dot gnu.org
           Assignee|unassigned at gcc dot gnu.org      |aldyh at gcc dot gnu.org

--- Comment #6 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
I'll take a look.

[Bug debug/58123] [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-02-18
     Ever confirmed|0                           |1

--- Comment #7 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
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?

[Bug debug/58123] [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor

[jan.kratochvil at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

--- Comment #8 from Jan Kratochvil <jan.kratochvil at redhat dot com> ---
(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).

[Bug debug/58123] [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

--- Comment #9 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01216.html

[Bug debug/58123] [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor

[asmwarrior at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

--- Comment #10 from asmwarrior <asmwarrior at gmail dot com> ---
(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.

[Bug debug/58123] [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

--- Comment #11 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
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

[Bug debug/58123] [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor

[aldyh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #12 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
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.

[Bug debug/58123] [4.8/4.9/5 Regression] debug line not tracked for last autovariable dtor

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58123

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
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.