[PATCHv6] Make "skip" work on inline frames

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

[-- Attachment #1: Type: text/plain, Size: 3027 bytes --]

On 12/15/19 1:46 AM, Simon Marchi wrote:
> On 2019-12-02 11:47 a.m., Bernd Edlinger wrote:
>> On 12/2/19 3:34 AM, Simon Marchi wrote:
>>> On 2019-11-24 6:22 a.m., Bernd Edlinger wrote:
>>>> This is just a minor update on the patch
>>>> since the function SYMBOL_PRINT_NAME was removed with
>>>> commit 987012b89bce7f6385ed88585547f852a8005a3f
>>>> I replaced it with sym->print_name (), otherwise the
>>>> patch is unchanged.
>>>
>>> Hi Bernd,
>>>
>>> Sorry, I had lost this in the mailing list noise.
>>>
>>> I played a bit with the patch and different cases of figure.  I am not able to understand
>>> the purpose of each of your changes (due to the complexity of that particular code), but
>>> I didn't find anything that stood out as wrong to me.  Pedro might be able to do a more
>>> in-depth review of the event handling code.
>>>
>>> If the test tests specifically skipping of inline functions, I'd name it something more
>>> descriptive than "skip2.exp", maybe "skip-inline.exp"?
>>>
>>> Unfortunately, your test doesn't pass on my computer (gcc 9.2.0), but neither does the
>>> gdb.base/skip.exp.  I am attaching the gdb.log when running your test, if it can help.
>>>
>>> Simon
>>>
>>
>> Hi Simon,
>>
>> I only tested that with gcc-4.8, and both test cases worked with that gcc version.
>>
>> I tried now with gcc-trunk version from a few days ago, and I think I see
>> what you mean.
>>
>> skip2.c (now skip-inline.c) can be fixed by removing the assignment
>> to x in the first line, which is superfluous (and copied from skip.c).
>> But skip.c cannot be fixed this way.  I only see a chance to allow
>> the stepping back to main and then to foo happen.
>>
>> Does this modified test case work for you?
>>
>>
>>
>> Thanks
>> Bernd.
>>
> 
> Hi Bernd,
> 
> Thanks for fixing the skip.exp test at the same time.  I'd prefer if this was done as a
> separate patch though, since it's an issue separate from the inline stepping issue you
> were originally tackling.

Okay, I split that out as a separate patch now.

> 
> So the patch looks good to me if you remove those bits, and fix the following nits:
> 
> - Remove "load_lib completion-support.exp" from the test.
> - The indentation in the .exp should use tabs for multiple of 8 columns, instead of just spaces (like you did in the .c).
> 

Done.  Also added changelog text, which I forgot previously.

> A comment I would have on the bits in skip.exp:
> 
>     # with recent gcc we jump once back to main before entering foo here
>     # if that happens try to step a second time
>     gdb_test "step" "foo \\(\\) at.*" "step 3" "main \\(\\) at .*" "step"
> 
> It's usually not helpful to say "with recent gcc", since it doesn't mean much, especially
> when reading this 10 years from now.  Instead, mention the specific gcc version this was
> observed with.  Also, begin the sentence with a capital letter and finish with a period.
> 

Done.


Is it OK for trunk?


Thanks
Bernd.

[-- Attachment #2: changelog.txt --]
[-- Type: text/plain, Size: 420 bytes --]

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

	* infcmd.c (prepare_one_step): Step over skipped inline functions.
	* infrun.c (inline_frame_is_marked_for_skip): New helper function.
	(process_event_stop_test): Keep stepping over skipped inline functions.

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

	* gdb.base/skip-inline.c: New file.
	* gdb.base/skip-inline.exp: New file.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch --]
[-- Type: text/x-patch; name="0001-Check-all-inline-frames-if-they-are-marked-for-skip.patch", Size: 4454 bytes --]

From ed6be155e5cbf560b2fc20735fbe8f5b37271e75 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Fri, 18 Oct 2019 14:28:45 +0200
Subject: [PATCH 1/2] Check all inline frames if they are marked for skip

This makes the skip command work in optimized builds,
where skipped functions may be inlined.
Previously that was only working when stepping into
a non-inlined function.
---
 gdb/infcmd.c | 20 ++++++++++++++++++--
 gdb/infrun.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 2a25346..af66eaf 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -52,6 +52,7 @@
 #include "thread-fsm.h"
 #include "top.h"
 #include "interps.h"
+#include "skip.h"
 #include "gdbsupport/gdb_optional.h"
 #include "source.h"
 #include "cli/cli-style.h"
@@ -1106,14 +1107,29 @@ prepare_one_step (struct step_command_fsm *sm)
 	      && inline_skipped_frames (tp))
 	    {
 	      ptid_t resume_ptid;
+	      const char *fn = NULL;
+	      symtab_and_line sal;
+	      struct symbol *sym;
 
 	      /* Pretend that we've ran.  */
 	      resume_ptid = user_visible_resume_ptid (1);
 	      set_running (resume_ptid, 1);
 
 	      step_into_inline_frame (tp);
-	      sm->count--;
-	      return prepare_one_step (sm);
+
+	      frame = get_current_frame ();
+	      sal = find_frame_sal (frame);
+	      sym = get_frame_function (frame);
+
+	      if (sym != NULL)
+		fn = sym->print_name ();
+
+	      if (sal.line == 0
+		  || !function_name_is_marked_for_skip (fn, sal))
+		{
+		  sm->count--;
+		  return prepare_one_step (sm);
+		}
 	    }
 
 	  pc = get_frame_pc (frame);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6a346d6..7ddd21d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4034,6 +4034,45 @@ stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
   return 0;
 }
 
