[binutils-gdb] gdb: make user-created frames reinflatable

[binutils-gdb] gdb: make user-created frames reinflatable

[Simon Marchi]

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

commit 836a8d3710462596bb4184617999a9507adc3629
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Tue Dec 13 22:34:39 2022 -0500

    gdb: make user-created frames reinflatable
    
    This patch teaches frame_info_ptr to reinflate user-created frames
    (frames created through create_new_frame, with the "select-frame view"
    command).
    
    Before this patch, frame_info_ptr doesn't support reinflating
    user-created frames, because it currently reinflates by getting the
    current target frame (for frame 0) or frame_find_by_id (for other
    frames).  To reinflate a user-created frame, we need to call
    create_new_frame, to make it lookup an existing user-created frame, or
    otherwise create one.
    
    So, in prepare_reinflate, get the frame id even if the frame has level
    0, if it is user-created.  In reinflate, if the saved frame id is user
    create it, call create_new_frame.
    
    In order to test this, I initially enhanced the gdb.base/frame-view.exp
    test added by the previous patch by setting a pretty-printer for the
    type of the function parameters, in which we do an inferior call.  This
    causes print_frame_args to not reinflate its frame (which is a
    user-created one) properly.  On one machine (my Arch Linux one), it
    properly catches the bug, as the frame is not correctly restored after
    printing the first parameter, so it messes up the second parameter:
    
        frame
        #0  baz (z1=hahaha, z2=<error reading variable: frame address is not available.>) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:40
        40        return z1.m + z2.n;
        (gdb) FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame
        frame
        #0  baz (z1=hahaha, z2=<error reading variable: frame address is not available.>) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:40
        40        return z1.m + z2.n;
        (gdb) FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame again
    
    However, on another machine (my Ubuntu 22.04 one), it just passes fine,
    without the appropriate fix.  I then thought about writing a selftest
    for that, it's more reliable.  I left the gdb.base/frame-view.exp pretty
    printer test there, it's already written, and we never know, it might
    catch some unrelated issue some day.
    
    Change-Id: I5849baf77991fc67a15bfce4b5e865a97265b386
    Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Diff:
---
 gdb/Makefile.in                          |  1 +
 gdb/frame.c                              | 22 ++++++---
 gdb/frame.h                              |  7 ++-
 gdb/testsuite/gdb.base/frame-view.c      |  6 +++
 gdb/testsuite/gdb.base/frame-view.exp    | 46 ++++++++++++++++--
 gdb/testsuite/gdb.base/frame-view.py     | 41 ++++++++++++++++
 gdb/unittests/frame_info_ptr-selftests.c | 80 ++++++++++++++++++++++++++++++++
 7 files changed, 192 insertions(+), 11 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index cce85b87436..c3711a0d2f5 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -458,6 +458,7 @@ SELFTESTS_SRCS = \
 	unittests/environ-selftests.c \
 	unittests/filtered_iterator-selftests.c \
 	unittests/format_pieces-selftests.c \
+	unittests/frame_info_ptr-selftests.c \
 	unittests/function-view-selftests.c \
 	unittests/gdb_tilde_expand-selftests.c \
 	unittests/gmp-utils-selftests.c \
diff --git a/gdb/frame.c b/gdb/frame.c
index 9ab8fa0310e..2d2af874d6a 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -3213,7 +3213,8 @@ frame_info_ptr::prepare_reinflate ()
 {
   m_cached_level = frame_relative_level (*this);
 
-  if (m_cached_level != 0)
+  if (m_cached_level != 0
+      || (m_ptr != nullptr && m_ptr->this_id.value.user_created_p))
     m_cached_id = get_frame_id (*this);
 }
 
@@ -3232,13 +3233,22 @@ frame_info_ptr::reinflate ()
       return;
     }
 
