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 8CA313858C98 for ; Thu, 4 Apr 2024 18:03:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8CA313858C98 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8CA313858C98 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712253833; cv=none; b=kTr+eusqimI8FeL41iGow+q5trVEmJ3YgJjxDtAQ/ksjGa52h99vrihIdhu9NXw0FQwAEXv421etg2JFb1tIMFWyoYaz//31jOSzusMFKnL6Ugy9hmOYips92tv4iWGImT6fBniYOeIGbHuOoJiG0msn1jnTZQTK/hme5NigkJc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712253833; c=relaxed/simple; bh=nbUnIQ4iWMJ0CnhCYERMe9bdNMlfb5XDvqJ2DGaqTZM=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=RTh+gKJfghikR3sBESIndXwo9mdBHVcl8lcy1+gVJ2uIi+8xTINDMSKC+r+bzT7egrv5SZsStyAKGsKtVlwLrTlV3jxUVIUURwgswQAu3p31OZ5k/q0lGsFtOu5DprFhNbCNUzyvsXnhfwlYaFMLwDsMkDaCmWPaQT9xUaevJNs= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1712253827; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xwJO23wHgs3zr5GJPuIHfpUrc5ZpWQFC1nJwDLMBZ14=; b=d3BS9HZj+u1NPSUWOIB8PEKWwJ9oFyzQHcAyCZbPvkPzCBzE22WaqSG8aME50921cjEQYN 78zYwodi2vkE0b8Pz0K6ZHvgvOfpZ/la3Nr8oCb/qUUTDGkw4I87YP0dm1qUSNBqjqjw4m JTSH/MIa/ghuUIig/SoVk5KFrPspz0E= Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-423-DRsVOLUuNRqKlw7PITJVUg-1; Thu, 04 Apr 2024 14:03:45 -0400 X-MC-Unique: DRsVOLUuNRqKlw7PITJVUg-1 Received: by mail-qv1-f70.google.com with SMTP id 6a1803df08f44-69173411419so14559586d6.2 for ; Thu, 04 Apr 2024 11:03:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712253825; x=1712858625; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=xwJO23wHgs3zr5GJPuIHfpUrc5ZpWQFC1nJwDLMBZ14=; b=ZnpSzzPmgMdKYF8PgXlOVUgEJEH9CUNSM15Zl5xz/rVxQeyMkY7N9hjIN0dDePgPjd dq7SQaOtoF5wVTHSTMVFKLKP05hr1hA1wbr3a66KtRY8IUPqrhuJ1iOKXZ+fxFgXdM7Y Ov4yyAoc8CZK6aCZ5pcpt4iU1snK3l9vom7k0Z0Va98V7DWgEMNkyApQ3cg3qNfA4KR4 2YGGt4zt+jFP6PPZxlGzi8uTPhrZYmGItYuf6NHoovUOiiUOXZKJTe9SRy+r0N+3aoZU QASr/J71xJsslCQGFiWQfT/RPhGYq7SGBV0AYBucxFUxxAX21XnDV5AdgdrE0WaT/edc WqLg== X-Forwarded-Encrypted: i=1; AJvYcCXcTG8wTPmE379hrVE0ZdJLeSt1wGrp1q5cDl23bBF6ZaZIpYtUHdYx0cgQUse8IQtVdjmn87Ia/nrd1gGIIHsxuKv+YgOBwOhaew== X-Gm-Message-State: AOJu0YzmhzDckiKxToJYVxelxWxAmFaCyDzTdw8m+u607euK6mAeRl8M s6faueKiq0q4RLdjWaIILwo5hmfnOne71G80MxVjsPjpnhFbfSMaMkrrNTeSLFOOcvbSr9LNCQJ X8oe6OuZqTAKPcPrdB8VWuP2eKP0KT4PzLcQgDAuDVvgjDBkdGrAdN1caRn4= X-Received: by 2002:a05:6214:2602:b0:696:2b92:3e7a with SMTP id gu2-20020a056214260200b006962b923e7amr2842400qvb.2.1712253825321; Thu, 04 Apr 2024 11:03:45 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFe99WXNryrBZwjWuENQ0qNQT5A9Ii6Bdd78uwkuCwmrpGbf1ttzR+Jars92NMsbeStBjcguQ== X-Received: by 2002:a05:6214:2602:b0:696:2b92:3e7a with SMTP id gu2-20020a056214260200b006962b923e7amr2842375qvb.2.1712253824867; Thu, 04 Apr 2024 11:03:44 -0700 (PDT) Received: from ?IPV6:2804:14d:8084:92c5::1001? ([2804:14d:8084:92c5::1001]) by smtp.gmail.com with ESMTPSA id t1-20020a0cef41000000b006993932c377sm364031qvs.65.2024.04.04.11.03.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 04 Apr 2024 11:03:44 -0700 (PDT) Message-ID: <93207d40-dfa0-49ad-a528-e08d1680e885@redhat.com> Date: Thu, 4 Apr 2024 15:03:42 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 1/4] gdb, infrun, btrace: fix reverse/replay stepping at end of execution history To: Markus Metzger , gdb-patches@sourceware.org References: <20240312113423.3543956-1-markus.t.metzger@intel.com> <20240312113423.3543956-2-markus.t.metzger@intel.com> From: Guinevere Larsen In-Reply-To: <20240312113423.3543956-2-markus.t.metzger@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=-11.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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 List-Id: On 3/12/24 08:34, Markus Metzger wrote: > When trying to step over a breakpoint at the end of the trace, the > step-over will fail with no-history. This does not clear step_over_info > so a subsequent resume will cause GDB to not resume the thread and expect > a SIGTRAP to complete the step-over. This will never come causing GDB to > hang in the wait-for-event poll. > > That step-over failed after actually completing the step. This is wrong. > The step-over itself should have failed and the step should not have > completed. Fix it by moving the end of execution history check to before > we are stepping. > > This exposes another issue, however. When completing a step-over at the > end of the execution history, we implicitly stop replaying that thread. A > continue command would resume after the step-over and, since we're no > longer replaying, would continue recording. > > Fix that by recording the replay state in the thread's control state and > failing with no-history in keep_going if we're switching from replay to > recording. I am a little confused by your test case. From the commit message description, I expected the following GDB session to have been problematic: $ gdb record_goto (gdb) start 49 fun4 (); (gdb) record btrace (gdb) break fun1 (gdb) continue in frame fun1 () 23 } (gdb) reverse-next In frame fun4 () 41 fun1 (); (gdb) next No more reverse-execution history. 23 } (gdb) next # problems due to the SIGTRAP from the previous line (Pardon my hand written gdb session, my new system has an AMD cpu, so I seem to be locked out of btrace). My confusion comes from your test case never explicitly setting any breakpoints. I don't see why the inferior would have a sigtrap generated to test it. Am I misunderstanding the issue you described? -- Cheers, Guinevere Larsen She/Her/Hers > --- > gdb/gdbthread.h | 3 ++ > gdb/infrun.c | 25 +++++++++++++ > gdb/record-btrace.c | 19 +++++----- > gdb/testsuite/gdb.btrace/cont-hang.exp | 43 ++++++++++++++++++++++ > gdb/testsuite/gdb.btrace/step-hang.exp | 42 ++++++++++++++++++++++ > gdb/testsuite/gdb.btrace/stepn.exp | 50 ++++++++++++++++++++++++++ > 6 files changed, 173 insertions(+), 9 deletions(-) > create mode 100644 gdb/testsuite/gdb.btrace/cont-hang.exp > create mode 100644 gdb/testsuite/gdb.btrace/step-hang.exp > create mode 100644 gdb/testsuite/gdb.btrace/stepn.exp > > diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h > index 11c553a99ca..4931a0cc2f1 100644 > --- a/gdb/gdbthread.h > +++ b/gdb/gdbthread.h > @@ -173,6 +173,9 @@ struct thread_control_state > command. This is used to decide whether "set scheduler-locking > step" behaves like "on" or "off". */ > int stepping_command = 0; > + > + /* Whether the thread was replaying when the command was issued. */ > + bool is_replaying = false; > }; > > /* Inferior thread specific part of `struct infcall_suspend_state'. */ > diff --git a/gdb/infrun.c b/gdb/infrun.c > index bbb98f6dcdb..f38d96b64df 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -3089,6 +3089,8 @@ clear_proceed_status_thread (struct thread_info *tp) > > /* Discard any remaining commands or status from previous stop. */ > bpstat_clear (&tp->control.stop_bpstat); > + > + tp->control.is_replaying = target_record_is_replaying (tp->ptid); > } > > /* Notify the current interpreter and observers that the target is about to > @@ -8972,6 +8974,29 @@ keep_going_pass_signal (struct execution_control_state *ecs) > gdb_assert (ecs->event_thread->ptid == inferior_ptid); > gdb_assert (!ecs->event_thread->resumed ()); > > + /* When a thread reaches the end of its execution history, it automatically > + stops replaying. This is so the user doesn't need to explicitly stop it > + with a separate command. > + > + We do not want a single command (e.g. continue) to transition from > + replaying to recording, though, e.g. when starting from a breakpoint we > + needed to step over at the end of the trace. When we reach the end of the > + execution history during stepping, stop with no-history. > + > + The other direction is fine. When we're at the end of the execution > + history, we may reverse-continue to start replaying. */ > + if (ecs->event_thread->control.is_replaying > + && !target_record_is_replaying (ecs->event_thread->ptid)) > + { > + interps_notify_no_history (); > + ecs->ws.set_no_history (); > + set_last_target_status (ecs->target, ecs->ptid, ecs->ws); > + stop_print_frame = true; > + stop_waiting (ecs); > + normal_stop (); > + return; > + } > + > /* Save the pc before execution, to compare with pc after stop. */ > ecs->event_thread->prev_pc > = regcache_read_pc_protected (get_thread_regcache (ecs->event_thread)); > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c > index 6350400c318..ae273fda507 100644 > --- a/gdb/record-btrace.c > +++ b/gdb/record-btrace.c > @@ -2340,6 +2340,16 @@ record_btrace_single_step_forward (struct thread_info *tp) > if (replay == NULL) > return btrace_step_no_history (); > > + /* The execution trace contains (and ends with) the current instruction. > + This instruction has not been executed, yet, so the trace really ends > + one instruction earlier. > + > + We'd fail later on in btrace_insn_next () but we must not trigger > + breakpoints as we're not really able to step. */ > + btrace_insn_end (&end, btinfo); > + if (btrace_insn_cmp (replay, &end) == 0) > + return btrace_step_no_history (); > + > /* Check if we're stepping a breakpoint. */ > if (record_btrace_replay_at_breakpoint (tp)) > return btrace_step_stopped (); > @@ -2362,15 +2372,6 @@ record_btrace_single_step_forward (struct thread_info *tp) > } > while (btrace_insn_get (replay) == NULL); > > - /* Determine the end of the instruction trace. */ > - btrace_insn_end (&end, btinfo); > - > - /* The execution trace contains (and ends with) the current instruction. > - This instruction has not been executed, yet, so the trace really ends > - one instruction earlier. */ > - if (btrace_insn_cmp (replay, &end) == 0) > - return btrace_step_no_history (); > - > return btrace_step_spurious (); > } > > diff --git a/gdb/testsuite/gdb.btrace/cont-hang.exp b/gdb/testsuite/gdb.btrace/cont-hang.exp > new file mode 100644 > index 00000000000..cddcf68b3ab > --- /dev/null > +++ b/gdb/testsuite/gdb.btrace/cont-hang.exp > @@ -0,0 +1,43 @@ > +# This testcase is part of GDB, the GNU debugger. > +# > +# Copyright 2024 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 do not hang when trying to continue over a breakpoint at > +# the end of the trace. > + > +require allow_btrace_tests > + > +standard_testfile record_goto.c > +if [prepare_for_testing "failed to prepare" $testfile $srcfile] { > + return -1 > +} > + > +if ![runto_main] { > + return -1 > +} > + > +# Trace the call to the test function. > +gdb_test_no_output "record btrace" > +gdb_test "next" "main\.3.*" > + > +# We need to be replaying, otherwise, we'd just continue recording. > +gdb_test "reverse-stepi" > +gdb_test "break" > + > +# Continuing will step over the breakpoint and then run into the end of > +# the execution history. This ends replay, so we can continue recording. > +gdb_test "continue" "No more reverse-execution history.*" > +gdb_continue_to_end > diff --git a/gdb/testsuite/gdb.btrace/step-hang.exp b/gdb/testsuite/gdb.btrace/step-hang.exp > new file mode 100644 > index 00000000000..91ea813955d > --- /dev/null > +++ b/gdb/testsuite/gdb.btrace/step-hang.exp > @@ -0,0 +1,42 @@ > +# This testcase is part of GDB, the GNU debugger. > +# > +# Copyright 2024 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 do not hang when trying to step over a breakpoint at the > +# end of the trace. > + > +require allow_btrace_tests > + > +standard_testfile record_goto.c > +if [prepare_for_testing "failed to prepare" $testfile $srcfile] { > + return -1 > +} > + > +if ![runto_main] { > + return -1 > +} > + > +# Trace the call to the test function. > +gdb_test_no_output "record btrace" > +gdb_test "next" "main\.3.*" > + > +# We need to be replaying, otherwise, we'd just continue recording. > +gdb_test "reverse-stepi" > +gdb_test "break" > + > +# Stepping over the breakpoint ends replaying and we can continue recording. > +gdb_test "step" "main\.3.*" > +gdb_continue_to_end > diff --git a/gdb/testsuite/gdb.btrace/stepn.exp b/gdb/testsuite/gdb.btrace/stepn.exp > new file mode 100644 > index 00000000000..4aec90adc65 > --- /dev/null > +++ b/gdb/testsuite/gdb.btrace/stepn.exp > @@ -0,0 +1,50 @@ > +# This testcase is part of GDB, the GNU debugger. > +# > +# Copyright 2024 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 step n does not start recording when issued while replaying. > + > +require allow_btrace_tests > + > +standard_testfile record_goto.c > +if [prepare_for_testing "failed to prepare" $testfile $srcfile] { > + return -1 > +} > + > +if ![runto_main] { > + return -1 > +} > + > +# Trace the call to the test function. > +gdb_test_no_output "record btrace" > +gdb_test "next" "main\.3.*" > + > +# Stepping should bring us to the end of the execution history, but should > +# not resume recording. > +with_test_prefix "stepi" { > + gdb_test "reverse-stepi" > + gdb_test "stepi 5" "No more reverse-execution history.*main\.3.*" > +} > + > +with_test_prefix "step" { > + gdb_test "reverse-step" > + gdb_test "step 5" "No more reverse-execution history.*main\.3.*" > +} > + > +with_test_prefix "next" { > + gdb_test "reverse-next" > + gdb_test "next 5" "No more reverse-execution history.*main\.3.*" > +}