+/* Look for an inline frame that is marked for skip.
+   If PREV_FRAME is TRUE start at the previous frame,
+   otherwise start at the current frame.  Stop at the
+   first non-inline frame, or at the frame where the
+   step started.  */
+
+static bool
+inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
+{
+  struct frame_info *frame = get_current_frame ();
+
+  if (prev_frame)
+    frame = get_prev_frame (frame);
+
+  for (; frame != NULL; frame = get_prev_frame (frame))
+    {
+      const char *fn = NULL;
+      symtab_and_line sal;
+      struct symbol *sym;
+
+      if (frame_id_eq (get_frame_id (frame), tp->control.step_frame_id))
+	break;
+      if (get_frame_type (frame) != INLINE_FRAME)
+	break;
+
+      sal = find_frame_sal (frame);
+      sym = get_frame_function (frame);
+
+      if (sym != NULL)
+	fn = sym->print_name ();
+
+      if (sal.line != 0
+	  && function_name_is_marked_for_skip (fn, sal))
+	return true;
+    }
+
+  return false;
+}
+
 /* If the event thread has the stop requested flag set, pretend it
    stopped for a GDB_SIGNAL_0 (i.e., as if it stopped due to
    target_stop).  */
@@ -6524,7 +6563,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
 	if (tmp_sal.line != 0
 	    && !function_name_is_marked_for_skip (ecs->stop_func_name,
-						  tmp_sal))
+						  tmp_sal)
+	    && !inline_frame_is_marked_for_skip (true, ecs->event_thread))
 	  {
 	    if (execution_direction == EXEC_REVERSE)
 	      handle_step_into_function_backward (gdbarch, ecs);
@@ -6690,7 +6730,14 @@ process_event_stop_test (struct execution_control_state *ecs)
 
 	  if (call_sal.line == ecs->event_thread->current_line
 	      && call_sal.symtab == ecs->event_thread->current_symtab)
-	    step_into_inline_frame (ecs->event_thread);
+	    {
+	      step_into_inline_frame (ecs->event_thread);
+	      if (inline_frame_is_marked_for_skip (false, ecs->event_thread))
+		{
+		  keep_going (ecs);
+		  return;
+		}
+	    }
 
 	  end_stepping_range (ecs);
 	  return;
@@ -6724,7 +6771,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	fprintf_unfiltered (gdb_stdlog,
 			    "infrun: stepping through inlined function\n");
 
-      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL)
+      if (ecs->event_thread->control.step_over_calls == STEP_OVER_ALL
+	  || inline_frame_is_marked_for_skip (false, ecs->event_thread))
 	keep_going (ecs);
       else
 	end_stepping_range (ecs);
