From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 55CB13858D28 for ; Wed, 21 Jun 2023 12:35:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 55CB13858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1687350928; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=L/+v2b/+EVfm5vciJykrpXVg5BjYkRdTPCKx8P99T+s=; b=QAArQs2cFyTw4RgzlaeWwuEjzfN0e1bSuAPZ7DyJfByqAKuoWnuFBy+142RuqT8QhoaZsq zwPjNnWQ/FssF16Tfli5QnBpAI7RCUW3OAZs6sc6Pr4y5wA3Zj/gMysF9+OADmEPY4KUNI NCmtzWbSHMEHgqOykedwdraXeRKCvgA= Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-491-6OJm4L-oO6y9pIG7upQ2gQ-1; Wed, 21 Jun 2023 08:35:27 -0400 X-MC-Unique: 6OJm4L-oO6y9pIG7upQ2gQ-1 Received: by mail-qv1-f72.google.com with SMTP id 6a1803df08f44-62ff7c6583cso59743826d6.1 for ; Wed, 21 Jun 2023 05:35:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687350925; x=1689942925; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=L/+v2b/+EVfm5vciJykrpXVg5BjYkRdTPCKx8P99T+s=; b=GLAmhpOSe2wd2Fb85sS7g8qc/PT1eJl3+jXt8avyIc7jozV+zlFJG7O7EfweAx7e8P B0SCjTe/qXVy0zHUkrPdSqdc9xtQRPCS46NINT+O46tnawvFdYI3qexCQB+x+hGI0f03 70BPPHWtdc3GCzyIK+qLikEj+iBR4wt7qei7Arb6DsJ1QQbUEgQx8ZgXB8rsAXzU3WXk SaANjrqLTtSiv2Cevx/5KZXh8dqTeppvlvir6ZrxWmGATiKkYOlC9SA/etBK3wFJ4ZkG t5WqjSLWVdCi8MECGQNyM5afEun/pUXj761dJmvOGXsJ67OcgN9XHu1D9CvpAzJAlJkW u/Qw== X-Gm-Message-State: AC+VfDwF0+gGPGpmDtNcniJj+9VWW9OobckcxAuOxhV7VL+fC/GKSWYm Oc4qhPSoaL0NvyFRcIzd5R+eMMYAuMgZAuc9C7uS3QFsH8tLAaMvRJMMmCfekVagK5IiBDRyExA CXa63Z82yCsduGMMMYgND+g== X-Received: by 2002:a05:6214:1bce:b0:630:7a8:f602 with SMTP id m14-20020a0562141bce00b0063007a8f602mr16840498qvc.7.1687350925346; Wed, 21 Jun 2023 05:35:25 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ7PrKUWq0aNMXuwkq23tzKTMOqnimBZe/4KXyIpkUilPgasVcKPkmAwol+n6CwazEHOqQiJmg== X-Received: by 2002:a05:6214:1bce:b0:630:7a8:f602 with SMTP id m14-20020a0562141bce00b0063007a8f602mr16840476qvc.7.1687350924937; Wed, 21 Jun 2023 05:35:24 -0700 (PDT) Received: from [192.168.0.129] (ip-94-112-225-44.bb.vodafone.cz. [94.112.225.44]) by smtp.gmail.com with ESMTPSA id e17-20020a0cf351000000b005e37909a7fcsm2467047qvm.13.2023.06.21.05.35.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 21 Jun 2023 05:35:24 -0700 (PDT) Message-ID: <6b2d835c-1395-52be-dc9b-2ae79b43f6f7@redhat.com> Date: Wed, 21 Jun 2023 14:35:22 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH v2] gdb/infrun: do not restart a stepped thread if not running (was: gdb/infrun: reset thread control's step info in end_stepping_range) To: Tankut Baris Aktemur , gdb-patches@sourceware.org Cc: simark@simark.ca References: <20230620152455.471716-1-tankut.baris.aktemur@intel.com> From: Bruno Larsen In-Reply-To: <20230620152455.471716-1-tankut.baris.aktemur@intel.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 20/06/2023 17:24, Tankut Baris Aktemur via Gdb-patches wrote: > ====== > Changes in V2: > > * Changed the thread state check to `(tp->state != THREAD_RUNNING)`. > * Rebased on the current master. > > The latest discussion was > https://sourceware.org/pipermail/gdb-patches/2023-May/199613.html > > ====== > > Suppose we have two inferiors on an all-stop target with schedule-multi > set on: > > $ gdb -q > (gdb) target extended-remote | gdbserver --multi - > Remote debugging using | gdbserver --multi - > Remote debugging using stdio > (gdb) file /temp/test > Reading symbols from /temp/test... > (gdb) set remote exec-file /temp/test > (gdb) start > Temporary breakpoint 1 at 0x115c: file test.c, line 8. > Starting program: /temp/test > stdin/stdout redirected > Process /temp/test created; pid = 864027 > ... > > Temporary breakpoint 1, main (argc=1, argv=0x7fffffffd218) at test.c:8 > 8 foo(); > (gdb) add-inferior > [New inferior 2] > Added inferior 2 on connection 1 (extended-remote | gdbserver --multi -) > (gdb) inferior 2 > [Switching to inferior 2 [] ()] > (gdb) file /temp/test > Reading symbols from /temp/test... > (gdb) set remote exec-file /temp/test > (gdb) tbreak 2 > Temporary breakpoint 2 at 0x555555555131: /temp/test.c:2. (2 locations) > (gdb) run > Starting program: /temp/test > stdin/stdout redirected > Process /temp/test created; pid = 864430 > ... > > Thread 2.1 "test" hit Temporary breakpoint 2, foo () at test.c:2 > 2 int a = 42; > (gdb) set schedule-multi on > (gdb) > > At this point, detaching the first inferior works fine: > > (gdb) detach inferiors 1 > Detaching from program: /temp/test, process 858904 > Detaching from process 858904 > [Inferior 1 (process 858904) detached] > (gdb) info inferiors > Num Description Connection Executable > 1 1 (extended-remote | gdbserver --multi -) /temp/test > * 2 process 858925 1 (extended-remote | gdbserver --multi -) /temp/test > (gdb) > > Let us now repeat exactly the same scenario, but before detaching, we > make the current thread single-step an instruction: > > ... > Thread 2.1 "test" hit Temporary breakpoint 2, foo () at test.c:2 > 2 int a = 42; > (gdb) stepi > 3 int b = 43; > (gdb) detach inferiors 1 > Detaching from program: /temp/test, process 876580 > Detaching from process 876580 > gdbserver: Couldn't reap LWP 876580 while detaching: No child processes > [Inferior 1 (process 876580) detached] > (gdb) 3 int b = 43; > > There is a mysterious line info output. Running the scenario with > infrun debug logs reveals more information. > > ... > Thread 2.1 "test" hit Temporary breakpoint 2, foo () at test.c:2 > 2 int a = 42; > (gdb) stepi > 3 int b = 43; > (gdb) set debug infrun on > (gdb) detach inferiors 1 > [infrun] scoped_disable_commit_resumed: reason=detaching > Detaching from program: /temp/test, process 872445 > Detaching from process 872445 > gdbserver: Couldn't reap LWP 872445 while detaching: No child processes > [Inferior 1 (process 872445) detached] > [infrun] start_step_over: enter > [infrun] start_step_over: stealing global queue of threads to step, length = 0 > [infrun] operator(): step-over queue now empty > [infrun] start_step_over: exit > [infrun] restart_stepped_thread: switching back to stepped thread (stepping) > [infrun] keep_going_stepped_thread: resuming previously stepped thread > [infrun] keep_going_stepped_thread: expected thread advanced also (0x555555555131 -> 0x555555555138) > [infrun] clear_step_over_info: clearing step over info > [infrun] do_target_resume: resume_ptid=-1.0.0, step=0, sig=GDB_SIGNAL_0 > [infrun] infrun_async: enable=1 > [infrun] reset: reason=detaching > [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target extended-remote > [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target extended-remote > (gdb) [infrun] fetch_inferior_event: enter > [infrun] scoped_disable_commit_resumed: reason=handling event > [infrun] do_target_wait: Found 2 inferiors, starting at #0 > [infrun] random_pending_event_thread: None found. > [infrun] print_target_wait_results: target_wait (-1.0.0 [process -1], status) = > [infrun] print_target_wait_results: 872464.872464.0 [Thread 872464.872464], > [infrun] print_target_wait_results: status->kind = STOPPED, sig = GDB_SIGNAL_TRAP > [infrun] handle_inferior_event: status->kind = STOPPED, sig = GDB_SIGNAL_TRAP > [infrun] context_switch: Switching context from 0.0.0 to 872464.872464.0 > [infrun] handle_signal_stop: stop_pc=0x555555555138 > [infrun] handle_signal_stop: [872464.872464.0] hit its single-step breakpoint > [infrun] handle_signal_stop: delayed software breakpoint trap, ignoring > [infrun] process_event_stop_test: stepi/nexti > [infrun] stop_waiting: stop_waiting > 3 int b = 43; > [infrun] infrun_async: enable=0 > [infrun] reset: reason=handling event > [infrun] maybe_set_commit_resumed_all_targets: not requesting commit-resumed for target extended-remote, no resumed threads > [infrun] fetch_inferior_event: exit > > GDB attempted to do a step-over for the current thread. This takes us > to the commit that introduced restarting step-overs: > > commit 408f66864a1a823591b26420410c982174c239a2 > Author: Pedro Alves > Date: Mon Jan 11 20:01:58 2021 +0000 > > detach in all-stop with threads running > > A following patch will add a testcase that has a number of threads > constantly stepping over a breakpoint, and then has GDB detach the > process, while threads are running. If we have more than one inferior > running, and we detach from just one of the inferiors, we expect that > the remaining inferior continues running. However, in all-stop, if > GDB needs to pause the target for the detach, nothing is re-resuming > the other inferiors after the detach. "info threads" shows the > threads as running, but they really aren't. This fixes it. > > However, the thread that was resumed for step-over in our scenario did > not have an interrupted step-over; it had completed its stepi already. > More debugging reveals that the thread is resumed because of the > following two conditions in `restart_stepped_thread`: > > if (tp->control.trap_expected) > { > infrun_debug_printf ("switching back to stepped thread (step-over)"); > > if (keep_going_stepped_thread (tp)) > return true; > } > > and > > if (tp->control.step_range_end) > { > infrun_debug_printf ("switching back to stepped thread (stepping)"); > > if (keep_going_stepped_thread (tp)) > return true; > } > > The root cause of the problem is, `restart_stepped_thread` checks for > the thread state as > > if (tp->state == THREAD_EXITED) > continue; > > but the thread's state is THREAD_STOPPED. To fix, we change the state > check to > > if (tp->state != THREAD_RUNNING) > > Additionally, the 'trap_expected' and the 'step_range_end' fields of > the thread's control remain set even after the "stepi" command > completes, creating a half-baked internal state that can be misleading > when debugging. We address this problem by clearing the control > fields when stepping completes. We also add a regression test. > > Regression-tested on X86_64 Linux using the default, native-gdbserver, > and native-extended-gdbserver board files. Hi Tankut! Thanks for working on this. I don't see any regressions, but the test you added doesn't seem to fail before your change. Does it not work with the default options for the testsuite? -- Cheers, Bruno > --- > gdb/infrun.c | 7 ++- > gdb/testsuite/gdb.multi/detach-stepi.c | 30 +++++++++++ > gdb/testsuite/gdb.multi/detach-stepi.exp | 66 ++++++++++++++++++++++++ > 3 files changed, 101 insertions(+), 2 deletions(-) > create mode 100644 gdb/testsuite/gdb.multi/detach-stepi.c > create mode 100644 gdb/testsuite/gdb.multi/detach-stepi.exp > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 58da1cef29e..f40d924dc7a 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -7776,7 +7776,7 @@ restart_stepped_thread (process_stratum_target *resume_target, > > for (thread_info *tp : all_threads_safe ()) > { > - if (tp->state == THREAD_EXITED) > + if (tp->state != THREAD_RUNNING) > continue; > > if (tp->has_pending_waitstatus ()) > @@ -7800,7 +7800,7 @@ restart_stepped_thread (process_stratum_target *resume_target, > > for (thread_info *tp : all_threads_safe ()) > { > - if (tp->state == THREAD_EXITED) > + if (tp->state != THREAD_RUNNING) > continue; > > if (tp->has_pending_waitstatus ()) > @@ -8523,6 +8523,9 @@ static void > end_stepping_range (struct execution_control_state *ecs) > { > ecs->event_thread->control.stop_step = 1; > + ecs->event_thread->control.trap_expected = 0; > + ecs->event_thread->control.step_range_start = 0; > + ecs->event_thread->control.step_range_end = 0; > stop_waiting (ecs); > } > > diff --git a/gdb/testsuite/gdb.multi/detach-stepi.c b/gdb/testsuite/gdb.multi/detach-stepi.c > new file mode 100644 > index 00000000000..d365645fb3f > --- /dev/null > +++ b/gdb/testsuite/gdb.multi/detach-stepi.c > @@ -0,0 +1,30 @@ > +/* 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 . */ > + > +void > +a_function () > +{ > + int a = 42; > +} > + > +int > +main () > +{ > + int b = 43; > + a_function (); > + return 0; > +} > diff --git a/gdb/testsuite/gdb.multi/detach-stepi.exp b/gdb/testsuite/gdb.multi/detach-stepi.exp > new file mode 100644 > index 00000000000..28ef8c4f9f7 > --- /dev/null > +++ b/gdb/testsuite/gdb.multi/detach-stepi.exp > @@ -0,0 +1,66 @@ > +# 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 . > + > +# Test detaching from an inferior after a thread in another inferior > +# completes a stepi. This is a regression test for a bug that was > +# causing an inadvertent resume of the just-stepped thread. > + > +standard_testfile > + > +if {[use_gdb_stub]} { > + untested "using gdb stub" > + return 0 > +} > + > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile]} { > + return -1 > +} > + > +if {![runto_main]} { > + return -1 > +} > + > +delete_breakpoints > + > +# Setup inferior 2. > +gdb_test "add-inferior" "Added inferior .*" \ > + "add empty inferior" > +gdb_test "inferior 2" "Switching to inferior .*" \ > + "switch to inferior" > + > +gdb_load $binfile > +runto "a_function" > +gdb_test "info inferiors" > + > +# The bug for which this regression test is written appears in > +# schedule-multi mode. > +gdb_test_no_output "set schedule-multiple on" > + > +# Single-step the thread in Inferior 2, then detach Inferior 1. > +gdb_test "info threads" ".*" "threads before stepi" > +gdb_test "stepi" > +gdb_test "info threads" ".*" "threads after stepi" > + > +gdb_test "set debug infrun on" > +gdb_test_multiple "detach inferior 1" "" { > + -re "resuming previously stepped thread.*$gdb_prompt" { > + fail $gdb_test_name > + } > + -re "$gdb_prompt $" { > + pass $gdb_test_name > + } > +}