* [PATCH] Fix setting watchpoints when current thread is running (PR gdb/31521)
@ 2024-03-21 16:43 Pedro Alves
2024-03-21 17:11 ` Eli Zaretskii
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2024-03-21 16:43 UTC (permalink / raw)
To: gdb-patches
Currently, when the current thread is running, you can print global
variables. However, if you try to set a watchpoint on the same
globals, GDB errors out, complaining that the selected thread is
running. Like so:
(gdb) c&
Continuing.
(gdb) p global
$1 = 1098377287
(gdb) watch global
Selected thread is running.
This patch makes setting the watchpoint work. The problem is that
update_watchpoint calls get_selected_frame unconditionally. We can
skip it if the watchpoint expression is only watching globals.
You'll now get:
(gdb) c&
Continuing.
(gdb) [New Thread 0x7ffff7d6e640 (LWP 434993)]
[New Thread 0x7ffff756d640 (LWP 434994)]
p global
$1 = 88168
(gdb) watch global
Hardware watchpoint 2: global
(gdb) [Switching to Thread 0x7ffff7d6e640 (LWP 434993)]
Thread 2 "function0" hit Hardware watchpoint 2: global
Old value = 185420
New value = 185423
int_return () at threads.c:39
39 }
This adds a testcase that exercises both all-stop and non-stop, and
also software and hardware watchpoints. It is kfailed for software
watchpoints, as those require another fix not handled by this patch
(the sw watchpoint doesn't fire because GDB doesn't force the
running-free thread to switch to single-stepping).
New NEWS blurb added. I don't think we need to change anything in the
manual.
https://sourceware.org/bugzilla/show_bug.cgi?id=31521
Change-Id: I68ca948541aea3edd4f70741f272f543187abe40
---
gdb/NEWS | 5 +
gdb/breakpoint.c | 11 +-
gdb/testsuite/gdb.base/watchpoint-running.c | 44 ++++++
gdb/testsuite/gdb.base/watchpoint-running.exp | 137 ++++++++++++++++++
4 files changed, 193 insertions(+), 4 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/watchpoint-running.c
create mode 100644 gdb/testsuite/gdb.base/watchpoint-running.exp
diff --git a/gdb/NEWS b/gdb/NEWS
index d8ac0bb06a7..d3df0f96f58 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -28,6 +28,11 @@ maintenance info line-table
if the line is considered the start of the epilgoue, and thus a point at
which the frame can be considered destroyed.
+watch
+awatch
+rwatch
+ GDB now lets you set a watchpoint even if the selected thread is running.
+
* New commands
info missing-debug-handler
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 102bd7fad41..44418956c69 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2154,7 +2154,7 @@ update_watchpoint (struct watchpoint *b, bool reparse)
{
std::vector<value_ref_ptr> val_chain;
struct value *v, *result;
- struct program_space *frame_pspace;
+ struct program_space *wp_pspace;
fetch_subexp_value (b->exp.get (), b->exp->op.get (), &v, &result,
&val_chain, false);
@@ -2173,7 +2173,10 @@ update_watchpoint (struct watchpoint *b, bool reparse)
b->val_valid = true;
}
- frame_pspace = get_frame_program_space (get_selected_frame (NULL));
+ if (b->exp_valid_block == nullptr)
+ wp_pspace = current_program_space;
+ else
+ wp_pspace = get_frame_program_space (get_selected_frame (NULL));
/* Look at each value on the value chain. */
gdb_assert (!val_chain.empty ());
@@ -2233,7 +2236,7 @@ update_watchpoint (struct watchpoint *b, bool reparse)
bp_location *loc = b->allocate_location ();
loc->gdbarch = v->type ()->arch ();
- loc->pspace = frame_pspace;
+ loc->pspace = wp_pspace;
loc->address
= gdbarch_remove_non_address_bits (loc->gdbarch, addr);
b->add_location (*loc);
@@ -2358,7 +2361,7 @@ update_watchpoint (struct watchpoint *b, bool reparse)
bpstat_stop_status requires a location to be able to report
stops, so make sure there's at least a dummy one. */
if (b->type == bp_watchpoint && !b->has_locations ())
- add_dummy_location (b, frame_pspace);
+ add_dummy_location (b, wp_pspace);
}
else if (!within_current_scope)
{
diff --git a/gdb/testsuite/gdb.base/watchpoint-running.c b/gdb/testsuite/gdb.base/watchpoint-running.c
new file mode 100644
index 00000000000..7e6dc5f605b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-running.c
@@ -0,0 +1,44 @@
+/* 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 <http://www.gnu.org/licenses/>. */
+
+#include <unistd.h>
+#include <stdint.h>
+
+uint64_t global_var;
+
+/* How long to usleep, in us. The exp file assumes this is lower than
+ 1s. */
+#define MS_USLEEP 100000
+
+/* How many increments per second MS_USLEEP corresponds to. */
+#define INCREMENTS_PER_SEC (1000000 / MS_USLEEP)
+
+int
+main ()
+{
+ while (1)
+ {
+ usleep (MS_USLEEP);
+ global_var++;
+
+ /* Time out after 30s. */
+ if (global_var >= (30 * INCREMENTS_PER_SEC))
+ return 1;
+ }
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-running.exp b/gdb/testsuite/gdb.base/watchpoint-running.exp
new file mode 100644
index 00000000000..bf5f563e737
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-running.exp
@@ -0,0 +1,137 @@
+# 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 <http://www.gnu.org/licenses/>.
+
+# This test verifies that you can set a watchpoint that watches global
+# memory, when the selected thread is running.
+
+set allow_hw_watchpoint_tests_p [allow_hw_watchpoint_tests]
+
+standard_testfile
+
+if [build_executable "failed to prepare" $testfile $srcfile {debug}] {
+ return -1
+}
+
+# STOP_MODE is either "all-stop" or "non-stop". HW is true if we are
+# testing hardware watchpoints, and false if we're testing software
+# watchpoints.
+
+proc test {stop_mode hw} {
+
+ save_vars { ::GDBFLAGS } {
+ if { $stop_mode == "non-stop" } {
+ append ::GDBFLAGS " -ex \"set non-stop on\""
+ } elseif {[target_info gdb_protocol] == "remote"
+ || [target_info gdb_protocol]== "extended-remote"} {
+ # Communicating with the target while the inferior is
+ # running depends on target running in non-stop mode.
+ # Force it on for remote targets, until this is the
+ # default.
+ append ::GDBFLAGS " -ex \"maint set target-non-stop on\""
+ }
+ clean_restart $::binfile
+ }
+
+ gdb_test_no_output "set can-use-hw-watchpoints $hw"
+
+ if {![runto_main]} {
+ return
+ }
+
+ delete_breakpoints
+
+ # Continue the program in the background.
+ set test "continue&"
+ gdb_test_multiple "continue&" $test {
+ -re "Continuing\\.\r\n$::gdb_prompt " {
+ pass $test
+ }
+ }
+
+ set val1 ""
+ gdb_test_multiple "print global_var" "global_var once" {
+ -re -wrap " = ($::decimal)" {
+ set val1 $expect_out(1,string)
+ pass "$gdb_test_name"
+ }
+ }
+
+ # Sleep for a bit, so the variable is sure to be incremented, if
+ # indeed we managed to get the target running.
+ sleep 1
+
+ set val2 ""
+ gdb_test_multiple "print global_var" "global_var twice" {
+ -re -wrap " = ($::decimal)" {
+ set val2 $expect_out(1,string)
+ pass "$gdb_test_name"
+ }
+ }
+
+ # The variable should have incremented. (Note we didn't give it
+ # sufficient time to ever wrap around.)
+ gdb_assert {$val1 != $val2} "values are different"
+
+ set wp_str [expr ($hw)?"Hardware watchpoint":"Watchpoint"]
+
+ # Now set a watchpoint, while the inferior is running. Since
+ # we're watching a global, and we can read global memory while the
+ # target is running, this should be able to work.
+ gdb_test_multiple "watch global_var" "" {
+ -re "$wp_str $::decimal: global_var\r\n$::gdb_prompt " {
+ pass $gdb_test_name
+ }
+ }
+
+ # Check that the watchpoint triggers.
+
+ save_vars ::timeout {
+ if {!$hw} {
+ # This doesn't currently work with software watchpoints.
+ # GDB should transparently temporarily pause the inferior,
+ # to force it to single step, but it doesn't, so the
+ # thread continues running free.
+ setup_kfail gdb/31521 *-*-*
+
+ # No point waiting too much for output we know isn't
+ # coming.
+ set ::timeout 1
+ }
+ set re [multi_line \
+ "$wp_str $::decimal: global_var" \
+ "" \
+ "Old value = $::decimal" \
+ "New value = $::decimal"]
+ gdb_test_multiple "" "watchpoint hit" {
+ -re $re {
+ pass $gdb_test_name
+ }
+ }
+ }
+}
+
+foreach hw {0 1} {
+ if {$hw && !$allow_hw_watchpoint_tests_p} {
+ continue
+ }
+ foreach stop_mode {all-stop non-stop} {
+ set wp_type [expr ($hw)?"hardware":"software"]
+ with_test_prefix "$stop_mode: $wp_type" {
+ test $stop_mode $hw
+ }
+ }
+}
base-commit: 9bec569fda7c76849cf3eb0e4a525f627d25f980
--
2.43.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix setting watchpoints when current thread is running (PR gdb/31521)
2024-03-21 16:43 [PATCH] Fix setting watchpoints when current thread is running (PR gdb/31521) Pedro Alves
@ 2024-03-21 17:11 ` Eli Zaretskii
2024-03-21 18:38 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Eli Zaretskii @ 2024-03-21 17:11 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
> From: Pedro Alves <pedro@palves.net>
> Date: Thu, 21 Mar 2024 16:43:31 +0000
>
> Currently, when the current thread is running, you can print global
> variables. However, if you try to set a watchpoint on the same
> globals, GDB errors out, complaining that the selected thread is
> running. Like so:
>
> (gdb) c&
> Continuing.
> (gdb) p global
> $1 = 1098377287
> (gdb) watch global
> Selected thread is running.
>
> This patch makes setting the watchpoint work. The problem is that
> update_watchpoint calls get_selected_frame unconditionally. We can
> skip it if the watchpoint expression is only watching globals.
What does this mean in practice? E.g., suppose GDB sets the
watchpoint when the selected thread is changing the variable to be
watched -- will the thread stop or won't it? IOW, I don't understand
what happens with programming the debug registers while some thread is
running.
> New NEWS blurb added. I don't think we need to change anything in the
> manual.
If this is deemed a bugfix, then we don't need to say anything in
NEWS, either, IMO.
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -28,6 +28,11 @@ maintenance info line-table
> if the line is considered the start of the epilgoue, and thus a point at
> which the frame can be considered destroyed.
>
> +watch
> +awatch
> +rwatch
> + GDB now lets you set a watchpoint even if the selected thread is running.
> +
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix setting watchpoints when current thread is running (PR gdb/31521)
2024-03-21 17:11 ` Eli Zaretskii
@ 2024-03-21 18:38 ` Pedro Alves
2024-04-12 17:57 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2024-03-21 18:38 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
On 2024-03-21 17:11, Eli Zaretskii wrote:
>> From: Pedro Alves <pedro@palves.net>
>> Date: Thu, 21 Mar 2024 16:43:31 +0000
>>
>> Currently, when the current thread is running, you can print global
>> variables. However, if you try to set a watchpoint on the same
>> globals, GDB errors out, complaining that the selected thread is
>> running. Like so:
>>
>> (gdb) c&
>> Continuing.
>> (gdb) p global
>> $1 = 1098377287
>> (gdb) watch global
>> Selected thread is running.
>>
>> This patch makes setting the watchpoint work. The problem is that
>> update_watchpoint calls get_selected_frame unconditionally. We can
>> skip it if the watchpoint expression is only watching globals.
>
> What does this mean in practice? E.g., suppose GDB sets the
> watchpoint when the selected thread is changing the variable to be
> watched -- will the thread stop or won't it? IOW, I don't understand
> what happens with programming the debug registers while some thread is
> running.
GDB very briefly temporarily pauses all threads to program the debug registers,
and immediately resumes them. But that is not user visible. That part actually
always worked -- we needed to make that work for when the current thread you have
selected is stopped, but there are other threads in the program that are
running -- we need to arm the debug registers of those running threads too, so
the target backend needs to know to pause any running thread.
We just never reached that code when the _selected_ thread is running because very early
in the "watch" code, we tried to get information about the current frame, while when the
thread is running, there is no frame object to consult, thus GDB threw an error.
>
>> New NEWS blurb added. I don't think we need to change anything in the
>> manual.
>
> If this is deemed a bugfix, then we don't need to say anything in
> NEWS, either, IMO.
OK, I was actually borderline with calling it a bug fix or a new feature.
I will just drop NEWS then.
Thanks!
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix setting watchpoints when current thread is running (PR gdb/31521)
2024-03-21 18:38 ` Pedro Alves
@ 2024-04-12 17:57 ` Pedro Alves
2024-05-07 8:46 ` Tom de Vries
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2024-04-12 17:57 UTC (permalink / raw)
To: gdb-patches
On 2024-03-21 18:38, Pedro Alves wrote:
> On 2024-03-21 17:11, Eli Zaretskii wrote:
>>> New NEWS blurb added. I don't think we need to change anything in the
>>> manual.
>>
>> If this is deemed a bugfix, then we don't need to say anything in
>> NEWS, either, IMO.
>
> OK, I was actually borderline with calling it a bug fix or a new feature.
> I will just drop NEWS then.
>
FYI, I've dropped the NEWS entry, and merged this now.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix setting watchpoints when current thread is running (PR gdb/31521)
2024-04-12 17:57 ` Pedro Alves
@ 2024-05-07 8:46 ` Tom de Vries
2024-05-07 10:30 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2024-05-07 8:46 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
On 4/12/24 19:57, Pedro Alves wrote:
> On 2024-03-21 18:38, Pedro Alves wrote:
>> On 2024-03-21 17:11, Eli Zaretskii wrote:
>
>>>> New NEWS blurb added. I don't think we need to change anything in the
>>>> manual.
>>>
>>> If this is deemed a bugfix, then we don't need to say anything in
>>> NEWS, either, IMO.
>>
>> OK, I was actually borderline with calling it a bug fix or a new feature.
>> I will just drop NEWS then.
>>
>
> FYI, I've dropped the NEWS entry, and merged this now.
This PR ( https://sourceware.org/bugzilla/show_bug.cgi?id=31705 )
reports that the test-case added by the commit fails on arm, and AFAIU
there's also a regression in gdb.threads/signal-command-handle-nopass.exp.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix setting watchpoints when current thread is running (PR gdb/31521)
2024-05-07 8:46 ` Tom de Vries
@ 2024-05-07 10:30 ` Pedro Alves
0 siblings, 0 replies; 6+ messages in thread
From: Pedro Alves @ 2024-05-07 10:30 UTC (permalink / raw)
To: Tom de Vries, gdb-patches
On 2024-05-07 09:46, Tom de Vries wrote:
> This PR ( https://sourceware.org/bugzilla/show_bug.cgi?id=31705 ) reports that the test-case added by the commit fails on arm, and AFAIU there's also a regression in gdb.threads/signal-command-handle-nopass.exp.
Replied on Bugzilla.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-05-07 10:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-21 16:43 [PATCH] Fix setting watchpoints when current thread is running (PR gdb/31521) Pedro Alves
2024-03-21 17:11 ` Eli Zaretskii
2024-03-21 18:38 ` Pedro Alves
2024-04-12 17:57 ` Pedro Alves
2024-05-07 8:46 ` Tom de Vries
2024-05-07 10:30 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).