-- 
1.9.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-Add-a-test-case-for-skip-with-inlined-functions.patch --]
[-- Type: text/x-patch; name="0002-Add-a-test-case-for-skip-with-inlined-functions.patch", Size: 5368 bytes --]

From 7bee2af24e4f21ae449f604ac549492d5fcab1a4 Mon Sep 17 00:00:00 2001
From: Bernd Edlinger <bernd.edlinger@hotmail.de>
Date: Wed, 30 Oct 2019 21:35:22 +0100
Subject: [PATCH 2/2] Add a test case for skip with inlined functions

---
 gdb/testsuite/gdb.base/skip-inline.c   | 64 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/skip-inline.exp | 78 ++++++++++++++++++++++++++++++++++
 2 files changed, 142 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/skip-inline.c
 create mode 100644 gdb/testsuite/gdb.base/skip-inline.exp

diff --git a/gdb/testsuite/gdb.base/skip-inline.c b/gdb/testsuite/gdb.base/skip-inline.c
new file mode 100644
index 0000000..e562149
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip-inline.c
@@ -0,0 +1,64 @@
+/* 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>
+
+int bar (void);
+int baz (int);
+void skip1_test_skip_file_and_function (void);
+void test_skip_file_and_function (void);
+
+__attribute__((__always_inline__)) static inline int
+foo (void)
+{
+  return bar ();
+}
+
+int
+main ()
+{
+  volatile int x;
+
+  /* step immediately into the inlined code */
+  baz (foo ());
+
+  /* step first over non-inline code, this involves a different code path */
+  x = 0; x = baz (foo ());
+
+  test_skip_file_and_function ();
+
+  return 0;
+}
+
+static void
+test_skip (void)
+{
+}
+
+static void
+end_test_skip_file_and_function (void)
+{
+  abort ();
+}
+
+void
+test_skip_file_and_function (void)
+{
+  test_skip ();
+  skip1_test_skip_file_and_function ();
+  end_test_skip_file_and_function ();
+}
diff --git a/gdb/testsuite/gdb.base/skip-inline.exp b/gdb/testsuite/gdb.base/skip-inline.exp
new file mode 100644
index 0000000..4ab9ecf
--- /dev/null
+++ b/gdb/testsuite/gdb.base/skip-inline.exp
@@ -0,0 +1,78 @@
+#   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" "skip-inline" \
+			  {skip-inline.c skip1.c } \
+			  {debug nowarnings}] } {
+    return -1
+}
+
+set srcfile skip-inline.c
+set srcfile1 skip1.c
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+# Create a skiplist entry for a specified file and function.
+
+gdb_test "skip function foo" "Function foo will be skipped when stepping\."
+
+gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+gdb_test "step" ".*" "step into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "in the baz, since foo was skipped"
+gdb_test "step" ".*" "step in the baz"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+gdb_test "step" ".*" "step back to main"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+gdb_test "step" ".*" "step again into baz, since foo will be skipped"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+gdb_test "step" ".*" "step in the baz, again"
+gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz, again"
+gdb_test "step" ".*" "step back to main, again"
+gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+with_test_prefix "double step" {
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+    gdb_test "step 2" ".*" "step into baz, since foo will be skipped"
+    gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
+    gdb_test "step" ".*" "step back to main"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+    gdb_test "step 2" ".*" "step again into baz, since foo will be skipped"
+    gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
+    gdb_test "step" ".*" "step back to main, again"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
+
+if ![runto_main] {
+    fail "can't run to main"
+    return
+}
+
+with_test_prefix "triple step" {
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
+    gdb_test "step 3" ".*" "step over baz"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
+    gdb_test "step 3" ".*" "step over baz, again"
+    gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
+}
-- 
1.9.1