[PATCH 2/6] gdb: make it possible to restore selected user-created frames

[Simon Marchi <simon.marchi@polymtl.ca>]

From: Simon Marchi <simon.marchi@efficios.com>

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.

The reinflation code works by saving the given frame's id (with an
exception for frame #0).  When reinflating, it unwinds from the target's
current frame, looking for a frame with that id.  Since user-created
frames created by create_new_frame can't be "re-found" through
unwinding, frame_info_ptr wouldn't be able to reinflate such a frame
today.  And since frame_info_ptr::reinflate asserts that m_ptr is not
nullptr on exit, I guess GDB would crash.

The same problem exists when saving and restoring the selected frame if
the selected frame is a user-created one (except the crash part).
save_selected_frame, restore_selected_frame and lookup_selected_frame
(all used in the process of saving and resoring the selected frame)
don't know how to handle user-created frames.  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 original 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.  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

However, 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 the scoped_restore_selected_frame object in
print_frame_args.  frame is saved here, and restored in the destructor
of scoped_restore_selected_frame:

    #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 not to fix all the "select-frame view"-related use
cases, but to fix this particular problem and then (in a subsequent
patch) use the same approach to make frame_info_ptr capable of
reinflating user-created frames, and then make frame_info_ptr automatic.

So, this patch adds the frame_info::is_user_created field and sets it in
create_new_frame (the field is zeroed-out by default by the other place
that instantiates frames, get_prev_frame_raw).  When selecting such a
frame, we remember through the selected_frame_level/select_frame_id pair
that the select frame is a user-created one (more on this below).  When
saving (save_selected_frame) and restoring (restore_selected_frame) the
selected frame, that pair is simply copied out and copied in.  Nothing
really needs to change here, except removing an assert in
restore_selected_frame.  After a restore, it's lookup_selected_frame's
job (if called) to re-instantiate a frame_info from the information in
that pair.  Add a bit of code there to handle the case of user-created
frames, which means calling create_new_frame again with the right
addresses.  The stack address and pc happen to be saved in the frame_id
itself, so it's quite straightforward.

Now, the question is, how to identify a user-created frame through the
selected_frame_level/selected_frame_id pair.  I initially added a
separate "selected_frame_is_user_created" flag to that pair.  Then it
seemed simpler to make it so user-created frames would be identified by
the selected_frame_level == 0.  User-created frames already happen to
have level 0.  And remember that when the saved frame is the current
target frame, selected_frame_level is -1.  We need to modify
select_frame in any case to make it so it will save the frame id and use
the saved level 0 for user-created frames, despite them having level
0.  So just with that, it's possible to distinguish user-created frames,
no need to add a separate flag.  It results in:

 - if selected_frame_level is -1: the selected frame is the current
   frame
 - if selected_frame_level is 0: the selected frame is a user-created
   one
 - if selected_frame_level is > 0: the selected frame is unwound from
   the current frame

I'm not opposed to adding a separate flag to make it extra clear, but I
think this is clear enough, especially since it's contained in frame.c
(and soon frame_info_ptr), the rest of GDB doesn't need to know about
this convention.  And adding a separate flag adds risks of having
invalid states between the saved frame fields.

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
---
 gdb/frame.c                           | 36 +++++++++++--
 gdb/frame.h                           |  4 ++
 gdb/testsuite/gdb.base/frame-view.c   | 74 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/frame-view.exp | 68 ++++++++++++++++++++++++
 4 files changed, 177 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/frame-view.c
 create mode 100644 gdb/testsuite/gdb.base/frame-view.exp

diff --git a/gdb/frame.c b/gdb/frame.c
index 6a6d968b9a97..e6e58a76668f 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -188,6 +188,10 @@ struct frame_info
   /* A frame specific string describing the STOP_REASON in more detail.
      Only valid when PREV_P is set, but even then may still be NULL.  */
   const char *stop_string;
+
+  /* True if this frame was created from addresses given by the user (see
+     create_new_frame) rather than through unwinding.  */
+  bool is_user_created;
 };
 
 /* See frame.h.  */
@@ -1669,6 +1673,11 @@ get_current_frame (void)
    never 0 and SELECTED_FRAME_ID is never the ID of the innermost
    frame.
 
+   An exception to that is if the selected frame is a used-created one
+   (created and selected through the "select-frame view" command).  That
+   is encoded by having SELECTED_FRAME_LEVEL == 0 and SELECTED_FRAME_ID
+   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
    selected frame.  */
@@ -1695,10 +1704,6 @@ 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);
-
   /* FRAME_ID can be null_frame_id only IFF frame_level is -1.  */
   gdb_assert ((frame_level == -1 && !frame_id_p (frame_id))
 	      || (frame_level != -1 && frame_id_p (frame_id)));
@@ -1731,6 +1736,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)
+    {
+      select_frame (create_new_frame (a_frame_id.stack_addr,
+				      a_frame_id.code_addr));
+      return;
+    }
+
   /* select_frame never saves 0 in SELECTED_FRAME_LEVEL, so we
      shouldn't see it here.  */
   gdb_assert (frame_level > 0);
@@ -1855,7 +1869,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->is_user_created)
     {
       /* Treat the current frame especially -- we want to always
 	 save/restore it without warning, even if the frame ID changes
@@ -1934,6 +1951,7 @@ create_new_frame (CORE_ADDR addr, CORE_ADDR pc)
 
   fi = FRAME_OBSTACK_ZALLOC (struct frame_info);
 
+  fi->is_user_created = true;
   fi->next = create_sentinel_frame (current_program_space,
 				    get_current_regcache ());
 
@@ -2841,6 +2859,14 @@ get_frame_type (frame_info_ptr frame)
   return frame->unwind->type;
 }
 
+/* See frame.h.  */
+
+bool
+frame_is_user_created (const frame_info *frame)
+{
+  return frame->is_user_created;
+}
+
 struct program_space *
 get_frame_program_space (frame_info_ptr frame)
 {
diff --git a/gdb/frame.h b/gdb/frame.h
index cf8bbc6a52bd..12cb27573f4e 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -419,6 +419,10 @@ extern int frame_relative_level (frame_info_ptr fi);
 
 extern enum frame_type get_frame_type (frame_info_ptr);
 
+/* Return whether FRAME is user created.  */
+
+extern bool frame_is_user_created (const frame_info *frame);
+
 /* Return the frame's program space.  */
 extern struct program_space *get_frame_program_space (frame_info_ptr);
 
diff --git a/gdb/testsuite/gdb.base/frame-view.c b/gdb/testsuite/gdb.base/frame-view.c
new file mode 100644
index 000000000000..7b1cbd6abd42
--- /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 000000000000..45f0dcf08904
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frame-view.exp
@@ -0,0 +1,68 @@
+# 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.
+    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame"
+    gdb_test "frame" "#0  baz \\(z1=.*, z2=.*\\).*" "frame again"
+}
+
+test_select_frame_view
-- 
2.38.1