[PATCH 2/2] gdb: fix missing print frame when stepping out of function

[Andrew Burgess <aburgess@redhat.com>]

From: Tom de Vries <tdevries@suse.de>

Consider test-case gdb.dwarf2/dw2-extend-inline-block.exp,
specifically the "contiguous block" prefix part, which can be stepped
through like this:

...
  $ gdb -q outputs/gdb.dwarf2/dw2-extend-inline-block/dw2-extend-inline-block-5
  Reading symbols from dw2-extend-inline-block-5...
  (gdb) start
  ... snip ...
  Temporary breakpoint 1, main () at dw2-extend-inline-block.c:35
  35	  /* main:2 */
  (gdb) s
  36	  /* main:3 */ foo ();				/* foo call line */
  (gdb)
  foo () at dw2-extend-inline-block.c:26
  26	  /* foo:1 */
  (gdb) s
  27	  /* foo:2 */
  (gdb) s
  28	  /* foo:3 */
  (gdb) s
  main () at dw2-extend-inline-block.c:37
  37	  /* main:4 */
  (gdb)
...

If we slightly modify the line program:

...
	DW_LNE_set_address main_4
+	DW_LNS_advance_line 1
	DW_LNS_copy

	DW_LNE_set_address main_5
-	DW_LNS_advance_line 1
	DW_LNS_negate_stmt
	DW_LNS_copy
...

we change the 36/0x401165 entry into a 37/0x401165 entry:

...
  File name                     Line number    Starting address    View    Stmt
  dw2-extend-inline-block.c              34            0x401116               x
  dw2-extend-inline-block.c              35            0x401129               x
  dw2-extend-inline-block.c              26            0x401138               x
  dw2-extend-inline-block.c              27            0x401147               x
  dw2-extend-inline-block.c              28            0x401156               x
  dw2-extend-inline-block.c              36            0x401156
 -dw2-extend-inline-block.c              36            0x401165
 +dw2-extend-inline-block.c              37            0x401165
  dw2-extend-inline-block.c              37            0x401174               x
  dw2-extend-inline-block.c              38            0x401183               x
  dw2-extend-inline-block.c              39            0x401192               x
  dw2-extend-inline-block.c              40            0x4011a1               x
  dw2-extend-inline-block.c               -            0x4011b7
...

As it happens, the fix to extend truncated inlined function blocks
doesn't work in this case.  This is PR gdb/33930.

We can work around this by making sure that the inlined function block
isn't truncated in the first place:

...
-			DW_AT_high_pc main_3 addr
+			DW_AT_high_pc main_4 addr
...

But then we still run into PR gdb/33981: the problem that gdb doesn't
notify us when stepping out of foo:

...
  (gdb) step^M
  28        /* foo:3 */^M
  (gdb) step^M
  37        /* main:4 */^M
  (gdb)
...

What happens is that the slightly different line program triggers a
different stepping path, which includes a case of "stepped to a
different frame, but it's not the start of a statement", which
refreshes the stepping info and consequently updates
tp->control.step_frame_id to the frame id of main.

So by the time we're stopped at line 37, and are trying to figure out
what to print in print_stop_location, this condition evaluates to
true:

...
      if (tp->control.stop_step
	  && (tp->control.step_frame_id
	      == get_frame_id (get_current_frame ()))
	  && (tp->control.step_start_function
	      == find_symbol_for_pc (tp->stop_pc ())))
...

and we get:

...
      /* Finished step in same frame and same file, just print source
	 line.  */
      source_flag = SRC_LINE;
...

It's good to realize here that because foo is inlined into main,
tp->control.step_start_function is not foo but main, so consequently
the step_start_function check (which checks if we are still in the
same function) also passes, even though we actually stepped from foo
into main.

The problem is the use of find_symbol_for_pc, this function
deliberately skips over inline frames and returns the symbol for the
innermost non-inline frame.

