[binutils-gdb] gdb: make it possible to restore selected user-created frames

[binutils-gdb] gdb: make it possible to restore selected user-created frames

[Simon Marchi]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=bc2cbe815bdbac3bd027bf4acc94c554c29b0189

commit bc2cbe815bdbac3bd027bf4acc94c554c29b0189
Author: Simon Marchi <simon.marchi@efficios.com>
Date:   Tue Dec 13 22:34:38 2022 -0500

    gdb: make it possible to restore selected user-created frames
    
    I would like to improve frame_info_ptr to automatically grab the
    information needed to reinflate a frame, and automatically reinflate it
    as needed.  One thing that is in the way is the fact that some frames
    can be created out of thin air by the create_new_frame function.  These
    frames are not the fruit of unwinding from the target's current frame.
    These frames are created by the "select-frame view" command.
    
    These frames are not correctly handled by the frame save/restore
    functions, save_selected_frame, restore_selected_frame and
    lookup_selected_frame.  This can be observed here, using the test
    included in this patch:
    
        $ ./gdb --data-directory=data-directory -nx -q testsuite/outputs/gdb.base/frame-view/frame-view
        Reading symbols from testsuite/outputs/gdb.base/frame-view/frame-view...
        (gdb) break thread_func
        Breakpoint 1 at 0x11a2: file /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c, line 42.
        (gdb) run
        Starting program: /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/frame-view/frame-view
    
        [Thread debugging using libthread_db enabled]
        Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
        [New Thread 0x7ffff7cc46c0 (LWP 4171134)]
        [Switching to Thread 0x7ffff7cc46c0 (LWP 4171134)]
    
        Thread 2 "frame-view" hit Breakpoint 1, thread_func (p=0x0) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42
        42        foo (11);
        (gdb) info frame
        Stack level 0, frame at 0x7ffff7cc3ee0:
         rip = 0x5555555551a2 in thread_func (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42); saved rip = 0x7ffff7d4e8fd
         called by frame at 0x7ffff7cc3f80
         source language c.
         Arglist at 0x7ffff7cc3ed0, args: p=0x0
         Locals at 0x7ffff7cc3ed0, Previous frame's sp is 0x7ffff7cc3ee0
         Saved registers:
          rbp at 0x7ffff7cc3ed0, rip at 0x7ffff7cc3ed8
        (gdb) thread 1
        [Switching to thread 1 (Thread 0x7ffff7cc5740 (LWP 4171122))]
        #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6
    
    Here, we create a custom frame for thread 1 (using the stack from thread
    2, for convenience):
    
        (gdb) select-frame view 0x7ffff7cc3f80 0x5555555551a2
    
    The first calls to "frame" looks good:
    
        (gdb) frame
        #0  thread_func (p=0x7ffff7d4e630) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:42
        42        foo (11);
    
    But not the second one:
    
        (gdb) frame
        #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6
    
    This second "frame" command shows the current target frame instead of
    the user-created frame.
    
    It's not totally clear how the "select-frame view" feature is expected
    to behave, especially since it's not tested.  I heard accounts that it
    used to be possible to select a frame like this and do "up" and "down"
    to navigate the backtrace starting from that frame.  The fact that
    create_new_frame calls frame_unwind_find_by_frame to install the right
    unwinder suggest that it used to be possible.  But that doesn't work
    today:
    
        (gdb) select-frame view 0x7ffff7cc3f80 0x5555555551a2
        (gdb) up
        Initial frame selected; you cannot go up.
        (gdb) down
        Bottom (innermost) frame selected; you cannot go down.
    
    and "backtrace" always shows the actual thread's backtrace, it ignores
    the user-created frame:
    
        (gdb) bt
        #0  0x00007ffff7d4b4b6 in ?? () from /usr/lib/libc.so.6
        #1  0x00007ffff7d50403 in ?? () from /usr/lib/libc.so.6
        #2  0x000055555555521a in main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:56
    
    I don't want to address all the `select-frame view` issues , but I think
    we can agree that the "frame" command changing the selected frame, as
    shown above, is a bug.  I would expect that command to show the
    currently selected frame and not change it.
    
    This happens because of the scoped_restore_selected_frame object in
    print_frame_args.  The frame information is saved in the constructor
    (the backtrace below), and restored in the destructor.
    
        #0  save_selected_frame (frame_id=0x7ffdc0020ad0, frame_level=0x7ffdc0020af0) at /home/simark/src/binutils-gdb/gdb/frame.c:1682
        #1  0x00005631390242f0 in scoped_restore_selected_frame::scoped_restore_selected_frame (this=0x7ffdc0020ad0) at /home/simark/src/binutils-gdb/gdb/frame.c:324
        #2  0x000056313993581e in print_frame_args (fp_opts=..., func=0x62100023bde0, frame=..., num=-1, stream=0x60b000000300) at /home/simark/src/binutils-gdb/gdb/stack.c:755
        #3  0x000056313993ad49 in print_frame (fp_opts=..., frame=..., print_level=1, print_what=SRC_AND_LOC, print_args=1, sal=...) at /home/simark/src/binutils-gdb/gdb/stack.c:1401
        #4  0x000056313993835d in print_frame_info (fp_opts=..., frame=..., print_level=1, print_what=SRC_AND_LOC, print_args=1, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:1126
        #5  0x0000563139932e0b in print_stack_frame (frame=..., print_level=1, print_what=SRC_AND_LOC, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:368
        #6  0x0000563139932bbe in print_stack_frame_to_uiout (uiout=0x611000016840, frame=..., print_level=1, print_what=SRC_AND_LOC, set_current_sal=1) at /home/simark/src/binutils-gdb/gdb/stack.c:346
        #7  0x0000563139b0641e in print_selected_thread_frame (uiout=0x611000016840, selection=...) at /home/simark/src/binutils-gdb/gdb/thread.c:1993
        #8  0x0000563139940b7f in frame_command_core (fi=..., ignored=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1871
        #9  0x000056313994db9e in frame_command_helper<frame_command_core>::base_command (arg=0x0, from_tty=1) at /home/simark/src/binutils-gdb/gdb/stack.c:1976
    
    Since the user-created frame has level 0 (identified by the saved level
    -1), lookup_selected_frame just reselects the target's current frame,
    and the user-created frame is lost.
    
    My goal here is to fix this particular problem.
    
    Currently, select_frame does not set selected_frame_id and
    selected_frame_level for frames with level 0.  It leaves them at
    null_frame_id / -1, indicating to restore_selected_frame to use the
    target's current frame.  User-created frames also have level 0, so add a
    special case them such that select_frame saves their selected id and
    level.
    
    save_selected_frame does not need any change.
    
    Change the assertion in restore_selected_frame that checks `frame_level
    != 0` to account for the fact that we can restore user-created frames,
    which have level 0.
    
    Finally, change lookup_selected_frame to make it able to re-create
    user-created frame_info objects from selected_frame_level and
    selected_frame_id.
    
    Add a minimal test case for the case described above, that is the
    "select-frame view" command followed by the "frame" command twice.  In
    order to have a known stack frame to switch to, the test spawns a second
    thread, and tells the first thread to use the other thread's top frame.
    
    Change-Id: Ifc77848dc465fbd21324b9d44670833e09fe98c7
    Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Diff:
