[3/3] gdb: Fix 'next' skipping over remainder of inline function

Message ID 20240201152118.598375-4-tlloyddavies@undo.io
State New
Headers
Series Fix inline function stepping |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 fail Testing failed
linaro-tcwg-bot/tcwg_gdb_check--master-arm fail Testing failed

Commit Message

Toby Lloyd Davies Feb. 1, 2024, 3:21 p.m. UTC
  An inline function's instructions are often interleaved with it's
callers. When stepping out of the inline function and then back into it
again we would treat this as stepping into a new inline function. Thus a
'next' command from within a inline function could skip over the rest of
the inline function.

This occurred because the step_frame_id was refreshed at the end of
process_event_stop_test when it detected that we had changed frame. Fix
this by refreshing step info only when the stack frame has changed.
However, not refreshing the step info when we step out of an inline
frame poses a problem if we subsequently step into a different inline
frame. The code wouldn't be able to determine whether we had stepped
into or out to this inline function, and thus whether a 'next' command
should skip it. If that inline function was in the backtrace when we
started stepping then we don't want to skip it but if it was not in the
backtrace then we want to skip it.

Therefore, to correctly determine when we have stepped into an inline frame
requires knowledge of every inline frame that was being stepped through
when the command was issued. So store these inline frame ids in a
vector. Then we can treat us as having stepped into an inline function
if we can't find the current frame in the vector and we haven't stepped
into a different stack frame.

Additionally, this fixes another bug. When stepping out of an inline
function and immediately entering into another inline function we would
not detect this as stepping into an inline function and thus a 'next'
command wouldn't step over the new inline function. Note that in this
case we would normally detect this as the entry point to the inline
function and create a inline callsite here, but if we didn't detect this
as an entry point then we would incorrectly step into the function.
---
 gdb/gdbthread.h                               |   6 +
 gdb/infrun.c                                  |  57 +++---
 .../gdb.dwarf2/dw2-inline-stepping-4.c        |  59 +++++++
 .../gdb.dwarf2/dw2-inline-stepping-4.exp      | 167 ++++++++++++++++++
 4 files changed, 260 insertions(+), 29 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-stepping-4.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-inline-stepping-4.exp
  

Patch

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index e7035d40ad4..ddfd4ecc965 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -146,6 +146,12 @@  struct thread_control_state
      any inlined frames).  */
   struct frame_id step_stack_frame_id {};
 
+  /* Frame IDs of inline frames as of when stepping command was issued.
+     From newest inline frame until newest non-inline frame
+     (exclusive). This is how we know when we step into an inlined
+     subroutine call.  */
+  std::vector<struct frame_id> step_inline_frame_ids {};
+
   /* True if the the thread is presently stepping over a breakpoint or
      a watchpoint, either with an inline step over or a displaced (out
      of line) step, and we're now expecting it to report a trap for
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 927e464e479..845341b8fff 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4780,6 +4780,13 @@  set_step_info (thread_info *tp, frame_info_ptr frame,
 
   tp->control.step_frame_id = get_frame_id (frame);
   tp->control.step_stack_frame_id = get_stack_frame_id (frame);
+  tp->control.step_inline_frame_ids.clear ();
+  for (frame_info_ptr f = frame;
+       f && get_frame_type (f) == INLINE_FRAME;
+       f = get_prev_frame (f))
+    {
+      tp->control.step_inline_frame_ids.push_back (get_frame_id (f));
+    }
 
   tp->current_symtab = sal.symtab;
   tp->current_line = sal.line;
@@ -5002,23 +5009,6 @@  adjust_pc_after_break (struct thread_info *thread,
     }
 }
 
-static bool
-stepped_in_from (frame_info_ptr frame, struct frame_id step_frame_id)
-{
-  for (frame = get_prev_frame (frame);
-       frame != nullptr;
-       frame = get_prev_frame (frame))
-    {
-      if (get_frame_id (frame) == step_frame_id)
-	return true;
-
-      if (get_frame_type (frame) != INLINE_FRAME)
-	break;
-    }
-
-  return false;
-}
-
 /* 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
@@ -5039,11 +5029,16 @@  inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
       symtab_and_line sal;
       struct symbol *sym;
 
-      if (get_frame_id (frame) == tp->control.step_frame_id)
-	break;
       if (get_frame_type (frame) != INLINE_FRAME)
 	break;
 
+      /* Don't skip frames we were already stepping through. */
+      if (std::find (tp->control.step_inline_frame_ids.begin (),
+		     tp->control.step_inline_frame_ids.end (),
+		     get_frame_id (frame))
+	  != tp->control.step_inline_frame_ids.end ())
+	break;
+
       sal = find_frame_sal (frame);
       sym = get_frame_function (frame);
 
