Re: [PATCH] Fix an issue with the gdb step-over aka. "n" command

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

On 12/20/19 7:13 AM, Simon Marchi wrote:
> Hi Bernd,
> 
> On 2019-12-19 5:53 p.m., Bernd Edlinger wrote:
>> Does this explanation make sense?
> 
> Yes.  Well I think so, I have to admit this is a bit over my head,
> there are a lot of pieces to put together to have a good understanding
> of the problem.  I just did a first read, I'll sleep on it and come
> back to it later.
> 
> Thanks for the small reproducer, this is extremely valuable.  I think it
> will be a good idea to integrate it as a test case in the test suite.
> 
> In your patch to dwarf2read.c, I was a bit surprised to see:
> 
>   m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
> 
> So your fix only works if the inlined subroutine comes from another file?  If
> I move the tree_check function in next-inline.cc, the fix no longer applies,
> and we get the broken behavior.  From your previous email, I understand that
> this is expected.  I guess that if both are in the same file, we can't detect
> this situation using the same technique.
> 
> I also read about location views, since that's what Alexandre referred to.  It
> sounds like it's a magic solution that will allow GDB to do the right thing in
> this kind of situation.  If that's indeed the case, then it might be good to start
> exploring this path.  I'd like to have a better understanding of how this will help
> GDB make a smarter "next", and what kind of effort is needed to make GDB use it.  My
> understanding is that location views allow having an address mapped to multiple
> source locations.  For example, here's the problematic address in the next-inline
> test case I've compiled:
> 
>   ./next-inline.h:[++]
>   next-inline.h                                 28              0x1175               x
>   next-inline.h                                 30              0x1175       1       x
> 
>   ./next-inline.cc:[++]
>   next-inline.cc                                22              0x1175       2
> 
> Today, when I ask GDB "which source line does this address correspond to", it gives me
> one answer.  Does this mean that GDB will now say that 0x1175 corresponds to
> 
> - next-inline.h:28
> - next-inline.h:30
> - next-inline.cc:22
> 
> all at the same time?  Is one of these source locations more important than the others?
> If execution happens to stop exactly at this address, which location do we present to
> the user?
> 
> And to come back the problem at hand, how does this help GDB make a smarter "next"?
> 
> Btw, I stumbled on a bug with the TUI source display.  It might be caused by this patch,
> or it might be that this patch uncovers it.
> 
> When I do these actions:
> 
> - Start GDB with the next-inline test file (from this patch)
> - Enable the TUI
> - Type "start"
> - Type "s"
> - Type "n" twice
> 
> The TUI source display wrongfully jumps to the header file, line 24.
> When I type "frame", it says I'm stopped at next-line.cc:24.  So it
> is showing the right line number of the wrong file.
> 

I think meanwhile the display bug in the TUI window is fixed (by one of Tom Tromey's
recent patches), could you please check again?

I post both parts of the patch again for your reference here, and add a ChangeLog
which I forgot previously:

gdb:
2019-12-28  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* dwarf2read.c (lnp_state_machine): New member m_last_address.
	(lnp_state_machine::record_line): After a file changes or
	end sequence always assume a statement boundary.

gdb/testsuite:
2019-12-28  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* gdb.cp/next-inline.exp: New file.
	* gdb.cp/next-inline.cc: New file.
	* gdb.cp/next-inline.h: New file.



Thanks
Bernd.

From 63c0053d0c2bf9ec8ffbcb06712fe8aa243c32fc Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Sat, 23 Nov 2019 20:59:25 +0100
Subject: [PATCH 1/2] Fix an issue with the gdb step-over aka. "n" command

When debugging optimized code with inlined functions built
with -gstatement-frontiers debug info, it may happen that
an inline function has a line location exactly at the
end of a block which has the is_stmt attribute set, followed
by a location info which is at the call site but has the
is_stmt attribute cleared and is therefore ignored.

The visual effect is that "n" does sometimes not step over
the inlined function but instead jumps to the last line of
the inline function, but the inline function has already
returned, hence the line number information is inconsistent
at this point.