---
 gdb/frame.c                           | 30 ++++++++++----
 gdb/testsuite/gdb.base/frame-view.c   | 74 +++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/frame-view.exp | 70 +++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+), 7 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 0909109c1f4..9ab8fa0310e 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -69,6 +69,7 @@ set_backtrace_options user_set_backtrace_options;
 
 static frame_info_ptr get_prev_frame_raw (frame_info_ptr this_frame);
 static const char *frame_stop_reason_symbol_string (enum unwind_stop_reason reason);
+static frame_info_ptr create_new_frame (frame_id id);
 
 /* Status of some values cached in the frame_info object.  */
 
@@ -1669,9 +1670,12 @@ get_current_frame (void)
 
    If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1,
    and the target has stack and is stopped, the selected frame is the
-   current (innermost) frame.  This means that SELECTED_FRAME_LEVEL is
-   never 0 and SELECTED_FRAME_ID is never the ID of the innermost
-   frame.
+   current (innermost) target frame.  SELECTED_FRAME_ID is never the ID
+   of the current (innermost) target frame.  SELECTED_FRAME_LEVEL may
+   only be 0 if the selected frame is a user-created one (created and
+   selected through the "select-frame view" command), in which case
+   SELECTED_FRAME_ID is the frame id derived from the user-provided
+   addresses.
 
    If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1,
    and the target has no stack or is executing, then there's no