-  /* Frame #0 needs special handling, see comment in select_frame.  */
-  if (m_cached_level == 0)
-    m_ptr = get_current_frame ().get ();
+  if (m_cached_id.user_created_p)
+    m_ptr = create_new_frame (m_cached_id).get ();
   else
     {
-      gdb_assert (frame_id_p (m_cached_id));
-      m_ptr = frame_find_by_id (m_cached_id).get ();
+      /* Frame #0 needs special handling, see comment in select_frame.  */
+      if (m_cached_level == 0)
+	m_ptr = get_current_frame ().get ();
+      else
+	{
+	  /* If we reach here without a valid frame id, it means we are trying
+	     to reinflate a frame whose id was not know at construction time.
+	     We're probably trying to reinflate a frame while computing its id
+	     which is not possible, and would indicate a problem with GDB.  */
+	  gdb_assert (frame_id_p (m_cached_id));
+	  m_ptr = frame_find_by_id (m_cached_id).get ();
+	}
     }
 
   gdb_assert (m_ptr != nullptr);
diff --git a/gdb/frame.h b/gdb/frame.h
index 70ad606c134..74524ec41f4 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -325,7 +325,12 @@ private:
   /* The underlying pointer.  */
   frame_info *m_ptr = nullptr;
 
-  /* The frame_id of the underlying pointer.  */
+  /* The frame_id of the underlying pointer.
+
+     For the current target frames (frames with level 0, obtained through
+     get_current_frame), we don't save the frame id, we leave it at
+     null_frame_id.  For user-created frames (also with level 0, but created
+     with create_new_frame), we do save the id.  */
   frame_id m_cached_id = null_frame_id;
 
   /* The frame level of the underlying pointer.  */
diff --git a/gdb/testsuite/gdb.base/frame-view.c b/gdb/testsuite/gdb.base/frame-view.c
index 7b1cbd6abd4..e330da0b1af 100644
--- a/gdb/testsuite/gdb.base/frame-view.c
+++ b/gdb/testsuite/gdb.base/frame-view.c
@@ -28,6 +28,12 @@ struct type_2
   int n;
 };
 