This patch detects a is_stmt location info followed by
a location info in a different file at the same address,
but without is_stmt location set, and forces the second
location info to replace the previous one.
---
 gdb/dwarf2read.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 819d37c..2f9a181 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -21148,6 +21148,7 @@ private:
      example, when discriminators are present.  PR 17276.  */
   unsigned int m_last_line = 0;
   bool m_line_has_non_zero_discriminator = false;
+  CORE_ADDR m_last_address = (CORE_ADDR) -1;
 };
 
 void
@@ -21330,7 +21331,10 @@ lnp_state_machine::record_line (bool end_sequence)
   else if (m_op_index == 0 || end_sequence)
     {
       fe->included_p = 1;
-      if (m_record_lines_p && (producer_is_codewarrior (m_cu) || m_is_stmt))
+      if (m_record_lines_p
+	  && (producer_is_codewarrior (m_cu) || m_is_stmt || end_sequence
+	      || (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
+		  && m_last_address == m_address)))
 	{
 	  if (m_last_subfile != m_cu->get_builder ()->get_current_subfile ()
 	      || end_sequence)
@@ -21353,6 +21357,7 @@ lnp_state_machine::record_line (bool end_sequence)
 		}
 	      m_last_subfile = m_cu->get_builder ()->get_current_subfile ();
 	      m_last_line = m_line;
+	      m_last_address = m_address;
 	    }
 	}
     }
-- 
1.9.1

From 95ba4780488b53104ea51cc0702f99a9a800984b Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Thu, 19 Dec 2019 23:41:37 +0100
Subject: [PATCH 2/2] Add a test case for step over inline functions

---
 gdb/testsuite/gdb.cp/next-inline.cc  | 34 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.cp/next-inline.exp | 39 ++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.cp/next-inline.h   | 34 +++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)
 create mode 100644 gdb/testsuite/gdb.cp/next-inline.cc
 create mode 100644 gdb/testsuite/gdb.cp/next-inline.exp
 create mode 100644 gdb/testsuite/gdb.cp/next-inline.h

diff --git a/gdb/testsuite/gdb.cp/next-inline.cc b/gdb/testsuite/gdb.cp/next-inline.cc
new file mode 100644
index 0000000..dcf5ab9
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/next-inline.cc
@@ -0,0 +1,34 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "next-inline.h"
+
+int __attribute__((noinline, noclone))
+get_alias_set (tree *t)
+{ if (t != NULL
+      && TREE_TYPE (t).z != 1
+      && TREE_TYPE (t).z != 2
+      && TREE_TYPE (t).z != 3)
+    return 0;
+  return 1;
+}
+
+tree xx;
+int main()
+{ get_alias_set(&xx);
+  abort();
+}
diff --git a/gdb/testsuite/gdb.cp/next-inline.exp b/gdb/testsuite/gdb.cp/next-inline.exp
new file mode 100644
index 0000000..6badc8c
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/next-inline.exp
@@ -0,0 +1,39 @@
+#   Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" "next-inline" \
+			  {next-inline.cc} \
+			  {debug nowarnings optimize=-O2}] } {
+    return -1
+}
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+gdb_test "bt" "\\s*\\#0\\s+main.*" "in main"
+gdb_test "step" ".*" "step into get_alias_set"
+gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline"
+gdb_test "next" ".*" "next step 1"
+gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 1"
+gdb_test "next" ".*" "next step 2"
+gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 2"
+gdb_test "next" ".*" "next step 3"
+gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 3"
+gdb_test "next" ".*" "next step 4"
+gdb_test "bt" "\\s*\\#0\\s+\[^\r\]*next-inline\\.cc:.*" "not in inline 4"
diff --git a/gdb/testsuite/gdb.cp/next-inline.h b/gdb/testsuite/gdb.cp/next-inline.h
new file mode 100644
index 0000000..99fb1b2
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/next-inline.h
@@ -0,0 +1,34 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+struct tree{
+  volatile int x;
+  volatile int z;
+};
+
+#define TREE_TYPE(NODE) (*tree_check (NODE, 0))
+
+inline tree *
+tree_check (tree *t, int i)
+{
+  if (t->x != i)
+    abort();
+  tree *x = t;
+  return x;
+}
-- 
1.9.1