@@ -1699,9 +1703,9 @@ void
 restore_selected_frame (frame_id frame_id, int frame_level)
   noexcept
 {
-  /* save_selected_frame never returns level == 0, so we shouldn't see
-     it here either.  */
-  gdb_assert (frame_level != 0);
+  /* Unless it is a user-created frame, save_selected_frame never returns
+     level == 0, so we shouldn't see it here either.  */
+  gdb_assert (frame_level != 0 || frame_id.user_created_p);
 
   /* FRAME_ID can be null_frame_id only IFF frame_level is -1.  */
   gdb_assert ((frame_level == -1 && !frame_id_p (frame_id))
@@ -1735,6 +1739,15 @@ lookup_selected_frame (struct frame_id a_frame_id, int frame_level)
       return;
     }
 
+  /* This means the selected frame was a user-created one.  Create a new one
+     using the user-provided addresses, which happen to be in the frame id.  */
+  if (frame_level == 0)
+    {
+      gdb_assert (a_frame_id.user_created_p);
+      select_frame (create_new_frame (a_frame_id));
+      return;
+    }
+
   /* select_frame never saves 0 in SELECTED_FRAME_LEVEL, so we
      shouldn't see it here.  */
   gdb_assert (frame_level > 0);
@@ -1859,7 +1872,10 @@ select_frame (frame_info_ptr fi)
 
   selected_frame = fi;
   selected_frame_level = frame_relative_level (fi);
-  if (selected_frame_level == 0)
+
+  /* If the frame is a user-created one, save its level and frame id just like
+     any other non-level-0 frame.  */
+  if (selected_frame_level == 0 && !fi->this_id.value.user_created_p)
     {
       /* Treat the current frame especially -- we want to always
 	 save/restore it without warning, even if the frame ID changes
diff --git a/gdb/testsuite/gdb.base/frame-view.c b/gdb/testsuite/gdb.base/frame-view.c
new file mode 100644
index 00000000000..7b1cbd6abd4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frame-view.c
@@ -0,0 +1,74 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022 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 <pthread.h>
+#include <assert.h>
+
+struct type_1
+{
+  int m;
+};
+
+struct type_2
+{
+  int n;
+};
+
+static int
+baz (struct type_1 z1, struct type_2 z2)
+{
+  return z1.m + z2.n;
+}
+
+static int
+bar (struct type_1 y1, struct type_2 y2)
+{
+  return baz (y1, y2);
+}
+
+static int
+foo (struct type_1 x1, struct type_2 x2)
+{
+  return bar (x1, x2);
+}
+
+static void *
+thread_func (void *p)
+{
+  struct type_1 t1;
+  struct type_2 t2;
+  t1.m = 11;
+  t2.n = 11;
+  foo (t1, t2);
+
+  return NULL;
+}
+
+int
+main (void)
+{
+  pthread_t thread;
+  int res;
+
+  res = pthread_create (&thread, NULL, thread_func, NULL);
+  assert (res == 0);
+
+  res = pthread_join (thread, NULL);
+  assert (res == 0);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/frame-view.exp b/gdb/testsuite/gdb.base/frame-view.exp
new file mode 100644
index 00000000000..d2dba143448
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frame-view.exp
@@ -0,0 +1,70 @@
+# Copyright 2022 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/>.
+
+# Test the "frame view" family of commands.
+
+standard_testfile
+
+if { [build_executable "failed to prepare" \
+	${testfile} ${srcfile}] } {
+    return
+}
+
+proc test_select_frame_view {} {
+    clean_restart $::binfile
+
+    if { ![runto_main] } {
+	return
+    }
+
+    # Stop thread 2 at a baz.
+    gdb_test "break baz"
+    gdb_test "continue" "Thread 2.*hit Breakpoint $::decimal, baz .*"
+
+    # Grab the stack pointer and pc of thread 2's frame.
+    set frame_sp ""
+    set frame_pc ""
+
+    gdb_test_multiple "info frame" "" {
+	-re -wrap ".*frame at ($::hex):.*" {
+	    set frame_sp $expect_out(1,string)
+	    pass $gdb_test_name
+	}
+    }
+
+    gdb_test_multiple "print/x \$pc" "" {
+	-re -wrap " = ($::hex)" {
+	    set frame_pc $expect_out(1,string)
+	    pass $gdb_test_name
+	}
+    }
+
+    if { $frame_sp == "" || $frame_pc == "" } {
+	# Something must have failed and logged a failure above.
+	return
+    }
+
+    # Select thread 2's frame in thread 1.
+    gdb_test "thread 1" "Switching to thread 1 .*"
+    gdb_test_no_output "select-frame view $frame_sp $frame_pc"
+
+    # Verify that the "frame" command does not change the selected frame.
+    # There used to be a bug where the "frame" command would lose the
+    # selection of user-created frames.
+    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame"
+    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame again"
+}
+
+test_select_frame_view