+__attribute__((used)) static int
+called_from_pretty_printer (void)
+{
+  return 23;
+}
+
 static int
 baz (struct type_1 z1, struct type_2 z2)
 {
diff --git a/gdb/testsuite/gdb.base/frame-view.exp b/gdb/testsuite/gdb.base/frame-view.exp
index d2dba143448..edae16735ab 100644
--- a/gdb/testsuite/gdb.base/frame-view.exp
+++ b/gdb/testsuite/gdb.base/frame-view.exp
@@ -22,13 +22,28 @@ if { [build_executable "failed to prepare" \
     return
 }
 
-proc test_select_frame_view {} {
+# If WITH_PRETTY_PRINTER is true, load pretty printers for the function
+# parameter types, in which we do an inferior call.  This is meant to test
+# that the frame_info_ptr correctly reinflates frames created using
+# "select-frame view".
+
+proc test_select_frame_view { with_pretty_printer } {
     clean_restart $::binfile
 
+    if { $with_pretty_printer } {
+	require allow_python_tests
+    }
+
     if { ![runto_main] } {
 	return
     }
 
+    if { $with_pretty_printer } {
+	set remote_python_file \
+	    [gdb_remote_download host "${::srcdir}/${::subdir}/${::testfile}.py"]
+	gdb_test_no_output "source ${remote_python_file}" "load python file"
+    }
+
     # Stop thread 2 at a baz.
     gdb_test "break baz"
     gdb_test "continue" "Thread 2.*hit Breakpoint $::decimal, baz .*"
@@ -60,11 +75,34 @@ proc test_select_frame_view {} {
     gdb_test "thread 1" "Switching to thread 1 .*"
     gdb_test_no_output "select-frame view $frame_sp $frame_pc"
 
+    if { $with_pretty_printer } {
+	# When the pretty printer does its infcall, it is done on the currently
+	# selected thread, thread 1 here.  However, other threads are resumed
+	# at the same time.  This causes thread 2 to exit during that infcall,
+	# leading to this weirdness:
+	#
+	#     frame^M
+	#     #0  baz (z=[Thread 0x7ffff7cc26c0 (LWP 417519) exited]^M
+	#     hohoho) at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.base/frame-view.c:35^M
+	#     35        return z.n + 2;^M
+	#
+	# Avoid that by setting scheduler-locking on.
+	gdb_test_no_output "set scheduler-locking on"
+
+	set z1_pattern "hahaha"
+	set z2_pattern "hohoho"
+    } else {
+	set z1_pattern "\\.\\.\\."
+	set z2_pattern "\\.\\.\\."
+    }
+
     # 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"
+    gdb_test "frame" "#0  baz \\(z1=$z1_pattern, z2=$z2_pattern\\).*" "frame"
+    gdb_test "frame" "#0  baz \\(z1=$z1_pattern, z2=$z2_pattern\\).*" "frame again"
 }
 
-test_select_frame_view
+foreach_with_prefix with_pretty_printer {false true} {
+    test_select_frame_view $with_pretty_printer
+}
diff --git a/gdb/testsuite/gdb.base/frame-view.py b/gdb/testsuite/gdb.base/frame-view.py
new file mode 100644
index 00000000000..6c1b8bcf82b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/frame-view.py
@@ -0,0 +1,41 @@
+# Copyright (C) 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/>.
+
+
+class Printer1:
+    def to_string(self):
+        n = gdb.parse_and_eval("called_from_pretty_printer ()")
+        assert n == 23
+        return "hahaha"
+
+
+class Printer2:
+    def to_string(self):
+        n = gdb.parse_and_eval("called_from_pretty_printer ()")
+        assert n == 23
+        return "hohoho"
+
+
+def lookup_function(val):
+    if str(val.type) == "struct type_1":
+        return Printer1()
+
+    if str(val.type) == "struct type_2":
+        return Printer2()
+
+    return None
+
+
+gdb.pretty_printers.append(lookup_function)
diff --git a/gdb/unittests/frame_info_ptr-selftests.c b/gdb/unittests/frame_info_ptr-selftests.c
new file mode 100644
index 00000000000..edf1b9fa0bb
--- /dev/null
+++ b/gdb/unittests/frame_info_ptr-selftests.c
@@ -0,0 +1,80 @@
+/* Self tests for frame_info_ptr.
+
+   Copyright (C) 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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 "defs.h"
+
+#include "frame.h"
+#include "gdbsupport/selftest.h"
+#include "scoped-mock-context.h"
+#include "test-target.h"
+
+namespace selftests {
+
+static void
+validate_user_created_frame (frame_id id)
+{
+  SELF_CHECK (id.stack_status == FID_STACK_VALID);
+  SELF_CHECK (id.stack_addr == 0x1234);
+  SELF_CHECK (id.code_addr_p);
+  SELF_CHECK (id.code_addr == 0x5678);
+}
+
+static frame_info_ptr
+user_created_frame_callee (frame_info_ptr frame)
+{
+  validate_user_created_frame (get_frame_id (frame));
+
+  frame.prepare_reinflate ();
+  reinit_frame_cache ();
+  frame.reinflate ();
+
+  validate_user_created_frame (get_frame_id (frame));
+
+  return frame;
+}
+
+static void
+test_user_created_frame ()
+{
+  scoped_mock_context<test_target_ops> mock_context
+    (current_inferior ()->gdbarch);
+  frame_info_ptr frame = create_new_frame (0x1234, 0x5678);
+
+  validate_user_created_frame (get_frame_id (frame));
+
+  /* Pass the frame to a callee, which calls reinit_frame_cache.  This lets us
+     validate that the reinflation in both the callee and caller restore the
+     same frame_info object.  */
+  frame.prepare_reinflate ();
+  frame_info_ptr callees_frame_info = user_created_frame_callee (frame);
+  frame.reinflate ();
+
+  validate_user_created_frame (get_frame_id (frame));
+  SELF_CHECK (frame.get () == callees_frame_info.get ());
+}
+
+} /* namespace selftests */
+
+void _initialize_frame_info_ptr_selftests ();
+void
+_initialize_frame_info_ptr_selftests ()
+{
+  selftests::register_test ("frame_info_ptr_user",
+			    selftests::test_user_created_frame);
+}