It might be tempting to think that we should switch to use
find_symbol_for_pc_sect_maybe_inline, which will return the symbol for
an inline frame, but this also has problems, specifically, attempting
this caused a regression in gdb.opt/inline-cmds.exp.  The previous
version of this patch, which showed the regression can be found here:

  https://inbox.sourceware.org/gdb-patches/20260331132342.1050954-1-tdevries@suse.de

At a given $pc the inferior might be reported as being within an
inline frame, or it might be reported as being in the containing
non-inline frame.  When the user performs a 'step' the
step_start_function needs to be set based on the function symbol of
the frame the inferior is actually reported in.  And we have a
function that gives us this information, get_frame_function.  So
instead of looking up the step_start_function based on the $pc value,
set it based on the frame.

Management of step_start_function has been moved into member functions
within thread_control_state, and step_start_function is made private.

Test gdb.dwarf2/dw2-extend-inline-block.exp is extended with the
additional test case described above.

Test gdb.opt/inline-cmds.exp is extended to better expose the
regression that was mentioned above when using
find_symbol_for_pc_sect_maybe_inline.  Previously the regression was
only exposed during part of the test that ran in MI mode.  I've
extended the test so the regression is also revealed in CLI mode.

Tested on x86_64-linux.

Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33930
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33981
---
 gdb/gdbthread.h                               | 36 ++++++++++++++-
 gdb/infcmd.c                                  |  3 +-
 gdb/infrun.c                                  | 12 +++--
 .../gdb.dwarf2/dw2-extend-inline-block.exp    | 45 +++++++++++++++++--
 gdb/testsuite/gdb.opt/inline-cmds.exp         | 33 +++++++++-----
 5 files changed, 104 insertions(+), 25 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 68d7aebbbf7..d221af89744 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -126,8 +126,36 @@ struct thread_control_state
   CORE_ADDR step_range_start = 0;	/* Inclusive */
   CORE_ADDR step_range_end = 0;		/* Exclusive */
 
-  /* Function the thread was in as of last it started stepping.  */
-  struct symbol *step_start_function = nullptr;
+  /* Set m_step_start_function according to FRAME.  */
+  void set_step_start_function (const frame_info_ptr &frame)
+  {
+    m_step_start_function = get_frame_function (frame);
+  }
+
+  /* Reset step_start_function.  */
+  void reset_step_start_function ()
+  {
+    m_step_start_function = nullptr;
+  }
+
+  /* Return true if the function symbol of FRAME matches
+     m_step_start_function.  */
+  bool in_step_start_function (const frame_info_ptr &frame) const
+  {
+    return m_step_start_function == get_frame_function (frame);
+  }
+
+  /* Return true if step_start_function is set.  */
+  bool step_start_function_p () const
+  {
+    return m_step_start_function != nullptr;
+  }
+
+  /* Return step_start_function.  */
+  const struct symbol *step_start_function () const
+  {
+    return m_step_start_function;
+  }
 
   /* If GDB issues a target step request, and this is nonzero, the
      target should single-step this thread once, and then continue
@@ -176,6 +204,10 @@ struct thread_control_state
 
   /* True if the thread is evaluating a BP condition.  */
   bool in_cond_eval = false;
+
+private:
+  /* Function the thread was in when it last started stepping.  */
+  const struct symbol *m_step_start_function = nullptr;
 };
 
 /* Inferior thread specific part of `struct infcall_suspend_state'.  */
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 98b45f884b1..17691633def 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -898,8 +898,7 @@ set_step_frame (thread_info *tp)
   symtab_and_line sal = find_frame_sal (frame);
   set_step_info (tp, frame, sal);
 
-  CORE_ADDR pc = get_frame_pc (frame);
-  tp->control.step_start_function = find_symbol_for_pc (pc);
+  tp->control.set_step_start_function (frame);
 }
 
 /* Step until outside of current statement.  */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index aa1c3553131..0af8c67d554 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3099,7 +3099,7 @@ clear_proceed_status_thread (struct thread_info *tp)
   tp->control.step_frame_id = null_frame_id;
   tp->control.step_stack_frame_id = null_frame_id;
   tp->control.step_over_calls = STEP_OVER_UNDEBUGGABLE;
