[RFC v2 3/3] [gdb] Fix missing print frame when stepping out of function

[Tom de Vries <tdevries@suse.de>]

[ This is an RFC.  The patch fixes one problem, but introduces another
problem.  The point of the RFC is the analysis of the second problem. ]

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
  ...
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 PR33930.

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 PR33981: 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.

Fix this by using find_symbol_for_pc_maybe_inline instead of find_symbol_for_pc
in set_step_start_function and in_step_start_function.

Doing so however regresses gdb.opt/inline-cmds.exp.

To understand that regression, let's first investigate the behavior without
this patch.

We step from function main (lines 64/66) into function func1 (35):
...
$ gdb -q outputs/gdb.opt/inline-cmds/inline-cmds -ex "tbreak 64" -ex run
  ...
Temporary breakpoint 1, main () at inline-cmds.c:64
64	  y = 8; /* set mi break here */
(gdb) step
66	  result = func1 ();
(gdb) step
func1 () at inline-cmds.c:35
35	  bar ();
(gdb)
...

This seems trivial, but it's not.

If we list the effects of the two steps in terms of line number and pc, we get:
- 64/0x401124
- step
- 66/0x40112e
- step
- 35/0x40112e

We're initially stopped at pc 0x401124, and after the first step stopped at pc
0x40112e.

Effectively we've entered the inlined function func1 at this point, both
according to the line number info (where 0x40112e maps to line 35):
...
Line number    Starting address    View    Stmt
	 60            0x401116               x
	 63            0x40111a               x
	 64            0x401124               x
	 35            0x40112e               x
	 36            0x401133               x
...
and to the debug info:
...
 <2><395>: Abbrev Number: 1 (DW_TAG_inlined_subroutine)
    <396>   DW_AT_abstract_origin: <0x4b7>
    <39a>   DW_AT_low_pc      : 0x40112e
    <3a2>   DW_AT_high_pc     : 0x14
    <3aa>   DW_AT_call_file   : 1
    <3aa>   DW_AT_call_line   : 66
    <3ab>   DW_AT_call_column : 12
 <1><4b7>: Abbrev Number: 5 (DW_TAG_subprogram)
    <4b8>   DW_AT_external    : 1
    <4b8>   DW_AT_name        : func1
    <4bc>   DW_AT_decl_file   : 1
    <4bc>   DW_AT_decl_line   : 33
    <4bd>   DW_AT_decl_column : 17
    <4bd>   DW_AT_prototyped  : 1
    <4bd>   DW_AT_type        : <0x30b>
    <4c1>   DW_AT_inline      : 3       (declared as inline and inlined)
...

However, we're going to pretend that we're still in main at the call site at
line 66, and require another step to land at line 35.

This works as follows.

When stopping at pc 0x40112e, find_frame_sal is called, where this code:
...
  if (frame_inlined_callees (frame) > 0)
    {
      const symbol *sym;

      /* If the current frame has some inlined callees, and we have a next
	 frame, then that frame must be an inlined frame.  In this case
	 this frame's sal is the "call site" of the next frame's inlined
	 function, which can not be inferred from get_frame_pc.  */
      next_frame = get_next_frame (frame);
      if (next_frame)
	sym = get_frame_function (next_frame);
      else
	sym = inline_skipped_symbol (inferior_thread ());
...
selects the line number for the call site, 66.

Then we're going to decide what to print in print_stop_location, and decide
that since the step started in main, and stopped in main, there no need to
print anything else than the line, and so we get:
...
(gdb) step
66	  result = func1 ();
(gdb)
...

When executing the second step, prepare_one_step is called, which hits this
clause:
...
          /* Step at an inlined function behaves like "down".  */
          if (!sm->skip_subroutines
              && inline_skipped_frames (tp))
            {
              ...
              /* Pretend that we've ran.  */
...
and consequently the pc stays the same.

Instead, step_into_inline_frames make a change in the "inline frame state" of
the thread, so the next time find_frame_sal is called, the
"frame_inlined_callees (frame) > 0" condition no longer triggers, and line 35
is selected.

Then when deciding what to print in print_stop_location, the comparison of the
frame ids fails, so SRC_AND_LOC is selected and we get:
...
(gdb) step
func1 () at inline-cmds.c:35
35	  bar ();
(gdb)
...

Now with this patch, instead we get:
...
$ gdb -q outputs/gdb.opt/inline-cmds/inline-cmds -ex "tbreak 64" -ex run
  ...
Temporary breakpoint 1, main () at inline-cmds.c:64
64	  y = 8; /* set mi break here */
(gdb) s
main () at inline-cmds.c:66
66	  result = func1 ();
(gdb) s
func1 () at inline-cmds.c:35
35	  bar ();
(gdb)
...

The problem is that when stepping from line 64 to 66, we seem to be entering
main, while that's not the case, because we were already in main.

So how does that happen?

This time, when deciding what to print in print_stop_location, we observe
that we started the step in main, but are stopped in func1 (without the patch,
we would have observed that we're stopped in main):
...
(gdb) p m_step_start_function.m_name
$12 = 0x33cc351 "main"
(gdb) p find_symbol_for_pc_maybe_inline (pc).m_name
$13 = 0x33cc2e1 "func1"
(gdb)
...
so we get SRC_AND_LOC.

Then when doing the actual printing, the infrastructure does look at the
"inline frame state" and concludes that we're actually in main, and so we get:
...
(gdb) s
main () at inline-cmds.c:66
66	  result = func1 ();
(gdb)
...

In short, AFAIU the problem is that our proposed fix doesn't take the
"inline frame state" into account.

I hope that doing so will be easy, but I thought it would be a good idea to
record the current state using an RFC.

Tested on x86_64-linux.

Co-Authored-By: Andrew Burgess <aburgess@redhat.com>

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=33981
---
 gdb/gdbthread.h                               |  4 +-
 .../gdb.dwarf2/dw2-extend-inline-block.exp    | 45 +++++++++++++++++--
 2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index c0a828409a4..6b2d0ba0801 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -129,7 +129,7 @@ struct thread_control_state
   /* Set step_start_function according to PC.  */
   void set_step_start_function (CORE_ADDR pc)
   {
-    m_step_start_function = find_symbol_for_pc (pc);
+    m_step_start_function = find_symbol_for_pc_maybe_inline (pc);
   }
 
   /* Reset step_start_function.  */
@@ -141,7 +141,7 @@ struct thread_control_state
   /* Return true if PC is in step_start_function.  */
   bool in_step_start_function (CORE_ADDR pc)
   {
-    return m_step_start_function == find_symbol_for_pc (pc);
+    return m_step_start_function == find_symbol_for_pc_maybe_inline (pc);
   }
 
   /* Return true if step_start_function is set.  */
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-extend-inline-block.exp b/gdb/testsuite/gdb.dwarf2/dw2-extend-inline-block.exp
index 9e4798b53f3..3f92c1965d8 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 PR33930.
+	continue
+    }
+
     with_test_prefix $prefix {
 	set asm_file [standard_output_file ${testfile}-${suffix}.S]
 	$build_dwarf_func $asm_file
-- 
2.51.0