@@ -8091,14 +8086,17 @@  process_event_stop_test (struct execution_control_state *ecs)
     }
 
   /* Look for "calls" to inlined functions, part one.  If we are still
-     in the same real function we were stepping through, but we have
-     to go further up to find the exact frame ID, we are stepping
+     in the same real function we were stepping through, but we were not
+     previously stepping through this inline function, we are stepping
      through a more inlined call beyond its call site.  */
 
   if (get_frame_type (get_current_frame ()) == INLINE_FRAME
-      && (*curr_frame_id != original_frame_id)
-      && stepped_in_from (get_current_frame (),
-			  ecs->event_thread->control.step_frame_id))
+      && (std::find (ecs->event_thread->control.step_inline_frame_ids.begin (),
+		    ecs->event_thread->control.step_inline_frame_ids.end (),
+		    *curr_frame_id)
+	   == ecs->event_thread->control.step_inline_frame_ids.end ())
+      && (get_stack_frame_id (get_current_frame ())
+	  == ecs->event_thread->control.step_stack_frame_id))
     {
       infrun_debug_printf ("stepping through inlined function");
 
@@ -8208,10 +8206,11 @@  process_event_stop_test (struct execution_control_state *ecs)
 	  end_stepping_range (ecs);
 	  return;
 	}
