From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by sourceware.org (Postfix) with ESMTPS id 2A4063851C33 for ; Tue, 16 Mar 2021 09:35:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2A4063851C33 IronPort-SDR: oF/AXj63l1j/2XySV6D9H/jgx0RzsCPXvvqtqo+R7Src985t64ZOPG7MPbk+TIxzWuneag2Qn2 X5VtQ7m1v0Gw== X-IronPort-AV: E=McAfee;i="6000,8403,9924"; a="253245978" X-IronPort-AV: E=Sophos;i="5.81,251,1610438400"; d="scan'208";a="253245978" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Mar 2021 02:35:19 -0700 IronPort-SDR: R+ZPzqZfWPXGELEZzBxOtl5iKQVGzymBq/tzfSvHrfdgS6lLcPNKJWUytNZ58MwRst0GZRO/wp aD0YxpFvyzug== X-IronPort-AV: E=Sophos;i="5.81,251,1610438400"; d="scan'208";a="605193623" Received: from labpc2407.iul.intel.com (HELO localhost) ([172.28.50.61]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Mar 2021 02:35:18 -0700 From: Markus Metzger To: gdb-patches@sourceware.org Subject: [PATCH 4/4] gdb, infrun: fix multi-threaded reverse stepping Date: Tue, 16 Mar 2021 10:35:01 +0100 Message-Id: <20210316093501.936148-5-markus.t.metzger@intel.com> X-Mailer: git-send-email 2.29.2 In-Reply-To: <20210316093501.936148-1-markus.t.metzger@intel.com> References: <20210316093501.936148-1-markus.t.metzger@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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-List-Received-Date: Tue, 16 Mar 2021 09:35:23 -0000 When reverse-stepping a thread that has a pending breakpoint event, the thread is not resumed as part of the infcmd function. A first resume notices the event and returns without resuming the target. If the corresponding breakpoint has been deleted, event processing results in a second resume that performs the intended stepping action. That resume happens after the infcmd function returned and the temporarily modified execution direction was restored. I would have expected everything to be completed by the time the infcmd function returns but I cannot say whether the behaviour I'm seeing is intentional. Assuming it is, this patch addresses the loss of the execution direction by storing the direction in a thread's control state and changing most of infrun to take it from there rather than using the global variable. gdb/ChangeLog: 2021-02-26 Markus Metzger * gdbthread.h: Include "infrun.h". (struct thread_control_state) : New. * infrun.c (maybe_software_singlestep): Add thread argument. Update callers. Use thread's execution direction. (clear_proceed_status_thread): Set thread's execution direction. (resume_1): Temporarily set the thread's execution direction. (keep_going_stepped_thread): Likewise. (schedlock_applies): Use thread's execution direction. (proceed): Likewise. (adjust_pc_after_break): Likewise. (process_event_stop_test): Likewise. gdb/testsuite/ChangeLog: 2021-02-26 Markus Metzger * gdb.btrace/implicit-stop-replaying.exp: New file. --- gdb/gdbthread.h | 4 + gdb/infrun.c | 55 +++++---- .../gdb.btrace/implicit-stop-replaying.exp | 105 ++++++++++++++++++ 3 files changed, 145 insertions(+), 19 deletions(-) create mode 100644 gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 563b7bd8954..32ad0435552 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -33,6 +33,7 @@ struct symtab; #include "gdbsupport/common-gdbthread.h" #include "gdbsupport/forward-scope-exit.h" #include "displaced-stepping.h" +#include "infrun.h" struct inferior; struct process_stratum_target; @@ -162,6 +163,9 @@ struct thread_control_state /* Whether the thread was replaying when the command was issued. */ bool is_replaying = false; + + /* The execution direction when the command was issued. */ + enum exec_direction_kind execution_direction = EXEC_FORWARD; }; /* Inferior thread specific part of `struct infcall_suspend_state'. */ diff --git a/gdb/infrun.c b/gdb/infrun.c index 81e31cf0019..efcce5bcc4a 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -89,7 +89,8 @@ static void insert_step_resume_breakpoint_at_caller (struct frame_info *); static void insert_longjmp_resume_breakpoint (struct gdbarch *, CORE_ADDR); -static bool maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc); +static bool maybe_software_singlestep (const struct thread_info *tp, + struct gdbarch *gdbarch, CORE_ADDR pc); static void resume (gdb_signal sig); @@ -2048,11 +2049,12 @@ bool sched_multi = false; PC the location to step over. */ static bool -maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc) +maybe_software_singlestep (const struct thread_info *tp, + struct gdbarch *gdbarch, CORE_ADDR pc) { bool hw_step = true; - if (execution_direction == EXEC_FORWARD + if (tp->control.execution_direction == EXEC_FORWARD && gdbarch_software_single_step_p (gdbarch)) hw_step = !insert_single_step_breakpoints (gdbarch); @@ -2336,6 +2338,11 @@ resume_1 (enum gdb_signal sig) insert_breakpoints (); resume_ptid = internal_resume_ptid (user_step); + + scoped_restore save_exec_dir + = make_scoped_restore (&execution_direction, + tp->control.execution_direction); + do_target_resume (resume_ptid, false, GDB_SIGNAL_0); tp->resumed = true; return; @@ -2385,7 +2392,7 @@ resume_1 (enum gdb_signal sig) set_step_over_info (regcache->aspace (), regcache_read_pc (regcache), 0, tp->global_num); - step = maybe_software_singlestep (gdbarch, pc); + step = maybe_software_singlestep (tp, gdbarch, pc); insert_breakpoints (); } @@ -2404,7 +2411,7 @@ resume_1 (enum gdb_signal sig) /* Do we need to do it the hard way, w/temp breakpoints? */ else if (step) - step = maybe_software_singlestep (gdbarch, pc); + step = maybe_software_singlestep (tp, gdbarch, pc); /* Currently, our software single-step implementation leads to different results than hardware single-stepping in one situation: when stepping @@ -2475,7 +2482,7 @@ resume_1 (enum gdb_signal sig) else resume_ptid = internal_resume_ptid (user_step); - if (execution_direction != EXEC_REVERSE + if (tp->control.execution_direction != EXEC_REVERSE && step && breakpoint_inserted_here_p (aspace, pc)) { /* There are two cases where we currently need to step a @@ -2545,6 +2552,10 @@ resume_1 (enum gdb_signal sig) gdb_assert (pc_in_thread_step_range (pc, tp)); } + scoped_restore save_exec_dir + = make_scoped_restore (&execution_direction, + tp->control.execution_direction); + do_target_resume (resume_ptid, step, sig); tp->resumed = true; } @@ -2662,6 +2673,7 @@ clear_proceed_status_thread (struct thread_info *tp) bpstat_clear (&tp->control.stop_bpstat); tp->control.is_replaying = target_record_is_replaying (tp->ptid); + tp->control.execution_direction = ::execution_direction; } void @@ -2761,7 +2773,7 @@ schedlock_applies (struct thread_info *tp) && tp->control.stepping_command) || (scheduler_mode == schedlock_replay && target_record_will_replay (minus_one_ptid, - execution_direction))); + tp->control.execution_direction))); } /* Calls target_commit_resume on all targets. */ @@ -2903,7 +2915,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) { if (pc == cur_thr->suspend.stop_pc && breakpoint_here_p (aspace, pc) == ordinary_breakpoint_here - && execution_direction != EXEC_REVERSE) + && cur_thr->control.execution_direction != EXEC_REVERSE) /* There is a breakpoint at the address we will resume at, step one instruction before inserting breakpoints so that we do not stop right away (and report a second hit at this @@ -4135,7 +4147,7 @@ adjust_pc_after_break (struct thread_info *thread, breakpoint at PC - 1. We'd then report a hit on B1, although INSN1 hadn't been de-executed yet. Doing nothing is the correct behaviour. */ - if (execution_direction == EXEC_REVERSE) + if (thread->control.execution_direction == EXEC_REVERSE) return; /* If the target can tell whether the thread hit a SW breakpoint, @@ -6432,7 +6444,7 @@ process_event_stop_test (struct execution_control_state *ecs) delete_step_resume_breakpoint (ecs->event_thread); if (ecs->event_thread->control.proceed_to_finish - && execution_direction == EXEC_REVERSE) + && ecs->event_thread->control.execution_direction == EXEC_REVERSE) { struct thread_info *tp = ecs->event_thread; @@ -6447,7 +6459,7 @@ process_event_stop_test (struct execution_control_state *ecs) } fill_in_stop_func (gdbarch, ecs); if (ecs->event_thread->suspend.stop_pc == ecs->stop_func_start - && execution_direction == EXEC_REVERSE) + && ecs->event_thread->control.execution_direction == EXEC_REVERSE) { /* We are stepping over a function call in reverse, and just hit the step-resume breakpoint at the start address of @@ -6572,7 +6584,7 @@ process_event_stop_test (struct execution_control_state *ecs) if (pc_in_thread_step_range (ecs->event_thread->suspend.stop_pc, ecs->event_thread) - && (execution_direction != EXEC_REVERSE + && (ecs->event_thread->control.execution_direction != EXEC_REVERSE || frame_id_eq (get_frame_id (frame), ecs->event_thread->control.step_frame_id))) { @@ -6592,7 +6604,7 @@ process_event_stop_test (struct execution_control_state *ecs) CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc; if (stop_pc == ecs->event_thread->control.step_range_start && stop_pc != ecs->stop_func_start - && execution_direction == EXEC_REVERSE) + && ecs->event_thread->control.execution_direction == EXEC_REVERSE) end_stepping_range (ecs); else keep_going (ecs); @@ -6614,7 +6626,7 @@ process_event_stop_test (struct execution_control_state *ecs) backward through the trampoline code, and that's handled further down, so there is nothing for us to do here. */ - if (execution_direction != EXEC_REVERSE + if (ecs->event_thread->control.execution_direction != EXEC_REVERSE && ecs->event_thread->control.step_over_calls == STEP_OVER_UNDEBUGGABLE && in_solib_dynsym_resolve_code (ecs->event_thread->suspend.stop_pc)) { @@ -6747,7 +6759,7 @@ process_event_stop_test (struct execution_control_state *ecs) /* Reverse stepping through solib trampolines. */ - if (execution_direction == EXEC_REVERSE + if (ecs->event_thread->control.execution_direction == EXEC_REVERSE && ecs->event_thread->control.step_over_calls != STEP_OVER_NONE && (gdbarch_skip_trampoline_code (gdbarch, frame, stop_pc) || (ecs->stop_func_start == 0 @@ -6775,7 +6787,7 @@ process_event_stop_test (struct execution_control_state *ecs) stepped into (backwards), and continue to there. When we get there, we'll need to single-step back to the caller. */ - if (execution_direction == EXEC_REVERSE) + if (ecs->event_thread->control.execution_direction == EXEC_REVERSE) { /* If we're already at the start of the function, we've either just stepped backward into a single instruction function, @@ -6838,7 +6850,7 @@ process_event_stop_test (struct execution_control_state *ecs) tmp_sal) && !inline_frame_is_marked_for_skip (true, ecs->event_thread)) { - if (execution_direction == EXEC_REVERSE) + if (ecs->event_thread->control.execution_direction == EXEC_REVERSE) handle_step_into_function_backward (gdbarch, ecs); else handle_step_into_function (gdbarch, ecs); @@ -6856,7 +6868,7 @@ process_event_stop_test (struct execution_control_state *ecs) return; } - if (execution_direction == EXEC_REVERSE) + if (ecs->event_thread->control.execution_direction == EXEC_REVERSE) { /* If we're already at the start of the function, we've either just stepped backward into a single instruction function without line @@ -6885,7 +6897,7 @@ process_event_stop_test (struct execution_control_state *ecs) /* Reverse stepping through solib trampolines. */ - if (execution_direction == EXEC_REVERSE + if (ecs->event_thread->control.execution_direction == EXEC_REVERSE && ecs->event_thread->control.step_over_calls != STEP_OVER_NONE) { CORE_ADDR stop_pc = ecs->event_thread->suspend.stop_pc; @@ -7404,6 +7416,11 @@ keep_going_stepped_thread (struct thread_info *tp) tp->resumed = true; resume_ptid = internal_resume_ptid (tp->control.stepping_command); + + scoped_restore save_exec_dir + = make_scoped_restore (&execution_direction, + tp->control.execution_direction); + do_target_resume (resume_ptid, false, GDB_SIGNAL_0); } else diff --git a/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp b/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp new file mode 100644 index 00000000000..2a396d9497f --- /dev/null +++ b/gdb/testsuite/gdb.btrace/implicit-stop-replaying.exp @@ -0,0 +1,105 @@ +# This testcase is part of GDB, the GNU debugger. +# +# Copyright 2021 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 that we stop replaying other threads when stepping a thread at the +# end of its execution history. +# +# This is similar to the last test in multi-thread-step.exp, except that +# we reverse-step instead of record goto begin to start replaying and we +# step instead of continuing. +# +# This triggered a bug where GDB confused the execution direction and kept +# stepping both threads backwards instead of forwards. + +if { [skip_btrace_tests] } { + unsupported "target does not support record-btrace" + return -1 +} + +standard_testfile multi-thread-step.c +if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug libs=-lpthread}] { + return -1 +} + +if ![runto_main] { + untested "failed to run to main" + return -1 +} + +# Set up breakpoints. +set bp_1 [gdb_get_line_number "bp.1" $srcfile] +set bp_2 [gdb_get_line_number "bp.2" $srcfile] + +# Trace the code between the two breakpoints. +gdb_breakpoint $srcfile:$bp_1 +gdb_continue_to_breakpoint "continue to bp.1" ".*$srcfile:$bp_1\r\n.*" + +# Make sure GDB knows about the new thread. +gdb_test "info threads" ".*" +gdb_test_no_output "record btrace" + +# We have two threads at or close to bp.1 but handled only one stop event. +# Remove the breakpoint so we do not need to deal with the 2nd event. +delete_breakpoints +gdb_breakpoint $srcfile:$bp_2 +gdb_continue_to_breakpoint "continue to bp.2" ".*$srcfile:$bp_2\r\n.*" + +# Determine the thread that reported the breakpoint. +set thread "bad" +gdb_test_multiple "thread" "thread" { + -re "Current thread is \($decimal\).*\r\n$gdb_prompt $" { + set thread $expect_out(1,string) + } +} + +# Determine the other thread. +set other "bad" +if { $thread == 1 } { + set other 2 +} elseif { $thread == 2 } { + set other 1 +} + +# This test only works for scheduler-locking 'replay'. +gdb_test_no_output "set scheduler-locking replay" + +# Remove breakpoints or we might run into it right away. +delete_breakpoints + +# Start replaying the other thread. +gdb_test "thread apply $other reverse-stepi" ".*" +gdb_test "thread apply $other info record" "Replay in progress.*" + +# Step the thread that reported the breakpoint, which is not replaying. +gdb_test "next" "return arg;.*" + +proc check_not_replaying { thread } { + global gdb_prompt + + gdb_test_multiple "thread apply $thread info record" \ + "thread $thread not replaying" { + -re "Replay in progress" { + fail $gdb_test_name + } + -re "$gdb_prompt $" { + pass $gdb_test_name + } + } +} + +check_not_replaying 1 +check_not_replaying 2 -- 2.29.2 Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928