From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id OL6NCBkxK2ioNSoAWB0awg (envelope-from ) for ; Mon, 19 May 2025 09:24:41 -0400 Received: by simark.ca (Postfix, from userid 112) id 1F4D81E11C; Mon, 19 May 2025 09:24:41 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-9.0 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED, RCVD_IN_VALIDITY_RPBL,RCVD_IN_VALIDITY_SAFE autolearn=ham autolearn_force=no version=4.0.1 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id C909A1E102 for ; Mon, 19 May 2025 09:24:39 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 601CE3858C2C for ; Mon, 19 May 2025 13:24:39 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 601CE3858C2C Received: from mail-wm1-f48.google.com (mail-wm1-f48.google.com [209.85.128.48]) by sourceware.org (Postfix) with ESMTPS id 3D6563858C2C for ; Mon, 19 May 2025 13:23:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3D6563858C2C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 3D6563858C2C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=209.85.128.48 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1747661003; cv=none; b=bAz48AtDyKftcZNouTHwmkA9dTC7SS4X/KmfpStDRtfIaiBUQsPpkre2ix9sA93oqIWaO7ZTxaBkMb5VyNsd1XGTgh77WbTOTfmoFpzQsOvdCipd92bY+4foXCB2dgx+99RUJy+WRqSJQjpUDzaCHTKnFMcGQSFTA2vlkenN5ME= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1747661003; c=relaxed/simple; bh=9ZlxUxRLYoFWivHiu/WFnWjcz+6GgENDQxhh1sqQtlM=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=LjJXhujFwGr9DzFJGbxrZ0XUEmnHPpOWcq/rrj3JhR8wrlNGszBnVH6PDbL3zoNtQbbSiX6CE8H+WlIpH2giazNu5dbCK1jQQJhy2X4mqpTcIZ9zDX+HvfvovlXzNEO0Kj38ZfpnPkPBFOfxetV3YaZ/+phZ2+zZJTPW74zEV3Y= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3D6563858C2C Received: by mail-wm1-f48.google.com with SMTP id 5b1f17b1804b1-43cec5cd73bso29364275e9.3 for ; Mon, 19 May 2025 06:23:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1747661002; x=1748265802; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=28+Td+rJ7XwO6wG+qbqkA7IGWZ4Y8IKTTqEIJSlQ8II=; b=OibBpetpPtTl/iQGoeV0/cZ8ozcgJVo5+lfBPiuboeUKMBK4gTIl0spWKXIqQyRJBM dETApCt1S87f+n+rtKahd0eTXpH8RnD5tZ27xtMAcnO8/edC103mYsNkWx8OBtem/r/z Sg/ijSKv4fwNrawjTedpGj8zMmEZA2akCaNy3E7/gmumCljqfWnicCprir7pWnNpRNjJ 3rclIIb707lyhuklJg15+LquuOVmyOTGWFButVpMiaUsIFFLJZQpPQmQFh0fNfayiLqf hw4tCZKzQjFwdihAHzBkJhdXsa4Q4pygjRDNWjj+iZ9WBrl65wKGHNDkUEigB1owcv8Y p0OQ== X-Gm-Message-State: AOJu0Yxf9v+NWrlBNci6Fz88M3OLlpYxbVdvnMyPgpYJWCBjHRur0YY+ QLK5VT3CNfYgzrlINcRmwoeom5MdQhOBW6pEwgda5JmSFpl+Bw73KDlrrFVztdkx X-Gm-Gg: ASbGncs4Il1WzD1cR3OadtJqGjPG/A1hvge0dYdX+75k0W2wfPuIDaR7Se9rXlcROKs aZlRn3baQqnw0IeGRcEpx2v7PteVjzsFfgQDIv2cLvvOD4+ojM4ctLIw/VVodXFunItJe+HVpSR W3OjE24KP++I/9EzO3erm9aLnOXbR7xHug//AV++t3Mg5s3b4qbRIhGECCyvTCgEmvd+Hg2QSCa dGqtJv9Tw7yyPIcU6K60cpuFg9bZ2kcwKBk9go5kDtvWTstXr0dDhwo0GEI4FqysZEKkOIpnCzr +OwspiERjWM03pRTG70pvpRsuSjRPtw1UctQ3xUe3LkgTLlGX+I= X-Google-Smtp-Source: AGHT+IHVUKLvVcyvO/5De1cpdNvJ1aHTPNkouaEfYAAhR1pkZkkwzKA30LvwLMq2jrJRVDQO/lXJ3g== X-Received: by 2002:a05:600c:46c7:b0:441:d4e8:76c6 with SMTP id 5b1f17b1804b1-442fd675b5amr136269705e9.30.1747661001157; Mon, 19 May 2025 06:23:21 -0700 (PDT) Received: from localhost ([2001:8a0:4fe9:b400:8d90:6f0d:36bf:32df]) by smtp.gmail.com with UTF8SMTPSA id 5b1f17b1804b1-442fa3e2ce5sm158954595e9.13.2025.05.19.06.23.20 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 19 May 2025 06:23:20 -0700 (PDT) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH v2 04/47] infrun: Split currently_stepping, fix sw watchpoints issue Date: Mon, 19 May 2025 14:22:25 +0100 Message-ID: <20250519132308.3553663-5-pedro@palves.net> X-Mailer: git-send-email 2.49.0 In-Reply-To: <20250519132308.3553663-1-pedro@palves.net> References: <20250519132308.3553663-1-pedro@palves.net> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org The gdb.base/watchpoint.exp on Windows with non-stop support added (later in the series) exposed an issue with the currently_stepping logic when tested with software watchpoints. The issue only happens when: - You have multiple threads. gdb.base/watchpoint.exp exposed it on Windows because there the OS always spawns a few extra threads. - Displaced stepping is not available. The Windows non-stop work does not implement displaced stepping yet. That is left as an optimization for later. - The target backend is working in non-stop mode. I've written a new test that exposes the issue on GNU/Linux as well (on hardware single-step targets, like x86-64). There, we see: continue Continuing. ../../src/gdb/infrun.c:2918: internal-error: resume_1: Assertion `!(thread_has_single_step_breakpoints_set (tp) && step)' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. ----- Backtrace ----- FAIL: gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp: target-non-stop=on: displaced-stepping=off: continue until exit (GDB internal error) Currently, software watchpoints are implemented by forcing single-stepping. That is done by currently_stepping returning true when we have a software watchpoint. proceed calls resume, which calls resume_1, which then ends up always requesting a single-step resume, even if the higher layers wanted a continue. Now, if you set a software watchpoint, and then continue the program, and there's a breakpoint at the current PC, GDB needs to step over that breakpoint first. If displaced stepping is not available, then GDB temporarily pauses all threads, removes the breakpoint, single-steps the thread that needs to move past the breakpoint, and then finally, reinserts the breakpoint, and restarts all threads again. That last restarting step happens in the restart_threads infrun function. restart_threads iterates over all threads trying to restart them one by one. There, we have: if (currently_stepping (tp)) { infrun_debug_printf ("restart threads: [%s] was stepping", tp->ptid.to_string ().c_str ()); but, what if TP is actually a new thread that hasn't yet ever set stepping? currently_stepping still returns true, due to the software watchpoint, and we end up in keep_going_stepped_thread, here: if (tp->stop_pc () != tp->prev_pc) { ptid_t resume_ptid; infrun_debug_printf ("expected thread advanced also (%s -> %s)", paddress (current_inferior ()->arch (), tp->prev_pc), paddress (current_inferior ()->arch (), tp->stop_pc ())); ... because prev_pc was stale at that point (we had no reason to update it earlier). E.g. on Windows we see something like: [infrun] restart_threads: start: event_thread=1867996.1867996.0, inf=-1 [infrun] restart_threads: restart threads: [1867996.1867996.0] is event thread [infrun] restart_threads: restart threads: [1867996.1868003.0] was stepping [infrun] keep_going_stepped_thread: resuming previously stepped thread [infrun] keep_going_stepped_thread: expected thread advanced also (0 -> 0x7ffff7ce57f8) [infrun] clear_step_over_info: clearing step over info [infrun] do_target_resume: resume_ptid=1867996.1868003.0, step=0, sig=GDB_SIGNAL_0 On GNU/Linux, we may see: [infrun] keep_going_stepped_thread: expected thread advanced also (0x7ffff7d2683d -> 0x7ffff7ce57f8) there prev_pc might have been updated on an earlier proceed call, which makes the issue harder to see, but it is stale too here. That means we insert a single-step breakpoint at the current PC, and continue the thread, with target_resume directly, asking for a normal continue. Eventually, something causes a user-visible stop. For example, the software watchpoint triggers. That makes GDB stop all threads. Now, if the user re-resumes the program, say, with "continue", we fail this assertion in resume_1 coming from proceed: /* If STEP is set, it's a request to use hardware stepping facilities. But in that case, we should never use singlestep breakpoint. */ gdb_assert (!(thread_has_single_step_breakpoints_set (tp) && step)); "step" is true because currently_stepping returns true since we have a software watchpoint. And the thread has a single-step breakpoint installed from earlier, because of that code mentioned above, in keep_going_stepped_thread reached from restart_threads. This patch fixes the root cause -- the currently_stepping call in restart_threads returned true for a thread that has never set stepping in the first place. This is because currently_stepping really serves two purposes currently: #1 - for a thread that we are about to resume, should we set it stepping? #2 - for a thread that just stopped, was it stepping before? The fix is thus to decouple those two aspects: - for #1, we simply rename currently_stepping to should_step. - for #2, we record whether the thread was stepping before in a new currently_stepping flag in thread_info. As mentioned, there's a new testcase included. I tested this on x86-64 GNU/Linux, native and gdbserver, and on Windows x64 with the non-stop series. The assertion triggers on all of those with the fix, and is fixed by this patch on all of those, too. Change-Id: I7b07bc62e8570333d2e4856d2e55ae6e58f8260c --- gdb/gdbthread.h | 4 + gdb/infrun.c | 28 +++--- .../sw-watchpoint-step-over-bp-with-threads.c | 64 +++++++++++++ ...w-watchpoint-step-over-bp-with-threads.exp | 91 +++++++++++++++++++ 4 files changed, 173 insertions(+), 14 deletions(-) create mode 100644 gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.c create mode 100644 gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index c561e9a7b64..a8fd967c702 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -152,6 +152,10 @@ struct thread_control_state the finished single step. */ int trap_expected = 0; + /* True if the thread TP is in the middle of (software or hardware) + single-stepping. */ + bool currently_stepping = false; + /* Nonzero if the thread is being proceeded for a "finish" command or a similar situation when return value should be printed. */ int proceed_to_finish = 0; diff --git a/gdb/infrun.c b/gdb/infrun.c index 119bd151034..9f625a7bd44 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -87,7 +87,7 @@ static void sig_print_header (void); static void follow_inferior_reset_breakpoints (void); -static bool currently_stepping (struct thread_info *tp); +static bool should_step (thread_info *tp); static void insert_hp_step_resume_breakpoint_at_frame (const frame_info_ptr &); @@ -2656,7 +2656,7 @@ resume_1 (enum gdb_signal sig) "status %s (currently_stepping=%d).", tp->ptid.to_string ().c_str (), tp->pending_waitstatus ().to_string ().c_str (), - currently_stepping (tp)); + tp->control.currently_stepping); tp->inf->process_target ()->threads_executing = true; tp->set_resumed (true); @@ -2685,7 +2685,7 @@ resume_1 (enum gdb_signal sig) tp->stepped_breakpoint = 0; /* Depends on stepped_breakpoint. */ - step = currently_stepping (tp); + step = tp->control.currently_stepping = should_step (tp); if (current_inferior ()->thread_waiting_for_vfork_done != nullptr) { @@ -3060,7 +3060,7 @@ clear_proceed_status_thread (struct thread_info *tp) ("thread %s has pending wait status %s (currently_stepping=%d).", tp->ptid.to_string ().c_str (), tp->pending_waitstatus ().to_string ().c_str (), - currently_stepping (tp)); + tp->control.currently_stepping); } } @@ -5041,7 +5041,7 @@ adjust_pc_after_break (struct thread_info *thread, we also need to back up to the breakpoint address. */ if (thread_has_single_step_breakpoints_set (thread) - || !currently_stepping (thread) + || !thread->control.currently_stepping || (thread->stepped_breakpoint && thread->prev_pc == breakpoint_pc)) regcache_write_pc (regcache, breakpoint_pc); @@ -5368,7 +5368,7 @@ save_waitstatus (struct thread_info *tp, const target_waitstatus &ws) && software_breakpoint_inserted_here_p (aspace, pc)) tp->set_stop_reason (TARGET_STOPPED_BY_SW_BREAKPOINT); else if (!thread_has_single_step_breakpoints_set (tp) - && currently_stepping (tp)) + && tp->control.currently_stepping) tp->set_stop_reason (TARGET_STOPPED_BY_SINGLE_STEP); } } @@ -5563,7 +5563,7 @@ handle_one (const wait_one_event &event) paddress (current_inferior ()->arch (), t->stop_pc ()), t->ptid.to_string ().c_str (), - currently_stepping (t)); + t->control.currently_stepping); } } @@ -6668,7 +6668,7 @@ restart_threads (struct thread_info *event_thread, inferior *inf) tp->ptid.to_string ().c_str ()); } - if (currently_stepping (tp)) + if (tp->control.currently_stepping) { infrun_debug_printf ("restart threads: [%s] was stepping", tp->ptid.to_string ().c_str ()); @@ -6795,7 +6795,7 @@ finish_step_over (struct execution_control_state *ecs) paddress (current_inferior ()->arch (), tp->stop_pc ()), tp->ptid.to_string ().c_str (), - currently_stepping (tp)); + tp->control.currently_stepping); /* This in-line step-over finished; clear this so we won't start a new one. This is what handle_signal_stop would @@ -7204,7 +7204,7 @@ handle_signal_stop (struct execution_control_state *ecs) /* If not, perhaps stepping/nexting can. */ if (random_signal) random_signal = !(ecs->event_thread->stop_signal () == GDB_SIGNAL_TRAP - && currently_stepping (ecs->event_thread)); + && ecs->event_thread->control.currently_stepping); /* Perhaps the thread hit a single-step breakpoint of _another_ thread. Single-step breakpoints are transparent to the @@ -8632,12 +8632,12 @@ keep_going_stepped_thread (struct thread_info *tp) return true; } -/* Is thread TP in the middle of (software or hardware) - single-stepping? (Note the result of this function must never be - passed directly as target_resume's STEP parameter.) */ +/* Should thread TP be stepped (software or hardware)? (Note the + result of this function must never be passed directly as + target_resume's STEP parameter.) */ static bool -currently_stepping (struct thread_info *tp) +should_step (thread_info *tp) { return ((tp->control.step_range_end && tp->control.step_resume_breakpoint == nullptr) diff --git a/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.c b/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.c new file mode 100644 index 00000000000..7f014036c9b --- /dev/null +++ b/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.c @@ -0,0 +1,64 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2025 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 +#include + +static pthread_barrier_t threads_started_barrier; + +static void * +thread_func (void *arg) +{ + pthread_barrier_wait (&threads_started_barrier); + + while (1) + sleep (1); + + return NULL; +} + +static void +dummy () +{ +} + +static unsigned watched_global = 0; + +int +main (void) +{ + pthread_t thread; + int ret; + + alarm (30); + + pthread_barrier_init (&threads_started_barrier, NULL, 2); + + ret = pthread_create (&thread, NULL, thread_func, NULL); + assert (ret == 0); + + /* Make sure all threads are scheduled before hitting the + breakpoint. */ + pthread_barrier_wait (&threads_started_barrier); + + ++watched_global; /* break here start */ + + dummy (); /* just so there's extra code here */ + + return 0; /* break here end */ +} diff --git a/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp b/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp new file mode 100644 index 00000000000..b45db57e80f --- /dev/null +++ b/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp @@ -0,0 +1,91 @@ +# Copyright 2025 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 continuing with a software watchpoint installed, when there are +# multiple threads, and previously we stepped over a breakpoint. +# +# This is a regression test for a GDB bug where stepping over a +# breakpoint in-line made GDB insert a software single-step breakpoint +# in some threads by mistake, which later would cause an assertion to +# fail. +# +# The issue only triggered when: +# +# - The program has multiple threads. +# - The target backend is working in non-stop mode. +# - Displaced stepping is not available. +# - The target supports hardware single-step. +# +# However, we exercise more combinations for completeness. + +standard_testfile .c + +if { [build_executable "failed to prepare" $testfile $srcfile \ + {debug pthreads}] \ + == -1 } { + return +} + +proc test {target-non-stop displaced-stepping} { + + save_vars ::GDBFLAGS { + append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\"" + append ::GDBFLAGS " -ex \"set displaced ${displaced-stepping}\"" + clean_restart $::binfile + } + + if ![runto_main] { + return + } + + # Run to a breakpoint, and leave it installed, so that GDB needs + # to step over it before continuing. + gdb_breakpoint [gdb_get_line_number "break here start"] + gdb_continue_to_breakpoint "started" + + # GDB should know about at least two threads by now. + gdb_test "p \$_inferior_thread_count >= 2" " = 1" + + # Set a software watchpoint. This makes GDB single-step all + # instructions when we next continue. + gdb_test_no_output "set can-use-hw-watchpoints 0" + gdb_test "watch watched_global" "Watchpoint $::decimal: watched_global" + + # Continue with the software watchpoint in place. In the original + # bug, with displaced stepping disabled, this would make GDB + # incorrectly install a software single-step breakpoint on threads + # other than the main one. + gdb_test_multiple "cont" "continue to watchpoint" { + -re -wrap "Continuing.*Watchpoint.*watched_global.*Old value = 0.*New value = 1.*" { + pass $gdb_test_name + } + } + + # The final continue, with the software watchpoint set, so that + # GDB single-steps all threads (if the target is non-stop). With + # the buggy GDB, the non-main thread had a software single-step + # breakpoint set, and on hardware single-step targets, GDB would + # fail an assertion that checks that we never ask the target to + # hardware single-step a thread when we have a software + # single-step breakpoint set for that thread. + gdb_breakpoint [gdb_get_line_number "break here end"] + gdb_continue_to_breakpoint "end" +} + +foreach_with_prefix target-non-stop {auto on off} { + foreach_with_prefix displaced-stepping {auto on off} { + test ${target-non-stop} ${displaced-stepping} + } +} -- 2.49.0