-      else if (*curr_frame_id == original_frame_id)
+      else if (get_stack_frame_id (get_current_frame ())
+	       == ecs->event_thread->control.step_stack_frame_id)
 	{
 	  /* We are not at the start of a statement, and we have not changed
-	     frame.
+	     stack frame.
 
 	     We ignore this line table entry, and continue stepping forward,
 	     looking for a better place to stop.  */
@@ -8221,15 +8220,15 @@  process_event_stop_test (struct execution_control_state *ecs)
 	}
       else
 	{
-	  /* We are not the start of a statement, and we have changed frame.
+	  /* We are not the start of a statement, and we have changed stack frame.
 
 	     We ignore this line table entry, and continue stepping forward,
 	     looking for a better place to stop.  Keep refresh_step_info at
-	     true to note that the frame has changed, but ignore the line
+	     true to note that the stack frame has changed, but ignore the line
 	     number to make sure we don't ignore a subsequent entry with the
 	     same line number.  */
 	  stop_pc_sal.line = 0;
-	  infrun_debug_printf ("stepped to a different frame, but "
+	  infrun_debug_printf ("stepped to a different stack frame, but "
 			       "it's not the start of a statement");
 	}
     }
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping-4.c b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping-4.c
new file mode 100644
index 00000000000..3097fed5131
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping-4.c
@@ -0,0 +1,59 @@ 
+/* Copyright 2019-2024 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the FOOU 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
+   FOOU General Public License for more details.
+
+   You should have received a copy of the FOOU General Public License
+   along with this program.  If not, see <http://www.foou.org/licenses/>.  */
+
+/* This test relies on foo_a, foo_b and bar being inlined into main */
+
+volatile int global_var;
+
+static inline void  __attribute__ ((always_inline))
+foo_a ()
+{
+  asm ("foo_label: .globl foo_label");
+  global_var++;					/* foo inc global_var */
+}
+
+static inline void  __attribute__ ((always_inline))
+foo_b ()
+{
+  asm ("foo_label2: .globl foo_label2");
+  asm ("nop");                                  /* foo nop */
+  asm ("foo_label3: .globl foo_label3");
+  asm ("jmp bar_label2");                       /* foo jmp bar */
+}						/* foo end */
+
+static inline void  __attribute__ ((always_inline))
+bar ()
+{
+  asm ("bar_label: .globl bar_label");
+  asm ("nop");                                  /* bar nop */
+  asm ("bar_label2: .globl bar_label2");
+  global_var++;					/* bar inc global_var */
+}						/* bar end */
+
+int
+main ()
+{						/* main prologue */
+  asm ("main_label: .globl main_label");
+  global_var = 0;				/* main set global_var */
+  asm ("main_label2: .globl main_label2");
+  foo_a ();			                /* main call foo */
+  asm ("main_label3: .globl main_label3");
+  asm ("jmp foo_label3");                       /* main jmp foo */
+  foo_b ();
+  asm ("main_label4: .globl main_label4");
+  bar();                                        /* main call bar */
+  asm ("main_label5: .globl main_label5");
+  return global_var;                            /* main return global_var */
+}						/* main end */
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping-4.exp b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping-4.exp
new file mode 100644
index 00000000000..6abba731c88
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-stepping-4.exp
@@ -0,0 +1,167 @@ 
+# Copyright 2019-2024 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/>.
+#
+# Inline functions often have instructions interleaved with their
+# callers. Test that when the 'next' command steps out of an inline
+# function and then back into it again we do not skip the rest of the
+# inline function.
+#
+# Additionally, test that when the 'next' command steps out of one
+# inline function and immediately enters another inline function
+# (without going to the callsite), we skip over the inline function.
+#
+# The testfile uses jumps into inline functions so that we do not stop
+# at the inline callsite lines.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+require dwarf2_support
+
+# The .c files use __attribute__.
+require is_c_compiler_gcc
+
+standard_testfile .c .S
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile srcfile2
+    declare_labels cu_ranges_label foo_ranges_label lines_label bar_prog foo_prog
+
+    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
+	main_start main_len
+    set main_end "$main_start + $main_len"
+
+    set bar_call_line [gdb_get_line_number "main call bar"]
+    set foo_call_line [gdb_get_line_number "main call foo"]
+
+    cu {} {
+	compile_unit {
+	    {language @DW_LANG_C}
+	    {name dw2-inline-stepping-2.c}
+	    {low_pc 0 addr}
+	    {stmt_list ${lines_label} DW_FORM_sec_offset}
+	    {ranges ${cu_ranges_label} DW_FORM_sec_offset}
+	} {
+	    bar_prog: subprogram {
+		{name bar}
+		{inline 3 data1}
+	    }
+	    foo_prog: subprogram {
+		{name bar}
+		{inline 3 data1}
+	    }
+	    subprogram {
+		{external 1 flag}
+		{name main}
+		{low_pc $main_start addr}
+		{high_pc "$main_start + $main_len" addr}
+	    } {
+		inlined_subroutine {
+		    {abstract_origin %$foo_prog}
+			{ranges ${foo_ranges_label} DW_FORM_sec_offset}
+		    {call_file 1 data1}
+		    {call_line $foo_call_line data1}
+		}
+		inlined_subroutine {
+		    {abstract_origin %$bar_prog}
+		    {low_pc bar_label addr}
+		    {high_pc main_label5 addr}
+		    {call_file 1 data1}
+		    {call_line $bar_call_line data1}
+		}
+	    }
+	}
+    }
+
+    lines {version 2} lines_label {
+	include_dir "${srcdir}/${subdir}"
+	file_name "$srcfile" 1
+
+	program {
+
+	    DW_LNE_set_address main_label
+	    line [gdb_get_line_number "main set global_var"]
+	    DW_LNS_copy
+
+	    DW_LNE_set_address main_label2
+	    line [gdb_get_line_number "main call foo"]
+	    DW_LNS_copy
+
+	    DW_LNE_set_address foo_label
+	    line [gdb_get_line_number "foo inc global_var"]
+	    DW_LNS_copy
+
+	    DW_LNE_set_address main_label3
+	    line [gdb_get_line_number "main jmp foo"]
+	    DW_LNS_negate_stmt
+		DW_LNS_copy
+	    DW_LNS_negate_stmt
+
+
+	    DW_LNE_set_address foo_label2
+	    line [gdb_get_line_number "bar nop"]
+	    DW_LNS_copy
+
+	    DW_LNE_set_address foo_label3
+	    line [gdb_get_line_number "foo jmp bar"]
+	    DW_LNS_copy
+
+		DW_LNE_set_address main_label4
+	    line [gdb_get_line_number "main call bar"]
+	    DW_LNS_copy
+
+		DW_LNE_set_address bar_label
+	    line [gdb_get_line_number "bar nop"]
+	    DW_LNS_copy
+
+		DW_LNE_set_address bar_label2
+	    line [gdb_get_line_number "bar inc global_var"]
+	    DW_LNS_copy
+
+	    DW_LNE_set_address main_label5
+	    line [gdb_get_line_number "main return global_var"]
+	    DW_LNS_copy
+	    DW_LNE_set_address $main_end
+	    DW_LNE_end_sequence
+	}
+    }
+
+    ranges {is_64 [is_64_target]} {
+	cu_ranges_label: sequence {
+	    range ${main_start} ${main_end}
+	}
+	foo_ranges_label: sequence {
+		range {foo_label} {main_label3}
+		range {foo_label2} {main_label4}
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "step" ".*main call foo.*" "step to foo callsite"
+gdb_test "step" ".*foo inc global_var.*" "step into foo"
+
+gdb_test "next" ".foo jmp bar.*" "next within foo"
+
+gdb_test "next" ".*return global_var.*" "next over bar"