From patchwork Fri Dec 2 18:00:48 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 61378 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A27F9385B53B for ; Fri, 2 Dec 2022 18:01:59 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A27F9385B53B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1670004119; bh=1W3Wo5DrzSWR6k0v029wXqKCY7yHJPFMUC1YXUSaACM=; h=To:Cc:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=TyzWXnp4nhioBR43PmszsyG21SKG5eqNd1u7DGx4gCZrr/aposNTXYXzo7aqFJi0M KMNoKNRGg/m0AEuF5pUFIk17bx8RO3hoES9nYi8WNNJI6xdVvDmHG49pSEH1r8Z/qg N56yOWuGbeC7x/pU7XNZ4kQ9Ht3QK7WJc3NphrbE= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id C62AF3858417 for ; Fri, 2 Dec 2022 18:01:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C62AF3858417 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 2B2I0sKm020422 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 2 Dec 2022 13:00:59 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 2B2I0sKm020422 Received: from simark.localdomain (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 27D4F1E124; Fri, 2 Dec 2022 13:00:53 -0500 (EST) To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH 2/6] gdb: make it possible to restore selected user-created frames Date: Fri, 2 Dec 2022 13:00:48 -0500 Message-Id: <20221202180052.212745-3-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221202180052.212745-1-simon.marchi@polymtl.ca> References: <20221202180052.212745-1-simon.marchi@polymtl.ca> MIME-Version: 1.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Fri, 2 Dec 2022 18:00:54 +0000 X-Spam-Status: No, score=-3189.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Simon Marchi via Gdb-patches From: Simon Marchi Reply-To: Simon Marchi Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" From: Simon Marchi 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::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 Reviewed-By: Bruno Larsen --- 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 . */ + +#include +#include + +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 . + +# 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