From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f52.google.com (mail-wr1-f52.google.com [209.85.221.52]) by sourceware.org (Postfix) with ESMTPS id 77C323858D28 for ; Wed, 3 Nov 2021 18:43:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 77C323858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f52.google.com with SMTP id r8so4989767wra.7 for ; Wed, 03 Nov 2021 11:43:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=hHq8hglcOgfaHSD4gxcgK0ZRbZdaqedoJRRBCdV04+U=; b=pjEWAkbYCOkK/NpfNpiAkUBfN8jh7J3Nf/ordoBb18n6UPk6RxNLihoGKZzBEB9v73 q99NJBB3d2A5Kh2zX3n89UmUB57q0acn0l49/yn5mVIUHOzrd4QZBN43jqa/Pe4XNC8R M8/btn4oMRSqbWPkQqjeGURa7XEbFMF6LMt1+DhuCDFrsL8oYhKDyfCSxGM21EPIp6QV 6dZNuHSGgGyXNbsjTj9DDp/M1HbI2kq4KNN9UjdtnUI5lHjeT5Ek45xR5766JZZ4znKE bklEmjS2cTBQr1PRoAJ2jXDshd0e/Cm7vRWGFkXJV8UJKkXVtHizgkW3LXK94UDz9roP W2Sw== X-Gm-Message-State: AOAM531DSn1EiqqVwKdAKh2RAj5j4VoG/36b1V2ILjKdPkgpWpFd0Xty 03CB+qZXux964wCM8jLWJ2/0iDzc8u+Ldg== X-Google-Smtp-Source: ABdhPJwRmFH7+4DJ+qngCRcEG5tU47iDTlHmbiQ7vf+C4xKtZ3GQ99d4uZDbAwjhlX2KGv9gbR0hqw== X-Received: by 2002:a05:6000:10cb:: with SMTP id b11mr46426588wrx.71.1635965005267; Wed, 03 Nov 2021 11:43:25 -0700 (PDT) Received: from ?IPV6:2001:8a0:f912:1a00:fb57:3faf:e98:b979? ([2001:8a0:f912:1a00:fb57:3faf:e98:b979]) by smtp.gmail.com with ESMTPSA id l2sm2332278wmq.42.2021.11.03.11.43.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 03 Nov 2021 11:43:24 -0700 (PDT) Message-ID: <22163066-87d8-3507-ce0a-31470eaf4fa2@palves.net> Date: Wed, 3 Nov 2021 18:43:22 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.1 Subject: Re: [PATCH 4/4] gdb, infrun: fix multi-threaded reverse stepping Content-Language: en-US To: Markus Metzger , gdb-patches@sourceware.org References: <20210316093501.936148-1-markus.t.metzger@intel.com> <20210316093501.936148-5-markus.t.metzger@intel.com> From: Pedro Alves In-Reply-To: <20210316093501.936148-5-markus.t.metzger@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Wed, 03 Nov 2021 18:43:29 -0000 On 2021-03-16 09:35, Markus Metzger via Gdb-patches wrote: > 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. The going via the event loop to handle the pending event is intentional, simplifies things by only having one code path. (Please adjust the commit log given the new information.) > > 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. Do you envision that you'll support having some threads moving forward and other threads moving backwards, all in the same inferior or target? Like: (gdb) thread 1.1 (gdb) c& // thread 1.1 is now continuously recording (gdb) thread 1.2 (gdb) reverse-continue // thread 1.2 is running backwards Which makes me question -- seems odd to have both the direction recorded in all the threads, and then still have target_execution_direction() ? Do we want to eliminate one of these? Either the target direction, or the threads direction? If we don't want to support threads of the same target executing in different directions, then I suspect that we can reimplement this patch by adding a new target_set_execution_direction target method, that would be called in the case where infrun skips calling target_resume because of a pending event. If we do want to support mixed directions, then is the following snipped from fetch_inferior_event still useful for anything? scoped_restore save_exec_dir = make_scoped_restore (&execution_direction, target_execution_direction ()); AFAICS, when handling an event we'll now use the thread's execution direction, so flipping the global like this here is basically dead code. > > 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, You can just drop the "struct", given C++. > + 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, Ditto. > + 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); Shouldn't this be done for all do_target_resume calls? How about doing it from within do_target_resume instead? > + > 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" { This inadvertently leaves the prompt in the expect buffer. You can use -wrap to match it. > + fail $gdb_test_name > + } > + -re "$gdb_prompt $" { > + pass $gdb_test_name > + } > + } > +} > + > +check_not_replaying 1 > +check_not_replaying 2 >