-  tp->control.step_start_function = nullptr;
+  tp->control.reset_step_start_function ();
   tp->stop_requested = false;
 
   tp->control.stop_step = 0;
@@ -7751,9 +7751,9 @@ process_event_stop_test (struct execution_control_state *ecs)
   if (execution_direction != EXEC_REVERSE
       && ecs->event_thread->control.step_over_calls == STEP_OVER_UNDEBUGGABLE
       && in_solib_dynsym_resolve_code (ecs->event_thread->stop_pc ())
-      && (ecs->event_thread->control.step_start_function == nullptr
+      && (!ecs->event_thread->control.step_start_function_p ()
 	  || !in_solib_dynsym_resolve_code (
-	       ecs->event_thread->control.step_start_function->value_block ()
+	       ecs->event_thread->control.step_start_function ()->value_block ()
 		->entry_pc ())))
     {
       CORE_ADDR pc_after_resolver =
@@ -7877,8 +7877,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 	   == ecs->event_thread->control.step_stack_frame_id)
 	  && ((ecs->event_thread->control.step_stack_frame_id
 	       != outer_frame_id)
-	      || (ecs->event_thread->control.step_start_function
-		  != find_symbol_for_pc (ecs->event_thread->stop_pc ())))))
+	      || !ecs->event_thread->control.in_step_start_function (frame))))
     {
       CORE_ADDR stop_pc = ecs->event_thread->stop_pc ();
       CORE_ADDR real_stop_pc;
@@ -9365,8 +9364,7 @@ print_stop_location (const target_waitstatus &ws)
       if (frame_info_ptr frame = get_current_frame ();
 	  tp->control.stop_step
 	  && (tp->control.step_frame_id == get_frame_id (frame))
-	  && (tp->control.step_start_function
-	      == find_symbol_for_pc (tp->stop_pc ())))
+	  && tp->control.in_step_start_function (frame))
 	{
 	  symtab_and_line sal = find_frame_sal (frame);
 	  if (sal.symtab != tp->current_symtab)
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-extend-inline-block.exp b/gdb/testsuite/gdb.dwarf2/dw2-extend-inline-block.exp
index 9e4798b53f3..481dfbdd134 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-extend-inline-block.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-extend-inline-block.exp
@@ -52,8 +52,11 @@ get_func_info main
 # Create DWARF for the test.  In this case, inline function 'foo' is created
 # with a contiguous address range that needs extending.
 
-proc build_dwarf_for_contiguous_block { asm_file } {
+proc build_dwarf_for_contiguous_block { asm_file {range_correct 0} {variant 0} } {
     Dwarf::assemble $asm_file {
+	upvar range_correct range_correct
+	upvar variant variant
+
 	declare_labels lines_table inline_func
 
 	cu { } {
@@ -83,7 +86,11 @@ proc build_dwarf_for_contiguous_block { asm_file } {
 			DW_AT_call_file 1 data1
 			DW_AT_call_line $::foo_call_line data1
 			DW_AT_low_pc main_1 addr
-			DW_AT_high_pc main_3 addr
+			if {$range_correct} {
+			    DW_AT_high_pc main_4 addr
+			} else {
+			    DW_AT_high_pc main_3 addr
+			}
 		    }
 		}
 	    }
@@ -120,10 +127,15 @@ proc build_dwarf_for_contiguous_block { asm_file } {
 		DW_LNS_copy
 
 		DW_LNE_set_address main_4
+		if {$variant == 1} {
+		    DW_LNS_advance_line 1
+		}
 		DW_LNS_copy
 
 		DW_LNE_set_address main_5
-		DW_LNS_advance_line 1
+		if {$variant == 0} {
+		    DW_LNS_advance_line 1
+		}
 		DW_LNS_negate_stmt
 		DW_LNS_copy
 
@@ -146,6 +158,22 @@ proc build_dwarf_for_contiguous_block { asm_file } {
     }
 }
 
+# Like build_dwarf_for_contiguous_block, but use a slightly different line
+# info by setting variant == 1.
+# Use range_correct 1, so we're not testing the fix for PR33930.
+
+proc build_dwarf_for_contiguous_block_2 { asm_file } {
+    return [build_dwarf_for_contiguous_block $asm_file 1 1]
+}
+
+# Like build_dwarf_for_contiguous_block, but use a slightly different line
+# info by setting variant == 1.
+# Use range_correct 0, so we're testing the fix for PR33930.
+
+proc build_dwarf_for_contiguous_block_3 { asm_file } {
+    return [build_dwarf_for_contiguous_block $asm_file 0 1]
+}
+
 # Assuming GDB is stopped at the entry $pc for 'foo', use 'maint info
 # blocks' to check the block for 'foo' is correct.  This function checks
 # 'foo' created by 'build_dwarf_for_contiguous_block'.
@@ -555,6 +583,12 @@ set test_list \
 	 [list "contiguous block" \
 	      build_dwarf_for_contiguous_block \
 	      check_contiguous_block] \
+	 [list "contiguous block 2" \
+	      build_dwarf_for_contiguous_block_2 \
+	      check_contiguous_block] \
+	 [list "contiguous block 3" \
+	      build_dwarf_for_contiguous_block_3 \
+	      check_contiguous_block] \
 	]
 
 # Run all the tests.
@@ -566,6 +600,11 @@ foreach test_spec $test_list {
     set build_dwarf_func [lindex $test_spec 1]
     set check_block_func [lindex $test_spec 2]
 
+    if {$build_dwarf_func == "build_dwarf_for_contiguous_block_3"} {
+	# Work around PR gdb/33930.
+	continue
+    }
+
     with_test_prefix $prefix {
 	set asm_file [standard_output_file ${testfile}-${suffix}.S]
 	$build_dwarf_func $asm_file
diff --git a/gdb/testsuite/gdb.opt/inline-cmds.exp b/gdb/testsuite/gdb.opt/inline-cmds.exp
index 12db0709fe6..9bfc32f58b3 100644
--- a/gdb/testsuite/gdb.opt/inline-cmds.exp
+++ b/gdb/testsuite/gdb.opt/inline-cmds.exp
@@ -176,24 +176,35 @@ if { $bt_test == 0 } {
 # "stop" at the call sites before entering them.
 runto_main
 
-set msg "step into func1"
-set saw_call_site 0
-gdb_test_multiple "list" $msg {
-    -re "($first|$opt).*$gdb_prompt $" {
+set saw_call_site false
+set saw_main_frame false
+# Don't send the command using gdb_test_multiple, as it will be
+# injected into the regexp that start with '^' and use '-wrap', and we
+# send a different command using 'send_gdb' within the gdb_test_multiple.
+send_gdb "list\r"
+gdb_test_multiple "" "step into func1" {
+    -re "^(list|step)\r\n" {
+	exp_continue
+    }
+    -re -wrap "^\[^\r\n\]*($first|$opt)\[^\r\n\]*" {
 	send_gdb "step\r"
 	exp_continue
     }
-    -re "result = func1.*$gdb_prompt $" {
-	set saw_call_site 1
+    -re -wrap "^\[^\r\n\]*y = 8\[^\r\n\]*" {
+	send_gdb "step\r"
+	exp_continue
+    }
+    -re "^main \\(\\) at \[^\r\n\]+\r\n" {
+	set saw_main_frame true
+	exp_continue
+    }
+    -re -wrap "^\[^\r\n\]*result = func1 \\(\\);" {
+	set saw_call_site true
 	send_gdb "step\r"
 	exp_continue
     }
     -re "func1 \\\(\\\) at .*\r\n$decimal.*bar \\\(\\\);\r\n$gdb_prompt $" {
-	if { $saw_call_site } {
-	    pass $msg
-	} else {
-	    fail $msg
-	}
+	gdb_assert { $saw_call_site && !$saw_main_frame } $gdb_test_name
     }
 }
 
-- 
2.25.4