[3/6] gdb: make user-created frames reinflatable
Commit Message
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 again, with the right sp and pc.
So, in prepare_reinflate, get the frame id even if the frame has level
0, if it is user-created. In frame_info_ptr, the distinction between a
user-created frame and the current target frame (both have frame level
0) is that for user-created frames, m_cached id is a non-null frame id,
whereas for the current target frame, m_cached_id is left null. In
reinflate, we use that distinction to call create_new_frame in the case
of user-created frames.
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
---
gdb/frame-info.c | 56 ++++++++++++++++++++++++++-
gdb/frame-info.h | 7 +++-
gdb/testsuite/gdb.base/frame-view.c | 6 +++
gdb/testsuite/gdb.base/frame-view.exp | 47 ++++++++++++++++++++--
gdb/testsuite/gdb.base/frame-view.py | 41 ++++++++++++++++++++
5 files changed, 150 insertions(+), 7 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/frame-view.py
Comments
On 02/12/2022 19:00, Simon Marchi via Gdb-patches wrote:
> 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 again, with the right sp and pc.
>
> So, in prepare_reinflate, get the frame id even if the frame has level
> 0, if it is user-created. In frame_info_ptr, the distinction between a
> user-created frame and the current target frame (both have frame level
> 0) is that for user-created frames, m_cached id is a non-null frame id,
> whereas for the current target frame, m_cached_id is left null. In
> reinflate, we use that distinction to call create_new_frame in the case
> of user-created frames.
>
> 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
> ---
> gdb/frame-info.c | 56 ++++++++++++++++++++++++++-
> gdb/frame-info.h | 7 +++-
> gdb/testsuite/gdb.base/frame-view.c | 6 +++
> gdb/testsuite/gdb.base/frame-view.exp | 47 ++++++++++++++++++++--
> gdb/testsuite/gdb.base/frame-view.py | 41 ++++++++++++++++++++
> 5 files changed, 150 insertions(+), 7 deletions(-)
> create mode 100644 gdb/testsuite/gdb.base/frame-view.py
>
> diff --git a/gdb/frame-info.c b/gdb/frame-info.c
> index 40a872ea152d..d61fb7ed0e95 100644
> --- a/gdb/frame-info.c
> +++ b/gdb/frame-info.c
> @@ -21,6 +21,9 @@
>
> #include "frame-info.h"
> #include "frame.h"
> +#include "gdbsupport/selftest.h"
> +#include "scoped-mock-context.h"
> +#include "test-target.h"
>
> /* See frame-info-ptr.h. */
>
> @@ -33,7 +36,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 && frame_is_user_created (m_ptr)))
> m_cached_id = get_frame_id (*this);
> }
>
> @@ -54,7 +58,13 @@ frame_info_ptr::reinflate ()
>
> /* Frame #0 needs special handling, see comment in select_frame. */
> if (m_cached_level == 0)
> - m_ptr = get_current_frame ().get ();
> + {
> + if (!frame_id_p (m_cached_id))
You seem to be using the stack being valid to check if the frame is user
created, but in the commit message you mention that you use null frame
id. Wouldn't it be more reliable to check if m_cached_id == null_frame_id ?
>> diff --git a/gdb/frame-info.c b/gdb/frame-info.c
>> index 40a872ea152d..d61fb7ed0e95 100644
>> --- a/gdb/frame-info.c
>> +++ b/gdb/frame-info.c
>> @@ -21,6 +21,9 @@
>> #include "frame-info.h"
>> #include "frame.h"
>> +#include "gdbsupport/selftest.h"
>> +#include "scoped-mock-context.h"
>> +#include "test-target.h"
>> /* See frame-info-ptr.h. */
>> @@ -33,7 +36,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 && frame_is_user_created (m_ptr)))
>> m_cached_id = get_frame_id (*this);
>> }
>> @@ -54,7 +58,13 @@ frame_info_ptr::reinflate ()
>> /* Frame #0 needs special handling, see comment in select_frame. */
>> if (m_cached_level == 0)
>> - m_ptr = get_current_frame ().get ();
>> + {
>> + if (!frame_id_p (m_cached_id))
> You seem to be using the stack being valid to check if the frame is
stack -> frame id?
> user created, but in the commit message you mention that you use null
> frame id. Wouldn't it be more reliable to check if m_cached_id ==
> null_frame_id ?
Comparing anything against null_frame_id (even a null frame id) yields
false:
if (stack_status == FID_STACK_INVALID
|| r.stack_status == FID_STACK_INVALID)
/* Like a NaN, if either ID is invalid, the result is false.
Note that a frame ID is invalid iff it is the null frame ID. */
eq = false;
From: https://gitlab.com/gnutools/binutils-gdb/-/blob/a28fedbc3f582ce7c8bad2eb017b1dc072bb1da7/gdb/frame.c#L759-763
I don't really understand the point, I think it would be useful to be
able to compare a frame id to null_frame_id, like we compare pointers to
nullptr. But currently, the correct (and only?) way of checking if we
have a frame id or not in a frame_id is frame_id_p.
When the commit message says:
for user-created frames, m_cached_id is a non-null frame id, whereas
for the current target frame, m_cached_id is left null.
I could say "is valid" and "is invalid" instead.
Simon
On 12/12/2022 14:17, Simon Marchi wrote:
>
>>> diff --git a/gdb/frame-info.c b/gdb/frame-info.c
>>> index 40a872ea152d..d61fb7ed0e95 100644
>>> --- a/gdb/frame-info.c
>>> +++ b/gdb/frame-info.c
>>> @@ -21,6 +21,9 @@
>>> #include "frame-info.h"
>>> #include "frame.h"
>>> +#include "gdbsupport/selftest.h"
>>> +#include "scoped-mock-context.h"
>>> +#include "test-target.h"
>>> /* See frame-info-ptr.h. */
>>> @@ -33,7 +36,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 && frame_is_user_created (m_ptr)))
>>> m_cached_id = get_frame_id (*this);
>>> }
>>> @@ -54,7 +58,13 @@ frame_info_ptr::reinflate ()
>>> /* Frame #0 needs special handling, see comment in select_frame. */
>>> if (m_cached_level == 0)
>>> - m_ptr = get_current_frame ().get ();
>>> + {
>>> + if (!frame_id_p (m_cached_id))
>> You seem to be using the stack being valid to check if the frame is
> stack -> frame id?
No, I meant stack because frame_id_p checks stack_status ==
FID_STACK_INVALID.
>
>> user created, but in the commit message you mention that you use null
>> frame id. Wouldn't it be more reliable to check if m_cached_id ==
>> null_frame_id ?
> Comparing anything against null_frame_id (even a null frame id) yields
> false:
>
> if (stack_status == FID_STACK_INVALID
> || r.stack_status == FID_STACK_INVALID)
> /* Like a NaN, if either ID is invalid, the result is false.
> Note that a frame ID is invalid iff it is the null frame ID. */
> eq = false;
>
> From: https://gitlab.com/gnutools/binutils-gdb/-/blob/a28fedbc3f582ce7c8bad2eb017b1dc072bb1da7/gdb/frame.c#L759-763
>
> I don't really understand the point, I think it would be useful to be
> able to compare a frame id to null_frame_id, like we compare pointers to
> nullptr. But currently, the correct (and only?) way of checking if we
> have a frame id or not in a frame_id is frame_id_p.
Ah right, that explains it. I think we could add a comment to frame_id_p
to make it more clear for newer contributors like me.
However, it isn't the only way to check if a frame_id is null, we can
use the fact that null_frame_id != null_frame_id and check for:
if (m_cached_id != m_cached_id)
/* get current frame */
else
/* recreate user frame */
This is (apparently) very common for folks using Javascript to check if
something is NaN. I personally find it hideous and would much prefer
checking against null_frame_id directly. I just figured I might as well
throw this cursed knowledge out there :)
>
> When the commit message says:
>
> for user-created frames, m_cached_id is a non-null frame id, whereas
> for the current target frame, m_cached_id is left null.
>
> I could say "is valid" and "is invalid" instead.
No, I think the message is fine, I was mostly getting confused about the
use of frame_id_p.
>
> Simon
>
On 12/12/22 08:33, Bruno Larsen wrote:
> On 12/12/2022 14:17, Simon Marchi wrote:
>>
>>>> diff --git a/gdb/frame-info.c b/gdb/frame-info.c
>>>> index 40a872ea152d..d61fb7ed0e95 100644
>>>> --- a/gdb/frame-info.c
>>>> +++ b/gdb/frame-info.c
>>>> @@ -21,6 +21,9 @@
>>>> #include "frame-info.h"
>>>> #include "frame.h"
>>>> +#include "gdbsupport/selftest.h"
>>>> +#include "scoped-mock-context.h"
>>>> +#include "test-target.h"
>>>> /* See frame-info-ptr.h. */
>>>> @@ -33,7 +36,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 && frame_is_user_created (m_ptr)))
>>>> m_cached_id = get_frame_id (*this);
>>>> }
>>>> @@ -54,7 +58,13 @@ frame_info_ptr::reinflate ()
>>>> /* Frame #0 needs special handling, see comment in select_frame. */
>>>> if (m_cached_level == 0)
>>>> - m_ptr = get_current_frame ().get ();
>>>> + {
>>>> + if (!frame_id_p (m_cached_id))
>>> You seem to be using the stack being valid to check if the frame is
>> stack -> frame id?
>
> No, I meant stack because frame_id_p checks stack_status == FID_STACK_INVALID.
>
>>
>>> user created, but in the commit message you mention that you use null
>>> frame id. Wouldn't it be more reliable to check if m_cached_id ==
>>> null_frame_id ?
>> Comparing anything against null_frame_id (even a null frame id) yields
>> false:
>>
>> if (stack_status == FID_STACK_INVALID
>> || r.stack_status == FID_STACK_INVALID)
>> /* Like a NaN, if either ID is invalid, the result is false.
>> Note that a frame ID is invalid iff it is the null frame ID. */
>> eq = false;
>>
>> From: https://gitlab.com/gnutools/binutils-gdb/-/blob/a28fedbc3f582ce7c8bad2eb017b1dc072bb1da7/gdb/frame.c#L759-763
>>
>> I don't really understand the point, I think it would be useful to be
>> able to compare a frame id to null_frame_id, like we compare pointers to
>> nullptr. But currently, the correct (and only?) way of checking if we
>> have a frame id or not in a frame_id is frame_id_p.
>
> Ah right, that explains it. I think we could add a comment to frame_id_p to make it more clear for newer contributors like me.
>
> However, it isn't the only way to check if a frame_id is null, we can use the fact that null_frame_id != null_frame_id and check for:
>
> if (m_cached_id != m_cached_id)
> /* get current frame */
> else
> /* recreate user frame */
>
> This is (apparently) very common for folks using Javascript to check if something is NaN. I personally find it hideous and would much prefer checking against null_frame_id directly. I just figured I might as well throw this cursed knowledge out there :)
Huh, I didn't think of that! I don't like it either, it really doesn't
convey the intent.
Simon
@@ -21,6 +21,9 @@
#include "frame-info.h"
#include "frame.h"
+#include "gdbsupport/selftest.h"
+#include "scoped-mock-context.h"
+#include "test-target.h"
/* See frame-info-ptr.h. */
@@ -33,7 +36,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 && frame_is_user_created (m_ptr)))
m_cached_id = get_frame_id (*this);
}
@@ -54,7 +58,13 @@ frame_info_ptr::reinflate ()
/* Frame #0 needs special handling, see comment in select_frame. */
if (m_cached_level == 0)
- m_ptr = get_current_frame ().get ();
+ {
+ if (!frame_id_p (m_cached_id))
+ m_ptr = get_current_frame ().get ();
+ else
+ m_ptr = create_new_frame (m_cached_id.stack_addr,
+ m_cached_id.code_addr).get ();
+ }
else
{
gdb_assert (frame_id_p (m_cached_id));
@@ -63,3 +73,45 @@ frame_info_ptr::reinflate ()
gdb_assert (m_ptr != nullptr);
}
+
+#if GDB_SELF_TEST
+
+namespace selftests {
+
+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);
+
+ frame_id id = get_frame_id (frame);
+ 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);
+
+ frame.prepare_reinflate ();
+ reinit_frame_cache ();
+ frame.reinflate ();
+
+ id = get_frame_id (frame);
+ 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);
+}
+
+} /* namespace selftests */
+
+#endif
+
+void _initialize_frame_info ();
+void
+_initialize_frame_info ()
+{
+#if GDB_SELF_TEST
+ selftests::register_test ("frame_info_ptr_user",
+ selftests::test_user_created_frame);
+#endif
+}
@@ -157,7 +157,12 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
/* 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. */
@@ -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)
{
@@ -22,13 +22,29 @@ 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 && [skip_python_tests] } {
+ untested "skipping Python test"
+ return
+ }
+
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,9 +76,32 @@ 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.
- 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
+}
new file mode 100644
@@ -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)