* [PATCH v3 3/5] Fix for even more missed events; eliminate thread-hop code.
2014-03-07 1:10 [PATCH v3 0/5] Fix lost events, and handle multiple step-overs Pedro Alves
` (3 preceding siblings ...)
2014-03-07 1:10 ` [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set Pedro Alves
@ 2014-03-07 1:10 ` Pedro Alves
2014-03-20 13:59 ` [PUSHED] Re: [PATCH v3 0/5] Fix lost events, and handle multiple step-overs Pedro Alves
5 siblings, 0 replies; 25+ messages in thread
From: Pedro Alves @ 2014-03-07 1:10 UTC (permalink / raw)
To: gdb-patches
Even with deferred_step_ptid out of the way, GDB can still lose
watchpoints.
If a watchpoint triggers and the PC points to an address where a
thread-specific breakpoint for another thread is set, the thread-hop
code triggers, and we lose the watchpoint:
if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
{
int thread_hop_needed = 0;
struct address_space *aspace =
get_regcache_aspace (get_thread_regcache (ecs->ptid));
/* Check if a regular breakpoint has been hit before checking
for a potential single step breakpoint. Otherwise, GDB will
not see this breakpoint hit when stepping onto breakpoints. */
if (regular_breakpoint_inserted_here_p (aspace, stop_pc))
{
if (!breakpoint_thread_match (aspace, stop_pc, ecs->ptid))
thread_hop_needed = 1;
^^^^^^^^^^^^^^^^^^^^^
}
And on software single-step targets, even without a thread-specific
breakpoint in the way, here in the thread-hop code:
else if (singlestep_breakpoints_inserted_p)
{
...
if (!ptid_equal (singlestep_ptid, ecs->ptid)
&& in_thread_list (singlestep_ptid))
{
/* If the PC of the thread we were trying to single-step
has changed, discard this event (which we were going
to ignore anyway), and pretend we saw that thread
trap. This prevents us continuously moving the
single-step breakpoint forward, one instruction at a
time. If the PC has changed, then the thread we were
trying to single-step has trapped or been signalled,
but the event has not been reported to GDB yet.
There might be some cases where this loses signal
information, if a signal has arrived at exactly the
same time that the PC changed, but this is the best
we can do with the information available. Perhaps we
should arrange to report all events for all threads
when they stop, or to re-poll the remote looking for
this particular thread (i.e. temporarily enable
schedlock). */
CORE_ADDR new_singlestep_pc
= regcache_read_pc (get_thread_regcache (singlestep_ptid));
if (new_singlestep_pc != singlestep_pc)
{
enum gdb_signal stop_signal;
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: unexpected thread,"
" but expected thread advanced also\n");
/* The current context still belongs to
singlestep_ptid. Don't swap here, since that's
the context we want to use. Just fudge our
state and continue. */
stop_signal = ecs->event_thread->suspend.stop_signal;
ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_0;
ecs->ptid = singlestep_ptid;
ecs->event_thread = find_thread_ptid (ecs->ptid);
ecs->event_thread->suspend.stop_signal = stop_signal;
stop_pc = new_singlestep_pc;
}
else
{
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog,
"infrun: unexpected thread\n");
thread_hop_needed = 1;
stepping_past_singlestep_breakpoint = 1;
saved_singlestep_ptid = singlestep_ptid;
}
}
}
we either end up with thread_hop_needed, ignoring the watchpoint
SIGTRAP, or switch to the stepping thread, again ignoring that the
SIGTRAP could be for some other event.
The new test added by this patch exercises both paths.
So the fix is similar to the deferred_step_ptid fix -- defer the
thread hop to _after_ the SIGTRAP had a change of passing through the
regular bpstat handling. If the wrong thread hits a breakpoint, we'll
just end up with BPSTAT_WHAT_SINGLE, and if nothing causes a stop,
keep_going starts a step-over.
Most of the stepping_past_singlestep_breakpoint mechanism is really
not necessary -- setting the thread to step over a breakpoint with
thread->trap_expected is sufficient to keep all other threads locked.
It's best to still keep the flag in some form though, because when we
get to keep_going, the software single-step breakpoint we need to step
over is already gone -- an optimization done by a follow up patch will
check whether a step-over is still be necessary by looking to see
whether the breakpoint is still there, and would find the thread no
longer needs a step-over, while we still want it.
Special care is still needed to handle the case of PC of the thread we
were trying to single-step having changed, like in the old code. We
can't just keep_going and re-step it, as in that case we can over-step
the thread (if it was already done with the step, but hasn't reported
it yet, we'd ask it to step even further). That's now handled in
switch_back_to_stepped_thread. As bonus, we're now using a technique
that doesn't lose signals, unlike the old code -- we now insert a
breakpoint at PC, and resume, which either reports the breakpoint
immediately, or any pending signal.
Tested on x86_64 Fedora 17, against pristine mainline, and against a
branch that implements software single-step on x86.
gdb/
2014-03-05 Pedro Alves <palves@redhat.com>
* breakpoint.c (single_step_breakpoint_inserted_here_p): Make
extern.
* breakpoint.h (single_step_breakpoint_inserted_here_p): Declare.
* infrun.c (saved_singlestep_ptid)
(stepping_past_singlestep_breakpoint): Delete.
(resume): Remove stepping_past_singlestep_breakpoint handling.
(proceed): Store the prev_pc of the stepping thread too.
(init_wait_for_inferior): Adjust. Clear singlestep_ptid and
singlestep_pc.
(enum infwait_states): Delete infwait_thread_hop_state.
(struct execution_control_state) <hit_singlestep_breakpoint>: New
field.
(handle_inferior_event): Adjust.
(handle_signal_stop): Delete stepping_past_singlestep_breakpoint
handling and the thread-hop code. Before removing single-step
breakpoints, check whether the thread hit a single-step breakpoint
of another thread. If it did, the trap is not a random signal.
(switch_back_to_stepped_thread): If the event thread hit a
single-step breakpoint, unblock it before switching to the
stepping thread. Handle the case of the stepped thread having
advanced already.
(keep_going): Handle the case of the current thread moving past a
single-step breakpoint.
gdb/testsuite/
2014-03-05 Pedro Alves <palves@redhat.com>
* gdb.threads/step-over-trips-on-watchpoint.c: New file.
* gdb.threads/step-over-trips-on-watchpoint.exp: New file.
--
v2
- no longer make single-step breakpoints be real breakpoints
that end up in bpstat.
- step-over-trips-on-watchpoint.exp test moved to this patch,
and extended to test a thread-specific breakpoint triggering
at the same time as watchpoint.
---
gdb/breakpoint.c | 5 +-
gdb/breakpoint.h | 3 +
gdb/infrun.c | 308 ++++++++-------------
.../gdb.threads/step-over-trips-on-watchpoint.c | 67 +++++
.../gdb.threads/step-over-trips-on-watchpoint.exp | 90 ++++++
5 files changed, 272 insertions(+), 201 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.c
create mode 100644 gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.exp
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 03b21cb..19af9df 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -228,9 +228,6 @@ static void tcatch_command (char *arg, int from_tty);
static void detach_single_step_breakpoints (void);
-static int single_step_breakpoint_inserted_here_p (struct address_space *,
- CORE_ADDR pc);
-
static void free_bp_location (struct bp_location *loc);
static void incref_bp_location (struct bp_location *loc);
static void decref_bp_location (struct bp_location **loc);
@@ -15148,7 +15145,7 @@ detach_single_step_breakpoints (void)
/* Check whether a software single-step breakpoint is inserted at
PC. */
-static int
+int
single_step_breakpoint_inserted_here_p (struct address_space *aspace,
CORE_ADDR pc)
{
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index b4abcb8..d8e88fc 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1126,6 +1126,9 @@ extern int regular_breakpoint_inserted_here_p (struct address_space *,
extern int software_breakpoint_inserted_here_p (struct address_space *,
CORE_ADDR);
+extern int single_step_breakpoint_inserted_here_p (struct address_space *,
+ CORE_ADDR);
+
/* Returns true if there's a hardware watchpoint or access watchpoint
inserted in the range defined by ADDR and LEN. */
extern int hardware_watchpoint_inserted_in_range (struct address_space *,
diff --git a/gdb/infrun.c b/gdb/infrun.c
index b7c02d7..b7e2fd8 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -972,11 +972,6 @@ static ptid_t singlestep_ptid;
/* PC when we started this single-step. */
static CORE_ADDR singlestep_pc;
-/* If another thread hit the singlestep breakpoint, we save the original
- thread here so that we can resume single-stepping it later. */
-static ptid_t saved_singlestep_ptid;
-static int stepping_past_singlestep_breakpoint;
-
/* Info about an instruction that is being stepped over. Invalid if
ASPACE is NULL. */
@@ -1943,24 +1938,8 @@ a command like `return' or `jump' to continue execution."));
resume_ptid = user_visible_resume_ptid (step);
/* Maybe resume a single thread after all. */
- if (singlestep_breakpoints_inserted_p
- && stepping_past_singlestep_breakpoint)
- {
- /* The situation here is as follows. In thread T1 we wanted to
- single-step. Lacking hardware single-stepping we've
- set breakpoint at the PC of the next instruction -- call it
- P. After resuming, we've hit that breakpoint in thread T2.
- Now we've removed original breakpoint, inserted breakpoint
- at P+1, and try to step to advance T2 past breakpoint.
- We need to step only T2, as if T1 is allowed to freely run,
- it can run past P, and if other threads are allowed to run,
- they can hit breakpoint at P+1, and nested hits of single-step
- breakpoints is not something we'd want -- that's complicated
- to support, and has no value. */
- resume_ptid = inferior_ptid;
- }
- else if ((step || singlestep_breakpoints_inserted_p)
- && tp->control.trap_expected)
+ if ((step || singlestep_breakpoints_inserted_p)
+ && tp->control.trap_expected)
{
/* We're allowing a thread to run past a breakpoint it has
hit, by single-stepping the thread with the breakpoint
@@ -2222,6 +2201,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
gdbarch = get_regcache_arch (regcache);
aspace = get_regcache_aspace (regcache);
pc = regcache_read_pc (regcache);
+ tp = inferior_thread ();
if (step > 0)
step_start_function = find_pc_function (pc);
@@ -2278,13 +2258,19 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
thread that reported the most recent event. If a step-over
is required it returns TRUE and sets the current thread to
the old thread. */
+
+ /* Store the prev_pc for the stepping thread too, needed by
+ switch_back_to_stepping thread. */
+ tp->prev_pc = regcache_read_pc (get_current_regcache ());
+
if (prepare_to_proceed (step))
- force_step = 1;
+ {
+ force_step = 1;
+ /* The current thread changed. */
+ tp = inferior_thread ();
+ }
}
- /* prepare_to_proceed may change the current thread. */
- tp = inferior_thread ();
-
if (force_step)
tp->control.trap_expected = 1;
@@ -2434,8 +2420,6 @@ init_wait_for_inferior (void)
clear_proceed_status ();
- stepping_past_singlestep_breakpoint = 0;
-
target_last_wait_ptid = minus_one_ptid;
previous_inferior_ptid = inferior_ptid;
@@ -2443,6 +2427,9 @@ init_wait_for_inferior (void)
/* Discard any skipped inlined frames. */
clear_inline_frame_state (minus_one_ptid);
+
+ singlestep_ptid = null_ptid;
+ singlestep_pc = 0;
}
\f
@@ -2453,7 +2440,6 @@ init_wait_for_inferior (void)
enum infwait_states
{
infwait_normal_state,
- infwait_thread_hop_state,
infwait_step_watch_state,
infwait_nonstep_watch_state
};
@@ -2484,6 +2470,12 @@ struct execution_control_state
infwait_nonstep_watch_state state, and the thread reported an
event. */
int stepped_after_stopped_by_watchpoint;
+
+ /* True if the event thread hit the single-step breakpoint of
+ another thread. Thus the event doesn't cause a stop, the thread
+ needs to be single-stepped past the single-step breakpoint before
+ we can switch back to the original stepping thread. */
+ int hit_singlestep_breakpoint;
};
static void handle_inferior_event (struct execution_control_state *ecs);
@@ -3364,11 +3356,6 @@ handle_inferior_event (struct execution_control_state *ecs)
switch (infwait_state)
{
- case infwait_thread_hop_state:
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog, "infrun: infwait_thread_hop_state\n");
- break;
-
case infwait_normal_state:
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: infwait_normal_state\n");
@@ -3937,163 +3924,6 @@ handle_signal_stop (struct execution_control_state *ecs)
return;
}
- if (stepping_past_singlestep_breakpoint)
- {
- gdb_assert (singlestep_breakpoints_inserted_p);
- gdb_assert (ptid_equal (singlestep_ptid, ecs->ptid));
- gdb_assert (!ptid_equal (singlestep_ptid, saved_singlestep_ptid));
-
- stepping_past_singlestep_breakpoint = 0;
-
- /* We've either finished single-stepping past the single-step
- breakpoint, or stopped for some other reason. It would be nice if
- we could tell, but we can't reliably. */
- if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
- {
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog,
- "infrun: stepping_past_"
- "singlestep_breakpoint\n");
- /* Pull the single step breakpoints out of the target. */
- if (!ptid_equal (ecs->ptid, inferior_ptid))
- context_switch (ecs->ptid);
- remove_single_step_breakpoints ();
- singlestep_breakpoints_inserted_p = 0;
-
- ecs->event_thread->control.trap_expected = 0;
-
- context_switch (saved_singlestep_ptid);
- if (deprecated_context_hook)
- deprecated_context_hook (pid_to_thread_id (saved_singlestep_ptid));
-
- resume (1, GDB_SIGNAL_0);
- prepare_to_wait (ecs);
- return;
- }
- }
-
- /* See if a thread hit a thread-specific breakpoint that was meant for
- another thread. If so, then step that thread past the breakpoint,
- and continue it. */
-
- if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
- {
- int thread_hop_needed = 0;
- struct address_space *aspace =
- get_regcache_aspace (get_thread_regcache (ecs->ptid));
-
- /* Check if a regular breakpoint has been hit before checking
- for a potential single step breakpoint. Otherwise, GDB will
- not see this breakpoint hit when stepping onto breakpoints. */
- if (regular_breakpoint_inserted_here_p (aspace, stop_pc))
- {
- if (!breakpoint_thread_match (aspace, stop_pc, ecs->ptid))
- thread_hop_needed = 1;
- }
- else if (singlestep_breakpoints_inserted_p)
- {
- /* We have not context switched yet, so this should be true
- no matter which thread hit the singlestep breakpoint. */
- gdb_assert (ptid_equal (inferior_ptid, singlestep_ptid));
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog, "infrun: software single step "
- "trap for %s\n",
- target_pid_to_str (ecs->ptid));
-
- /* The call to in_thread_list is necessary because PTIDs sometimes
- change when we go from single-threaded to multi-threaded. If
- the singlestep_ptid is still in the list, assume that it is
- really different from ecs->ptid. */
- if (!ptid_equal (singlestep_ptid, ecs->ptid)
- && in_thread_list (singlestep_ptid))
- {
- /* If the PC of the thread we were trying to single-step
- has changed, discard this event (which we were going
- to ignore anyway), and pretend we saw that thread
- trap. This prevents us continuously moving the
- single-step breakpoint forward, one instruction at a
- time. If the PC has changed, then the thread we were
- trying to single-step has trapped or been signalled,
- but the event has not been reported to GDB yet.
-
- There might be some cases where this loses signal
- information, if a signal has arrived at exactly the
- same time that the PC changed, but this is the best
- we can do with the information available. Perhaps we
- should arrange to report all events for all threads
- when they stop, or to re-poll the remote looking for
- this particular thread (i.e. temporarily enable
- schedlock). */
-
- CORE_ADDR new_singlestep_pc
- = regcache_read_pc (get_thread_regcache (singlestep_ptid));
-
- if (new_singlestep_pc != singlestep_pc)
- {
- enum gdb_signal stop_signal;
-
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog, "infrun: unexpected thread,"
- " but expected thread advanced also\n");
-
- /* The current context still belongs to
- singlestep_ptid. Don't swap here, since that's
- the context we want to use. Just fudge our
- state and continue. */
- stop_signal = ecs->event_thread->suspend.stop_signal;
- ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_0;
- ecs->ptid = singlestep_ptid;
- ecs->event_thread = find_thread_ptid (ecs->ptid);
- ecs->event_thread->suspend.stop_signal = stop_signal;
- stop_pc = new_singlestep_pc;
- }
- else
- {
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog,
- "infrun: unexpected thread\n");
-
- thread_hop_needed = 1;
- stepping_past_singlestep_breakpoint = 1;
- saved_singlestep_ptid = singlestep_ptid;
- }
- }
- }
-
- if (thread_hop_needed)
- {
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog, "infrun: thread_hop_needed\n");
-
- /* Switch context before touching inferior memory, the
- previous thread may have exited. */
- if (!ptid_equal (inferior_ptid, ecs->ptid))
- context_switch (ecs->ptid);
-
- /* Saw a breakpoint, but it was hit by the wrong thread.
- Just continue. */
-
- if (singlestep_breakpoints_inserted_p)
- {
- /* Pull the single step breakpoints out of the target. */
- remove_single_step_breakpoints ();
- singlestep_breakpoints_inserted_p = 0;
- }
-
- if (!non_stop)
- {
- /* Only need to require the next event from this thread
- in all-stop mode. */
- waiton_ptid = ecs->ptid;
- infwait_state = infwait_thread_hop_state;
- }
-
- ecs->event_thread->stepping_over_breakpoint = 1;
- keep_going (ecs);
- return;
- }
- }
-
/* See if something interesting happened to the non-current thread. If
so, then switch to that thread. */
if (!ptid_equal (ecs->ptid, inferior_ptid))
@@ -4111,9 +3941,36 @@ handle_signal_stop (struct execution_control_state *ecs)
frame = get_current_frame ();
gdbarch = get_frame_arch (frame);
+ /* Pull the single step breakpoints out of the target. */
if (singlestep_breakpoints_inserted_p)
{
- /* Pull the single step breakpoints out of the target. */
+ /* However, before doing so, if this single-step breakpoint was
+ actually for another thread, set this thread up for moving
+ past it. */
+ if (!ptid_equal (ecs->ptid, singlestep_ptid)
+ && ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
+ {
+ struct regcache *regcache;
+ struct address_space *aspace;
+ CORE_ADDR pc;
+
+ regcache = get_thread_regcache (ecs->ptid);
+ aspace = get_regcache_aspace (regcache);
+ pc = regcache_read_pc (regcache);
+ if (single_step_breakpoint_inserted_here_p (aspace, pc))
+ {
+ if (debug_infrun)
+ {
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: [%s] hit step over single-step"
+ " breakpoint of [%s]\n",
+ target_pid_to_str (ecs->ptid),
+ target_pid_to_str (singlestep_ptid));
+ }
+ ecs->hit_singlestep_breakpoint = 1;
+ }
+ }
+
remove_single_step_breakpoints ();
singlestep_breakpoints_inserted_p = 0;
}
@@ -4308,6 +4165,12 @@ handle_signal_stop (struct execution_control_state *ecs)
random_signal = !(ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP
&& currently_stepping (ecs->event_thread));
+ /* Perhaps the thread hit a single-step breakpoint of _another_
+ thread. Single-step breakpoints are transparent to the
+ breakpoints module. */
+ if (random_signal)
+ random_signal = !ecs->hit_singlestep_breakpoint;
+
/* No? Perhaps we got a moribund watchpoint. */
if (random_signal)
random_signal = !stopped_by_watchpoint;
@@ -5271,12 +5134,16 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
ecs->event_thread);
if (tp)
{
+ struct frame_info *frame;
+ struct gdbarch *gdbarch;
+
/* However, if the current thread is blocked on some internal
breakpoint, and we simply need to step over that breakpoint
to get it going again, do that first. */
if ((ecs->event_thread->control.trap_expected
&& ecs->event_thread->suspend.stop_signal != GDB_SIGNAL_TRAP)
- || ecs->event_thread->stepping_over_breakpoint)
+ || ecs->event_thread->stepping_over_breakpoint
+ || ecs->hit_singlestep_breakpoint)
{
keep_going (ecs);
return 1;
@@ -5326,7 +5193,52 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
ecs->event_thread = tp;
ecs->ptid = tp->ptid;
context_switch (ecs->ptid);
- keep_going (ecs);
+
+ stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
+ frame = get_current_frame ();
+ gdbarch = get_frame_arch (frame);
+
+ /* If the PC of the thread we were trying to single-step has
+ changed, then the thread we were trying to single-step
+ has trapped or been signalled, but the event has not been
+ reported to GDB yet. Re-poll the remote looking for this
+ particular thread (i.e. temporarily enable schedlock) by:
+
+ - setting a break at the current PC
+ - resuming that particular thread, only (by setting
+ trap expected)
+
+ This prevents us continuously moving the single-step
+ breakpoint forward, one instruction at a time,
+ overstepping. */
+
+ if (gdbarch_software_single_step_p (gdbarch)
+ && stop_pc != tp->prev_pc)
+ {
+ if (debug_infrun)
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: expected thread advanced also\n");
+
+ insert_single_step_breakpoint (get_frame_arch (frame),
+ get_frame_address_space (frame),
+ stop_pc);
+ singlestep_breakpoints_inserted_p = 1;
+ ecs->event_thread->control.trap_expected = 1;
+ singlestep_ptid = inferior_ptid;
+ singlestep_pc = stop_pc;
+
+ resume (0, GDB_SIGNAL_0);
+ prepare_to_wait (ecs);
+ }
+ else
+ {
+ if (debug_infrun)
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: expected thread still "
+ "hasn't advanced\n");
+ keep_going (ecs);
+ }
+
return 1;
}
}
@@ -5799,7 +5711,8 @@ keep_going (struct execution_control_state *ecs)
(watchpoints, etc.) but the one we're stepping over, step one
instruction, and then re-insert the breakpoint when that step
is finished. */
- if (ecs->event_thread->stepping_over_breakpoint
+ if ((ecs->hit_singlestep_breakpoint
+ || ecs->event_thread->stepping_over_breakpoint)
&& !use_displaced_stepping (get_regcache_arch (regcache)))
{
set_step_over_info (get_regcache_aspace (regcache),
@@ -5821,7 +5734,8 @@ keep_going (struct execution_control_state *ecs)
}
ecs->event_thread->control.trap_expected
- = ecs->event_thread->stepping_over_breakpoint;
+ = (ecs->event_thread->stepping_over_breakpoint
+ || ecs->hit_singlestep_breakpoint);
/* Do not deliver GDB_SIGNAL_TRAP (except when the user
explicitly specifies that such a signal should be delivered
diff --git a/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.c b/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.c
new file mode 100644
index 0000000..4e8ca9c
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.c
@@ -0,0 +1,67 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 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 <pthread.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+pthread_barrier_t barrier;
+pthread_t child_thread;
+
+volatile unsigned int counter = 1;
+volatile unsigned int watch_me;
+volatile unsigned int other;
+
+void *
+child_function (void *arg)
+{
+ pthread_barrier_wait (&barrier);
+
+ while (counter > 0)
+ {
+ counter++;
+
+ watch_me = 1; /* set breakpoint child here */
+ other = 1; /* set thread-specific breakpoint here */
+ usleep (1);
+ }
+
+ pthread_exit (NULL);
+}
+
+static int
+wait_threads (void)
+{
+ return 1; /* in wait_threads */
+}
+
+int
+main ()
+{
+ int res;
+ long i;
+
+ pthread_barrier_init (&barrier, NULL, 2);
+
+ res = pthread_create (&child_thread, NULL, child_function, NULL);
+ pthread_barrier_wait (&barrier);
+ wait_threads (); /* set wait-thread breakpoint here */
+
+ pthread_join (child_thread, NULL);
+
+ exit (EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.exp b/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.exp
new file mode 100644
index 0000000..c6c58cc
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.exp
@@ -0,0 +1,90 @@
+# Copyright (C) 2014 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/>.
+
+# Test that when a step-over trips on a watchpoint, that watchpoint is
+# reported.
+
+standard_testfile
+set executable ${testfile}
+
+# This test verifies that a watchpoint is detected in a multithreaded
+# program so the test is only meaningful on a system with hardware
+# watchpoints.
+if {[skip_hw_watchpoint_tests]} {
+ return 0
+}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+ executable [list debug "incdir=${objdir}"]] != "" } {
+ return -1
+}
+
+proc do_test { with_bp } {
+ global executable
+
+ if ${with_bp} {
+ set prefix "with thread-specific bp"
+ } else {
+ set prefix "no thread-specific bp"
+ }
+ with_test_prefix $prefix {
+ # Cover both stepping and non-stepping execution commands.
+ foreach command {"step" "next" "continue" } {
+ with_test_prefix $command {
+ clean_restart $executable
+
+ if ![runto_main] {
+ continue
+ }
+
+ gdb_breakpoint [gdb_get_line_number "set wait-thread breakpoint here"]
+ gdb_continue_to_breakpoint "run to wait-thread breakpoint"
+ gdb_test "info threads" "2 .*\\\* 1.*" "info threads shows all threads"
+
+ gdb_test_no_output "set scheduler-locking on"
+
+ delete_breakpoints
+
+ gdb_breakpoint [gdb_get_line_number "set breakpoint child here"]
+ gdb_test "thread 2" "Switching to .*"
+ gdb_continue_to_breakpoint "run to breakpoint in thread 2"
+ gdb_test "p counter = 0" " = 0" "unbreak loop in thread 2"
+ gdb_test "p watch_me = 0" " = 0" "clear watch_me"
+ gdb_test "watch watch_me" "Hardware watchpoint .*"
+
+ if ${with_bp} {
+ # Set a thread-specific breakpoint (for the wrong
+ # thread) right after instruction that triggers
+ # the watchpoint.
+ set linenum [gdb_get_line_number "set thread-specific breakpoint here"]
+ gdb_test "b $linenum thread 1"
+ }
+
+ # Switch back to thread 1 and disable scheduler locking.
+ gdb_test "thread 1" "Switching to .*"
+ gdb_test_no_output "set scheduler-locking off"
+
+ # Thread 2 is still stopped at a breakpoint that needs to be
+ # stepped over before proceeding thread 1. However, right
+ # where the step-over lands there's another breakpoint
+ # installed, which should trap and be reported to the user.
+ gdb_test "$command" "Hardware watchpoint.*: watch_me.*New value = 1.*"
+ }
+ }
+ }
+}
+
+do_test 0
+do_test 1
--
1.7.11.7
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 4/5] Handle multiple step-overs.
2014-03-07 1:10 [PATCH v3 0/5] Fix lost events, and handle multiple step-overs Pedro Alves
@ 2014-03-07 1:10 ` Pedro Alves
2014-03-07 1:10 ` [PATCH v3 1/5] Fix missing breakpoint/watchpoint hits, eliminate deferred_step_ptid Pedro Alves
` (4 subsequent siblings)
5 siblings, 0 replies; 25+ messages in thread
From: Pedro Alves @ 2014-03-07 1:10 UTC (permalink / raw)
To: gdb-patches
This test fails with current mainline.
If the program stopped for a breakpoint in thread 1, and then the user
switches to thread 2, and resumes the program, GDB first switches back
to thread 1 to step it over the breakpoint, in order to make progress.
However, that logic only considers the last reported event, assuming
only one thread needs that stepping over dance.
That's actually not true when we play with scheduler-locking. The
patch adds an example to the testsuite of multiple threads needing a
step-over before the stepping thread can be resumed. With current
mainline, the program re-traps the same breakpoint it had already
trapped before.
E.g.:
Breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:99
99 wait_threads (); /* set wait-threads breakpoint here */
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: continue to breakpoint: run to breakpoint
info threads
Id Target Id Frame
3 Thread 0x7ffff77c9700 (LWP 4310) "multiple-step-o" 0x00000000004007ca in child_function_3 (arg=0x1) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:43
2 Thread 0x7ffff7fca700 (LWP 4309) "multiple-step-o" 0x0000000000400827 in child_function_2 (arg=0x0) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:60
* 1 Thread 0x7ffff7fcb740 (LWP 4305) "multiple-step-o" main () at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:99
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: info threads shows all threads
set scheduler-locking on
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: set scheduler-locking on
break 44
Breakpoint 3 at 0x4007d3: file ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c, line 44.
(gdb) break 61
Breakpoint 4 at 0x40082d: file ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c, line 61.
(gdb) thread 3
[Switching to thread 3 (Thread 0x7ffff77c9700 (LWP 4310))]
#0 0x00000000004007ca in child_function_3 (arg=0x1) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:43
43 (*myp) ++;
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: thread 3
continue
Continuing.
Breakpoint 3, child_function_3 (arg=0x1) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:44
44 callme (); /* set breakpoint thread 3 here */
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: continue to breakpoint: run to breakpoint in thread 3
p *myp = 0
$1 = 0
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: unbreak loop in thread 3
thread 2
[Switching to thread 2 (Thread 0x7ffff7fca700 (LWP 4309))]
#0 0x0000000000400827 in child_function_2 (arg=0x0) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:60
60 (*myp) ++;
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: thread 2
continue
Continuing.
Breakpoint 4, child_function_2 (arg=0x0) at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:61
61 callme (); /* set breakpoint thread 2 here */
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: continue to breakpoint: run to breakpoint in thread 2
p *myp = 0
$2 = 0
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: unbreak loop in thread 2
thread 1
[Switching to thread 1 (Thread 0x7ffff7fcb740 (LWP 4305))]
#0 main () at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:99
99 wait_threads (); /* set wait-threads breakpoint here */
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: thread 1
set scheduler-locking off
(gdb) PASS: gdb.threads/multiple-step-overs.exp: step: set scheduler-locking off
At this point all thread are stopped for a breakpoint that needs stepping over.
(gdb) step
Breakpoint 2, main () at ../../../src/gdb/testsuite/gdb.threads/multiple-step-overs.c:99
99 wait_threads (); /* set wait-threads breakpoint here */
(gdb) FAIL: gdb.threads/multiple-step-overs.exp: step
But that "step" retriggers the same breakpoint instead of making
progress.
The patch teaches GDB to step over all breakpoints of all threads
before resuming the stepping thread.
Tested on x86_64 Fedora 17, against pristine mainline, and also my
branch that implements software single-stepping on x86.
gdb/
2014-03-05 Pedro Alves <palves@redhat.com>
* infrun.c (prepare_to_proceed): Delete.
(thread_still_needs_step_over): New function.
(find_thread_needs_step_over): New function.
(proceed): If the current thread needs a step-over, set its
steping_over_breakpoint flag. Adjust to use
find_thread_needs_step_over instead of prepare_to_proceed.
(process_event_stop_test): For BPSTAT_WHAT_STOP_NOISY and
BPSTAT_WHAT_STOP_SILENT, assume the thread stopped for a
breakpoint.
(switch_back_to_stepped_thread): Step over breakpoints of all
threads not the stepping thread, before switching back to the
stepping thread.
gdb/testsuite/
2014-03-05 Pedro Alves <palves@redhat.com>
* gdb.threads/multiple-step-overs.c: New file.
* gdb.threads/multiple-step-overs.exp: New file.
* gdb.threads/signal-while-stepping-over-bp-other-thread.exp:
Adjust expected infrun debug output.
---
gdb/infrun.c | 269 +++++++++++++--------
gdb/testsuite/gdb.threads/multiple-step-overs.c | 105 ++++++++
gdb/testsuite/gdb.threads/multiple-step-overs.exp | 80 ++++++
.../signal-while-stepping-over-bp-other-thread.exp | 2 +-
4 files changed, 351 insertions(+), 105 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/multiple-step-overs.c
create mode 100644 gdb/testsuite/gdb.threads/multiple-step-overs.exp
diff --git a/gdb/infrun.c b/gdb/infrun.c
index b7e2fd8..ef17f12 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -90,8 +90,6 @@ static int currently_stepping_or_nexting_callback (struct thread_info *tp,
static void xdb_handle_command (char *args, int from_tty);
-static int prepare_to_proceed (int);
-
static void print_exited_reason (int exitstatus);
static void print_signal_exited_reason (enum gdb_signal siggnal);
@@ -2088,75 +2086,74 @@ clear_proceed_status (void)
}
}
-/* Check the current thread against the thread that reported the most recent
- event. If a step-over is required return TRUE and set the current thread
- to the old thread. Otherwise return FALSE.
-
- This should be suitable for any targets that support threads. */
+/* Returns true if TP is still stopped at a breakpoint that needs
+ stepping-over in order to make progress. If the breakpoint is gone
+ meanwhile, we can skip the whole step-over dance. */
static int
-prepare_to_proceed (int step)
+thread_still_needs_step_over (struct thread_info *tp)
+{
+ if (tp->stepping_over_breakpoint)
+ {
+ struct regcache *regcache = get_thread_regcache (tp->ptid);
+
+ if (breakpoint_here_p (get_regcache_aspace (regcache),
+ regcache_read_pc (regcache)))
+ return 1;
+
+ tp->stepping_over_breakpoint = 0;
+ }
+
+ return 0;
+}
+
+/* Look a thread other than EXCEPT that has previously reported a
+ breakpoint event, and thus needs a step-over in order to make
+ progress. Returns NULL is none is found. STEP indicates whether
+ we're about to step the current thread, in order to decide whether
+ "set scheduler-locking step" applies. */
+
+static struct thread_info *
+find_thread_needs_step_over (int step, struct thread_info *except)
{
- ptid_t wait_ptid;
- struct target_waitstatus wait_status;
int schedlock_enabled;
+ struct thread_info *tp, *current;
/* With non-stop mode on, threads are always handled individually. */
gdb_assert (! non_stop);
- /* Get the last target status returned by target_wait(). */
- get_last_target_status (&wait_ptid, &wait_status);
-
- /* Make sure we were stopped at a breakpoint. */
- if (wait_status.kind != TARGET_WAITKIND_STOPPED
- || (wait_status.value.sig != GDB_SIGNAL_TRAP
- && wait_status.value.sig != GDB_SIGNAL_ILL
- && wait_status.value.sig != GDB_SIGNAL_SEGV
- && wait_status.value.sig != GDB_SIGNAL_EMT))
- {
- return 0;
- }
-
schedlock_enabled = (scheduler_mode == schedlock_on
|| (scheduler_mode == schedlock_step
&& step));
- /* Don't switch over to WAIT_PTID if scheduler locking is on. */
- if (schedlock_enabled)
- return 0;
-
- /* Don't switch over if we're about to resume some other process
- other than WAIT_PTID's, and schedule-multiple is off. */
- if (!sched_multi
- && ptid_get_pid (wait_ptid) != ptid_get_pid (inferior_ptid))
- return 0;
+ current = inferior_thread ();
- /* Switched over from WAIT_PID. */
- if (!ptid_equal (wait_ptid, minus_one_ptid)
- && !ptid_equal (inferior_ptid, wait_ptid))
+ /* If scheduler locking applies, we can avoid iterating over all
+ threads. */
+ if (schedlock_enabled)
{
- struct regcache *regcache = get_thread_regcache (wait_ptid);
+ if (except != current
+ && thread_still_needs_step_over (current))
+ return current;
- if (breakpoint_here_p (get_regcache_aspace (regcache),
- regcache_read_pc (regcache)))
- {
- /* Switch back to WAIT_PID thread. */
- switch_to_thread (wait_ptid);
-
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog,
- "infrun: prepare_to_proceed (step=%d), "
- "switched to [%s]\n",
- step, target_pid_to_str (inferior_ptid));
+ return NULL;
+ }
- /* We return 1 to indicate that there is a breakpoint here,
- so we need to step over it before continuing to avoid
- hitting it straight away. */
- return 1;
- }
+ ALL_THREADS (tp)
+ {
+ /* Ignore the EXCEPT thread. */
+ if (tp == except)
+ continue;
+ /* Ignore threads of processes we're not resuming. */
+ if (!sched_multi
+ && ptid_get_pid (tp->ptid) != ptid_get_pid (inferior_ptid))
+ continue;
+
+ if (thread_still_needs_step_over (tp))
+ return tp;
}
- return 0;
+ return NULL;
}
/* Basic routine for continuing the program in various fashions.
@@ -2179,8 +2176,6 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
struct thread_info *tp;
CORE_ADDR pc;
struct address_space *aspace;
- /* GDB may force the inferior to step due to various reasons. */
- int force_step = 0;
/* If we're stopped at a fork/vfork, follow the branch set by the
"set follow-fork-mode" command; otherwise, we'll just proceed
@@ -2208,6 +2203,9 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
if (step < 0)
stop_after_trap = 1;
+ /* Fill in with reasonable starting values. */
+ init_thread_stepping_state (tp);
+
if (addr == (CORE_ADDR) -1)
{
if (pc == stop_pc && breakpoint_here_p (aspace, pc)
@@ -2220,14 +2218,13 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
Note, we don't do this in reverse, because we won't
actually be executing the breakpoint insn anyway.
We'll be (un-)executing the previous instruction. */
-
- force_step = 1;
+ tp->stepping_over_breakpoint = 1;
else if (gdbarch_single_step_through_delay_p (gdbarch)
&& gdbarch_single_step_through_delay (gdbarch,
get_current_frame ()))
/* We stepped onto an instruction that needs to be stepped
again before re-inserting the breakpoint, do so. */
- force_step = 1;
+ tp->stepping_over_breakpoint = 1;
}
else
{
@@ -2246,6 +2243,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
;
else
{
+ struct thread_info *step_over;
+
/* In a multi-threaded task we may select another thread and
then continue or step.
@@ -2254,31 +2253,29 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
execution (i.e. it will report a breakpoint hit incorrectly).
So we must step over it first.
- prepare_to_proceed checks the current thread against the
- thread that reported the most recent event. If a step-over
- is required it returns TRUE and sets the current thread to
- the old thread. */
-
- /* Store the prev_pc for the stepping thread too, needed by
- switch_back_to_stepping thread. */
- tp->prev_pc = regcache_read_pc (get_current_regcache ());
-
- if (prepare_to_proceed (step))
+ Look for a thread other than the current (TP) that reported a
+ breakpoint hit and hasn't been resumed yet since. */
+ step_over = find_thread_needs_step_over (step, tp);
+ if (step_over != NULL)
{
- force_step = 1;
- /* The current thread changed. */
- tp = inferior_thread ();
+ if (debug_infrun)
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: need to step-over [%s] first\n",
+ target_pid_to_str (step_over->ptid));
+
+ /* Store the prev_pc for the stepping thread too, needed by
+ switch_back_to_stepping thread. */
+ tp->prev_pc = regcache_read_pc (get_current_regcache ());
+ switch_to_thread (step_over->ptid);
+ tp = step_over;
}
}
- if (force_step)
- tp->control.trap_expected = 1;
-
/* If we need to step over a breakpoint, and we're not using
displaced stepping to do so, insert all breakpoints (watchpoints,
etc.) but the one we're stepping over, step one instruction, and
then re-insert the breakpoint when that step is finished. */
- if (tp->control.trap_expected && !use_displaced_stepping (gdbarch))
+ if (tp->stepping_over_breakpoint && !use_displaced_stepping (gdbarch))
{
struct regcache *regcache = get_current_regcache ();
@@ -2290,6 +2287,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
insert_breakpoints ();
+ tp->control.trap_expected = tp->stepping_over_breakpoint;
+
if (!non_stop)
{
/* Pass the last stop signal to the thread we're resuming,
@@ -2353,14 +2352,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
correctly when the inferior is stopped. */
tp->prev_pc = regcache_read_pc (get_current_regcache ());
- /* Fill in with reasonable starting values. */
- init_thread_stepping_state (tp);
-
/* Reset to normal state. */
init_infwait_state ();
/* Resume inferior. */
- resume (force_step || step || bpstat_should_step (),
+ resume (tp->control.trap_expected || step || bpstat_should_step (),
tp->suspend.stop_signal);
/* Wait for it to stop (if not standalone)
@@ -4491,8 +4487,10 @@ process_event_stop_test (struct execution_control_state *ecs)
fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_STOP_NOISY\n");
stop_print_frame = 1;
- /* We are about to nuke the step_resume_breakpointt via the
- cleanup chain, so no need to worry about it here. */
+ /* Assume the thread stopped for a breapoint. We'll still check
+ whether a/the breakpoint is there when the thread is next
+ resumed. */
+ ecs->event_thread->stepping_over_breakpoint = 1;
stop_stepping (ecs);
return;
@@ -4502,9 +4500,10 @@ process_event_stop_test (struct execution_control_state *ecs)
fprintf_unfiltered (gdb_stdlog, "infrun: BPSTAT_WHAT_STOP_SILENT\n");
stop_print_frame = 0;
- /* We are about to nuke the step_resume_breakpoin via the
- cleanup chain, so no need to worry about it here. */
-
+ /* Assume the thread stopped for a breapoint. We'll still check
+ whether a/the breakpoint is there when the thread is next
+ resumed. */
+ ecs->event_thread->stepping_over_breakpoint = 1;
stop_stepping (ecs);
return;
@@ -5129,26 +5128,88 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
if (!non_stop)
{
struct thread_info *tp;
-
- tp = iterate_over_threads (currently_stepping_or_nexting_callback,
- ecs->event_thread);
- if (tp)
+ struct thread_info *stepping_thread;
+
+ /* If any thread is blocked on some internal breakpoint, and we
+ simply need to step over that breakpoint to get it going
+ again, do that first. */
+
+ /* However, if we see an event for the stepping thread, then we
+ know all other threads have been moved past their breakpoints
+ already. Let the caller check whether the step is finished,
+ etc., before deciding to move it past a breakpoint. */
+ if (ecs->event_thread->control.step_range_end != 0)
+ return 0;
+
+ /* Check if the current thread is blocked on an incomplete
+ step-over, interrupted by a random signal. */
+ if (ecs->event_thread->control.trap_expected
+ && ecs->event_thread->suspend.stop_signal != GDB_SIGNAL_TRAP)
{
- struct frame_info *frame;
- struct gdbarch *gdbarch;
+ if (debug_infrun)
+ {
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: need to finish step-over of [%s]\n",
+ target_pid_to_str (ecs->event_thread->ptid));
+ }
+ keep_going (ecs);
+ return 1;
+ }
- /* However, if the current thread is blocked on some internal
- breakpoint, and we simply need to step over that breakpoint
- to get it going again, do that first. */
- if ((ecs->event_thread->control.trap_expected
- && ecs->event_thread->suspend.stop_signal != GDB_SIGNAL_TRAP)
- || ecs->event_thread->stepping_over_breakpoint
- || ecs->hit_singlestep_breakpoint)
+ /* Check if the current thread is blocked by a single-step
+ breakpoint of another thread. */
+ if (ecs->hit_singlestep_breakpoint)
+ {
+ if (debug_infrun)
+ {
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: need to step [%s] over single-step "
+ "breakpoint\n",
+ target_pid_to_str (ecs->ptid));
+ }
+ keep_going (ecs);
+ return 1;
+ }
+
+ stepping_thread
+ = iterate_over_threads (currently_stepping_or_nexting_callback,
+ ecs->event_thread);
+
+ /* Check if any other thread except the stepping thread that
+ needs to start a step-over. Do that before actually
+ proceeding with step/next/etc. */
+ tp = find_thread_needs_step_over (stepping_thread != NULL,
+ stepping_thread);
+ if (tp != NULL)
+ {
+ if (debug_infrun)
{
- keep_going (ecs);
- return 1;
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: need to step-over [%s]\n",
+ target_pid_to_str (tp->ptid));
}
+ gdb_assert (!tp->control.trap_expected);
+ gdb_assert (tp->control.step_range_end == 0);
+
+ /* We no longer expect a trap in the current thread. Clear
+ the trap_expected flag before switching. This is what
+ keep_going would do as well, if we called it. */
+ ecs->event_thread->control.trap_expected = 0;
+
+ ecs->ptid = tp->ptid;
+ ecs->event_thread = tp;
+ switch_to_thread (ecs->ptid);
+ keep_going (ecs);
+ return 1;
+ }
+
+ tp = stepping_thread;
+ if (tp != NULL)
+ {
+ struct frame_info *frame;
+ struct gdbarch *gdbarch;
+
/* If the stepping thread exited, then don't try to switch
back and resume it, which could fail in several different
ways depending on the target. Instead, just keep going.
@@ -5199,10 +5260,10 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
gdbarch = get_frame_arch (frame);
/* If the PC of the thread we were trying to single-step has
- changed, then the thread we were trying to single-step
- has trapped or been signalled, but the event has not been
- reported to GDB yet. Re-poll the remote looking for this
- particular thread (i.e. temporarily enable schedlock) by:
+ changed, then that thread has trapped or been signaled,
+ but the event has not been reported to GDB yet. Re-poll
+ the target looking for this particular thread's event
+ (i.e. temporarily enable schedlock) by:
- setting a break at the current PC
- resuming that particular thread, only (by setting
@@ -5712,7 +5773,7 @@ keep_going (struct execution_control_state *ecs)
instruction, and then re-insert the breakpoint when that step
is finished. */
if ((ecs->hit_singlestep_breakpoint
- || ecs->event_thread->stepping_over_breakpoint)
+ || thread_still_needs_step_over (ecs->event_thread))
&& !use_displaced_stepping (get_regcache_arch (regcache)))
{
set_step_over_info (get_regcache_aspace (regcache),
diff --git a/gdb/testsuite/gdb.threads/multiple-step-overs.c b/gdb/testsuite/gdb.threads/multiple-step-overs.c
new file mode 100644
index 0000000..523a88a
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/multiple-step-overs.c
@@ -0,0 +1,105 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2009-2014 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 <pthread.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <signal.h>
+
+unsigned int args[2];
+
+pthread_barrier_t barrier;
+pthread_t child_thread_2, child_thread_3;
+
+void
+callme (void)
+{
+}
+
+void *
+child_function_3 (void *arg)
+{
+ int my_number = (long) arg;
+ volatile int *myp = (int *) &args[my_number];
+
+ pthread_barrier_wait (&barrier);
+
+ while (*myp > 0)
+ {
+ (*myp) ++;
+ callme (); /* set breakpoint thread 3 here */
+ }
+
+ pthread_exit (NULL);
+}
+
+void *
+child_function_2 (void *arg)
+{
+ int my_number = (long) arg;
+ volatile int *myp = (int *) &args[my_number];
+
+ pthread_barrier_wait (&barrier);
+
+ while (*myp > 0)
+ {
+ (*myp) ++;
+ callme (); /* set breakpoint thread 2 here */
+ }
+
+ pthread_exit (NULL);
+}
+
+static int
+wait_threads (void)
+{
+ return 1; /* in wait_threads */
+}
+
+int
+main ()
+{
+ int res;
+ long i;
+
+ /* Call these early so that PLTs for these are resolved soon,
+ instead of in the threads. RTLD_NOW should work as well. */
+ usleep (0);
+ pthread_barrier_init (&barrier, NULL, 1);
+ pthread_barrier_wait (&barrier);
+
+ pthread_barrier_init (&barrier, NULL, 2);
+
+ i = 0;
+ args[i] = 1;
+ res = pthread_create (&child_thread_2,
+ NULL, child_function_2, (void *) i);
+ pthread_barrier_wait (&barrier);
+ callme ();
+
+ i = 1;
+ args[i] = 1;
+ res = pthread_create (&child_thread_3,
+ NULL, child_function_3, (void *) i);
+ pthread_barrier_wait (&barrier);
+ wait_threads (); /* set wait-threads breakpoint here */
+
+ pthread_join (child_thread_2, NULL);
+ pthread_join (child_thread_3, NULL);
+
+ exit(EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.threads/multiple-step-overs.exp b/gdb/testsuite/gdb.threads/multiple-step-overs.exp
new file mode 100644
index 0000000..c79e5d2
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/multiple-step-overs.exp
@@ -0,0 +1,80 @@
+# Copyright (C) 2011-2014 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/>.
+
+# Test that GDB steps over all breakpoints of threads not the stepping
+# thread, before actually proceeding with the stepped thread.
+
+standard_testfile
+set executable ${testfile}
+
+if [target_info exists gdb,nosignals] {
+ verbose "Skipping ${testfile}.exp because of nosignals."
+ return -1
+}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+ executable [list debug "incdir=${objdir}"]] != "" } {
+ return -1
+}
+
+# Prepare environment for test. PREFIX is used as prefix in test
+# messages.
+
+proc setup { prefix } {
+ global executable
+
+ with_test_prefix $prefix {
+ clean_restart $executable
+
+ if ![runto_main] {
+ return -1
+ }
+
+ gdb_breakpoint [gdb_get_line_number "set wait-threads breakpoint here"]
+ gdb_continue_to_breakpoint "run to breakpoint"
+ gdb_test "info threads" "3 .* 2 .*\\\* 1.*" "info threads shows all threads"
+
+ gdb_test_no_output "set scheduler-locking on"
+
+ gdb_breakpoint [gdb_get_line_number "set breakpoint thread 3 here"]
+ gdb_breakpoint [gdb_get_line_number "set breakpoint thread 2 here"]
+
+ gdb_test "thread 3" "Switching.*"
+ gdb_continue_to_breakpoint "run to breakpoint in thread 3"
+ gdb_test "p *myp = 0" " = 0" "unbreak loop in thread 3"
+
+ gdb_test "thread 2" "Switching.*"
+ gdb_continue_to_breakpoint "run to breakpoint in thread 2"
+ gdb_test "p *myp = 0" " = 0" "unbreak loop in thread 2"
+
+ # Switch back to thread 1 and disable scheduler locking.
+ gdb_test "thread 1" "Switching.*"
+ gdb_test_no_output "set scheduler-locking off"
+
+ # Now all 3 threads are stopped for a breakpoint that needs to
+ # be stepped over before thread 1 is resumed.
+ }
+}
+
+setup "step"
+gdb_test "step" "in wait_threads .*"
+
+setup "next"
+gdb_test "set debug infrun 1" ".*"
+gdb_test "next" "pthread_join.*"
+
+setup "continue"
+gdb_breakpoint [gdb_get_line_number "EXIT_SUCCESS"]
+gdb_test "continue" "EXIT_SUCCESS.*"
diff --git a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
index bf5ea60..c46dad2 100644
--- a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
+++ b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
@@ -107,7 +107,7 @@ gdb_test "set debug infrun 1"
gdb_test \
"step" \
- ".*prepare_to_proceed \\(step=1\\), switched to.*signal arrived while stepping over breakpoint.*switching back to stepped thread.*stepped to a different line.*callme.*" \
+ ".*need to step-over.*resume \\(step=1.*signal arrived while stepping over breakpoint.*switching back to stepped thread.*stepped to a different line.*callme.*" \
"step"
set cnt_after [get_value "args\[$my_number\]" "get count after step"]
--
1.7.11.7
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set
2014-03-07 1:10 [PATCH v3 0/5] Fix lost events, and handle multiple step-overs Pedro Alves
` (2 preceding siblings ...)
2014-03-07 1:10 ` [PATCH v3 5/5] Make signal-while-stepping-over-bp-other-thread.exp run against remote targets too Pedro Alves
@ 2014-03-07 1:10 ` Pedro Alves
2014-03-16 3:41 ` Doug Evans
2014-03-07 1:10 ` [PATCH v3 3/5] Fix for even more missed events; eliminate thread-hop code Pedro Alves
2014-03-20 13:59 ` [PUSHED] Re: [PATCH v3 0/5] Fix lost events, and handle multiple step-overs Pedro Alves
5 siblings, 1 reply; 25+ messages in thread
From: Pedro Alves @ 2014-03-07 1:10 UTC (permalink / raw)
To: gdb-patches
Say the program is stopped at a breakpoint, and the user sets a
watchpoint. When the program is next resumed, GDB will first step
over the breakpoint, as explained in the manual:
@value {GDBN} normally ignores breakpoints when it resumes
execution, until at least one instruction has been executed. If it
it did not do this, you would be unable to proceed past a breakpoint
without first disabling the breakpoint. This rule applies whether
or not the breakpoint already existed when your program stopped.
However, GDB currently also removes watchpoints, catchpoints, etc.,
and that means that the first instruction off the breakpoint does not
trigger the watchpoint, catchpoint, etc.
testsuite/gdb.base/watchpoint.exp has a kfail for this.
The PR proposes installing watchpoints only when stepping over a
breakpoint, but that misses catchpoints, etc.
A better fix would instead work from the opposite direction -- remove
only real breakpoints, leaving all other kinds of breakpoints
inserted.
But, going further, it's really a waste to constantly remove/insert
all breakpoints when stepping over a single breakpoint (generating a
pair of RSP z/Z packets for each breakpoint), so the fix goes a step
further and makes GDB remove _only_ the breakpoint being stepped over,
leaving all others installed. This then has the added benefit of
reducing breakpoint-related RSP traffic substancialy when there are
many breakpoints set.
gdb/
2014-03-06 Pedro Alves <palves@redhat.com>
PR breakpoints/7143
* breakpoint.c (should_be_inserted): Don't insert breakpoints that
are being stepped over.
(breakpoint_address_match): Make extern.
* breakpoint.h (breakpoint_address_match): New declaration.
* inferior.h (stepping_past_instruction_at): New declaration.
* infrun.c (struct step_over_info): New type.
(step_over_info): New global.
(set_step_over_info, clear_step_over_info)
(stepping_past_instruction_at): New functions.
(handle_inferior_event): Clear the step-over info when
trap_expected is cleared.
(resume): Remove now stale comment.
(clear_proceed_status): Clear step-over info.
(proceed): Adjust step-over handling to set or clear the step-over
info instead of removing all breakpoints.
(handle_signal_stop): When setting up a thread-hop, don't remove
breakpoints here.
(stop_stepping): Clear step-over info.
(keep_going): Adjust step-over handling to set or clear step-over
info and then always inserting breakpoints, instead of removing
all breakpoints when stepping over one.
gdb/testsuite/
2014-03-06 Pedro Alves <palves@redhat.com>
PR breakpoints/7143
* gdb.base/watchpoint.exp: Mention bugzilla bug number instead of
old gnats gdb/38. Remove kfail. Adjust to use gdb_test instead
of gdb_test_multiple.
* gdb.cp/annota2.exp: Remove kfail for gdb/38.
* gdb.cp/annota3.exp: Remove kfail for gdb/38.
--
v3
- clear up step_over_aspace/step_over_address management.
- lots of polish.
v2
- always clear step_over_aspace and step_over_address when
starting to handle an event.
---
gdb/breakpoint.c | 20 ++--
gdb/breakpoint.h | 10 ++
gdb/inferior.h | 6 +
gdb/infrun.c | 199 +++++++++++++++++++++-------------
gdb/testsuite/gdb.base/watchpoint.exp | 13 +--
gdb/testsuite/gdb.cp/annota2.exp | 3 -
gdb/testsuite/gdb.cp/annota3.exp | 3 -
7 files changed, 155 insertions(+), 99 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1551b99..03b21cb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -165,11 +165,6 @@ static void describe_other_breakpoints (struct gdbarch *,
struct program_space *, CORE_ADDR,
struct obj_section *, int);
-static int breakpoint_address_match (struct address_space *aspace1,
- CORE_ADDR addr1,
- struct address_space *aspace2,
- CORE_ADDR addr2);
-
static int watchpoint_locations_match (struct bp_location *loc1,
struct bp_location *loc2);
@@ -2034,6 +2029,14 @@ should_be_inserted (struct bp_location *bl)
if (bl->pspace->breakpoints_not_allowed)
return 0;
+ /* Don't insert a breakpoint if we're trying to step past its
+ location. */
+ if ((bl->loc_type == bp_loc_software_breakpoint
+ || bl->loc_type == bp_loc_hardware_breakpoint)
+ && stepping_past_instruction_at (bl->pspace->aspace,
+ bl->address))
+ return 0;
+
return 1;
}
@@ -6792,12 +6795,9 @@ watchpoint_locations_match (struct bp_location *loc1,
&& loc1->length == loc2->length);
}
-/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
- same breakpoint location. In most targets, this can only be true
- if ASPACE1 matches ASPACE2. On targets that have global
- breakpoints, the address space doesn't really matter. */
+/* See breakpoint.h. */
-static int
+int
breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
struct address_space *aspace2, CORE_ADDR addr2)
{
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index bf1f52a..b4abcb8 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1135,6 +1135,16 @@ extern int hardware_watchpoint_inserted_in_range (struct address_space *,
extern int breakpoint_thread_match (struct address_space *,
CORE_ADDR, ptid_t);
+/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
+ same breakpoint location. In most targets, this can only be true
+ if ASPACE1 matches ASPACE2. On targets that have global
+ breakpoints, the address space doesn't really matter. */
+
+extern int breakpoint_address_match (struct address_space *aspace1,
+ CORE_ADDR addr1,
+ struct address_space *aspace2,
+ CORE_ADDR addr2);
+
extern void until_break_command (char *, int, int);
/* Initialize a struct bp_location. */
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 9f6375e..6ddca45 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -220,6 +220,12 @@ void set_step_info (struct frame_info *frame, struct symtab_and_line sal);
extern void clear_exit_convenience_vars (void);
+/* Returns true if we're trying to step past the instruction at
+ ADDRESS in ASPACE. */
+
+extern int stepping_past_instruction_at (struct address_space *aspace,
+ CORE_ADDR address);
+
/* From infcmd.c */
extern void post_create_inferior (struct target_ops *, int);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index bd55505..b7c02d7 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -977,6 +977,76 @@ static CORE_ADDR singlestep_pc;
static ptid_t saved_singlestep_ptid;
static int stepping_past_singlestep_breakpoint;
+/* Info about an instruction that is being stepped over. Invalid if
+ ASPACE is NULL. */
+
+struct step_over_info
+{
+ /* The instruction's address space. */
+ struct address_space *aspace;
+
+ /* The instruction's address. */
+ CORE_ADDR address;
+};
+
+/* The step-over info of the location that is being stepped over.
+
+ Note that with async/breakpoint always-inserted mode, a user might
+ set a new breakpoint/watchpoint/etc. exactly while a breakpoint is
+ being stepped over. As setting a new breakpoint inserts all
+ breakpoints, we need to make sure the breakpoint being stepped over
+ isn't inserted then. We do that by only clearing the step-over
+ info when the step-over is actually finished (or aborted).
+
+ Presently GDB can only step over a breakpoint at any given time.
+ Given threads that can't run code in the same address space as the
+ breakpoint's can't really miss the breakpoint, GDB could be taught
+ to step-over at most one breakpoint per address space (so this info
+ could move to the address space object if/when GDB is extended).
+ The set of breakpoints being stepped over will normally be much
+ smaller than the set of all breakpoints, so a flag in the
+ breakpoint location structure would be wasteful. A separate list
+ also saves complexity and run-time, as otherwise we'd have to go
+ through all breakpoint locations clearing their flag whenever we
+ start a new sequence. Similar considerations weigh against storing
+ this info in the thread object. Plus, not all step overs actually
+ have breakpoint locations -- e.g., stepping past a single-step
+ breakpoint, or stepping to complete a non-continuable
+ watchpoint. */
+static struct step_over_info step_over_info;
+
+/* Record the address of the breakpoint/instruction we're currently
+ stepping over. */
+
+static void
+set_step_over_info (struct address_space *aspace, CORE_ADDR address)
+{
+ step_over_info.aspace = aspace;
+ step_over_info.address = address;
+}
+
+/* Called when we're not longer stepping over a breakpoint / an
+ instruction, so all breakpoints are free to be (re)inserted. */
+
+static void
+clear_step_over_info (void)
+{
+ step_over_info.aspace = NULL;
+ step_over_info.address = 0;
+}
+
+/* See inferior.h. */
+
+int
+stepping_past_instruction_at (struct address_space *aspace,
+ CORE_ADDR address)
+{
+ return (step_over_info.aspace != NULL
+ && breakpoint_address_match (aspace, address,
+ step_over_info.aspace,
+ step_over_info.address));
+}
+
\f
/* Displaced stepping. */
@@ -1852,8 +1922,10 @@ a command like `return' or `jump' to continue execution."));
remove_single_step_breakpoints ();
singlestep_breakpoints_inserted_p = 0;
- insert_breakpoints ();
+ clear_step_over_info ();
tp->control.trap_expected = 0;
+
+ insert_breakpoints ();
}
if (should_resume)
@@ -1894,12 +1966,7 @@ a command like `return' or `jump' to continue execution."));
hit, by single-stepping the thread with the breakpoint
removed. In which case, we need to single-step only this
thread, and keep others stopped, as they can miss this
- breakpoint if allowed to run.
-
- The current code actually removes all breakpoints when
- doing this, not just the one being stepped over, so if we
- let other threads run, we can actually miss any
- breakpoint, not just the one at PC. */
+ breakpoint if allowed to run. */
resume_ptid = inferior_ptid;
}
@@ -2031,6 +2098,8 @@ clear_proceed_status (void)
stop_after_trap = 0;
+ clear_step_over_info ();
+
observer_notify_about_to_proceed ();
if (stop_registers)
@@ -2217,22 +2286,23 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
tp = inferior_thread ();
if (force_step)
+ tp->control.trap_expected = 1;
+
+ /* If we need to step over a breakpoint, and we're not using
+ displaced stepping to do so, insert all breakpoints (watchpoints,
+ etc.) but the one we're stepping over, step one instruction, and
+ then re-insert the breakpoint when that step is finished. */
+ if (tp->control.trap_expected && !use_displaced_stepping (gdbarch))
{
- tp->control.trap_expected = 1;
- /* If displaced stepping is enabled, we can step over the
- breakpoint without hitting it, so leave all breakpoints
- inserted. Otherwise we need to disable all breakpoints, step
- one instruction, and then re-add them when that step is
- finished. */
- if (!use_displaced_stepping (gdbarch))
- remove_breakpoints ();
+ struct regcache *regcache = get_current_regcache ();
+
+ set_step_over_info (get_regcache_aspace (regcache),
+ regcache_read_pc (regcache));
}
+ else
+ clear_step_over_info ();
- /* We can insert breakpoints if we're not trying to step over one,
- or if we are stepping over one but we're using displaced stepping
- to do so. */
- if (! tp->control.trap_expected || use_displaced_stepping (gdbarch))
- insert_breakpoints ();
+ insert_breakpoints ();
if (!non_stop)
{
@@ -3992,9 +4062,6 @@ handle_signal_stop (struct execution_control_state *ecs)
if (thread_hop_needed)
{
- struct regcache *thread_regcache;
- int remove_status = 0;
-
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: thread_hop_needed\n");
@@ -4013,35 +4080,17 @@ handle_signal_stop (struct execution_control_state *ecs)
singlestep_breakpoints_inserted_p = 0;
}
- /* If the arch can displace step, don't remove the
- breakpoints. */
- thread_regcache = get_thread_regcache (ecs->ptid);
- if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
- remove_status = remove_breakpoints ();
-
- /* Did we fail to remove breakpoints? If so, try
- to set the PC past the bp. (There's at least
- one situation in which we can fail to remove
- the bp's: On HP-UX's that use ttrace, we can't
- change the address space of a vforking child
- process until the child exits (well, okay, not
- then either :-) or execs. */
- if (remove_status != 0)
- error (_("Cannot step over breakpoint hit in wrong thread"));
- else
- { /* Single step */
- if (!non_stop)
- {
- /* Only need to require the next event from this
- thread in all-stop mode. */
- waiton_ptid = ecs->ptid;
- infwait_state = infwait_thread_hop_state;
- }
-
- ecs->event_thread->stepping_over_breakpoint = 1;
- keep_going (ecs);
- return;
+ if (!non_stop)
+ {
+ /* Only need to require the next event from this thread
+ in all-stop mode. */
+ waiton_ptid = ecs->ptid;
+ infwait_state = infwait_thread_hop_state;
}
+
+ ecs->event_thread->stepping_over_breakpoint = 1;
+ keep_going (ecs);
+ return;
}
}
@@ -5695,6 +5744,8 @@ stop_stepping (struct execution_control_state *ecs)
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: stop_stepping\n");
+ clear_step_over_info ();
+
/* Let callers know we don't want to wait for the inferior anymore. */
ecs->wait_some_more = 0;
}
@@ -5727,6 +5778,9 @@ keep_going (struct execution_control_state *ecs)
}
else
{
+ volatile struct gdb_exception e;
+ struct regcache *regcache = get_current_regcache ();
+
/* Either the trap was not expected, but we are continuing
anyway (if we got a signal, the user asked it be passed to
the child)
@@ -5740,33 +5794,30 @@ keep_going (struct execution_control_state *ecs)
already inserted breakpoints. Therefore, we don't
care if breakpoints were already inserted, or not. */
- if (ecs->event_thread->stepping_over_breakpoint)
+ /* If we need to step over a breakpoint, and we're not using
+ displaced stepping to do so, insert all breakpoints
+ (watchpoints, etc.) but the one we're stepping over, step one
+ instruction, and then re-insert the breakpoint when that step
+ is finished. */
+ if (ecs->event_thread->stepping_over_breakpoint
+ && !use_displaced_stepping (get_regcache_arch (regcache)))
{
- struct regcache *thread_regcache = get_thread_regcache (ecs->ptid);
-
- if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
- {
- /* Since we can't do a displaced step, we have to remove
- the breakpoint while we step it. To keep things
- simple, we remove them all. */
- remove_breakpoints ();
- }
+ set_step_over_info (get_regcache_aspace (regcache),
+ regcache_read_pc (regcache));
}
else
- {
- volatile struct gdb_exception e;
+ clear_step_over_info ();
- /* Stop stepping if inserting breakpoints fails. */
- TRY_CATCH (e, RETURN_MASK_ERROR)
- {
- insert_breakpoints ();
- }
- if (e.reason < 0)
- {
- exception_print (gdb_stderr, e);
- stop_stepping (ecs);
- return;
- }
+ /* Stop stepping if inserting breakpoints fails. */
+ TRY_CATCH (e, RETURN_MASK_ERROR)
+ {
+ insert_breakpoints ();
+ }
+ if (e.reason < 0)
+ {
+ exception_print (gdb_stderr, e);
+ stop_stepping (ecs);
+ return;
}
ecs->event_thread->control.trap_expected
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index 80d75cb..1123824 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -550,21 +550,16 @@ proc test_complex_watchpoint {} {
proc test_watchpoint_and_breakpoint {} {
global gdb_prompt
- # This is a test for PR gdb/38, which involves setting a
+ # This is a test for PR breakpoints/7143, which involves setting a
# watchpoint right after you've reached a breakpoint.
if [runto func3] then {
gdb_breakpoint [gdb_get_line_number "second x assignment"]
gdb_continue_to_breakpoint "second x assignment"
gdb_test "watch x" ".*atchpoint \[0-9\]+: x"
- gdb_test_multiple "next" "next after watch x" {
- -re ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*$gdb_prompt $" {
- pass "next after watch x"
- }
- -re "\[0-9\]+\[\t \]+y = 1;\r\n$gdb_prompt $" {
- kfail "gdb/38" "next after watch x"
- }
- }
+ gdb_test "next" \
+ ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*" \
+ "next after watch x"
gdb_test_no_output "delete \$bpnum" "delete watch x"
}
diff --git a/gdb/testsuite/gdb.cp/annota2.exp b/gdb/testsuite/gdb.cp/annota2.exp
index 00a6067..6fbf4b5 100644
--- a/gdb/testsuite/gdb.cp/annota2.exp
+++ b/gdb/testsuite/gdb.cp/annota2.exp
@@ -165,9 +165,6 @@ gdb_test_multiple "next" "watch triggered on a.x" {
-re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n${frames_invalid}${breakpoints_invalid}\r\n\032\032watchpoint 3\r\n.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*$srcfile\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$decimal\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" {
pass "watch triggered on a.x"
}
- -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n${frames_invalid}\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
- kfail "gdb/38" "watch triggered on a.x"
- }
}
diff --git a/gdb/testsuite/gdb.cp/annota3.exp b/gdb/testsuite/gdb.cp/annota3.exp
index 957d371..bbf9a1e 100644
--- a/gdb/testsuite/gdb.cp/annota3.exp
+++ b/gdb/testsuite/gdb.cp/annota3.exp
@@ -169,9 +169,6 @@ gdb_test_multiple "next" "watch triggered on a.x" {
-re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032watchpoint 3\r\n.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n(\032\032frame-begin 0 0x\[0-9a-z\]+\r\n|)main \\(\\) at .*$srcfile:$decimal\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" {
pass "watch triggered on a.x"
}
- -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
- kfail "gdb/38" "watch triggered on a.x"
- }
}
#
--
1.7.11.7
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 1/5] Fix missing breakpoint/watchpoint hits, eliminate deferred_step_ptid.
2014-03-07 1:10 [PATCH v3 0/5] Fix lost events, and handle multiple step-overs Pedro Alves
2014-03-07 1:10 ` [PATCH v3 4/5] Handle " Pedro Alves
@ 2014-03-07 1:10 ` Pedro Alves
2014-03-07 1:10 ` [PATCH v3 5/5] Make signal-while-stepping-over-bp-other-thread.exp run against remote targets too Pedro Alves
` (3 subsequent siblings)
5 siblings, 0 replies; 25+ messages in thread
From: Pedro Alves @ 2014-03-07 1:10 UTC (permalink / raw)
To: gdb-patches
Consider the case of the user doing "step" in thread 2, while thread 1
had previously stopped for a breakpoint. In order to make progress,
GDB makes thread 1 step over its breakpoint first (with all other
threads stopped), and once that is over, thread 2 then starts stepping
(with thread 1 and all others running free, by default). If GDB
didn't do that, thread 1 would just trip on the same breakpoint
immediately again. This is what the prepare_to_proceed /
deferred_step_ptid code is all about.
However, deferred_step_ptid code resumes the target with:
resume (1, GDB_SIGNAL_0);
prepare_to_wait (ecs);
return;
Recall we were just stepping over a breakpoint when we get here. That
means that _nothing_ had installed breakpoints yet! If there's
another breakpoint just after the breakpoint that was just stepped,
we'll miss it. The fix for that would be to use keep_going instead.
However, there are more problems. What if the instruction that was
just single-stepped triggers a watchpoint? Currently, GDB just
happily resumes the thread, losing that too...
Missed watchpoints will need yet further fixes, but we should keep
those in mind.
So the fix must be to let the trap fall through the regular bpstat
handling, and only if no breakpoint, watchpoint, etc. claims the trap,
shall we switch back to the stepped thread.
Now, nowadays, we have code at the tail end of trap handling that does
exactly that -- switch back to the stepped thread
(switch_back_to_the_stepped_thread).
So the deferred_step_ptid code is just standing in the way, and can
simply be eliminated, fixing bugs in the process. Sweet.
The comment about spurious "Switching to ..." made me pause, but is
actually stale nowadays. That isn't needed anymore.
previous_inferior_ptid used to be re-set at each (internal) event, but
now it's only touched in proceed and normal stop.
The two tests added by this patch fail without the fix.
Tested on x86_64 Fedora 17 (also against my software single-stepping
on x86 branch).
gdb/
2014-03-05 Pedro Alves <palves@redhat.com>
* infrun.c (previous_inferior_ptid): Adjust comment.
(deferred_step_ptid): Delete.
(infrun_thread_ptid_changed, prepare_to_proceed)
(init_wait_for_inferior): Adjust.
(handle_signal_stop): Delete deferred_step_ptid handling.
gdb/testsuite/
2014-03-05 Pedro Alves <palves@redhat.com>
* gdb.threads/step-over-lands-on-breakpoint.c: New file.
* gdb.threads/step-over-lands-on-breakpoint.exp: New file.
--
v2
- Tidy tests per Yao's review.
- Moved step-over-trips-on-watchpoint.{c|exp} to another patch.
---
gdb/infrun.c | 59 +-------------------
.../gdb.threads/step-over-lands-on-breakpoint.c | 65 ++++++++++++++++++++++
.../gdb.threads/step-over-lands-on-breakpoint.exp | 62 +++++++++++++++++++++
3 files changed, 130 insertions(+), 56 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.c
create mode 100644 gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.exp
diff --git a/gdb/infrun.c b/gdb/infrun.c
index b7c0969..bd55505 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -127,9 +127,9 @@ show_step_stop_if_no_debug (struct ui_file *file, int from_tty,
int sync_execution = 0;
-/* wait_for_inferior and normal_stop use this to notify the user
- when the inferior stopped in a different thread than it had been
- running in. */
+/* proceed and normal_stop use this to notify the user when the
+ inferior stopped in a different thread than it had been running
+ in. */
static ptid_t previous_inferior_ptid;
@@ -977,14 +977,6 @@ static CORE_ADDR singlestep_pc;
static ptid_t saved_singlestep_ptid;
static int stepping_past_singlestep_breakpoint;
-/* If not equal to null_ptid, this means that after stepping over breakpoint
- is finished, we need to switch to deferred_step_ptid, and step it.
-
- The use case is when one thread has hit a breakpoint, and then the user
- has switched to another thread and issued 'step'. We need to step over
- breakpoint in the thread which hit the breakpoint, but then continue
- stepping the thread user has selected. */
-static ptid_t deferred_step_ptid;
\f
/* Displaced stepping. */
@@ -1583,9 +1575,6 @@ infrun_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid)
if (ptid_equal (singlestep_ptid, old_ptid))
singlestep_ptid = new_ptid;
- if (ptid_equal (deferred_step_ptid, old_ptid))
- deferred_step_ptid = new_ptid;
-
for (displaced = displaced_step_inferior_states;
displaced;
displaced = displaced->next)
@@ -2103,10 +2092,6 @@ prepare_to_proceed (int step)
if (breakpoint_here_p (get_regcache_aspace (regcache),
regcache_read_pc (regcache)))
{
- /* If stepping, remember current thread to switch back to. */
- if (step)
- deferred_step_ptid = inferior_ptid;
-
/* Switch back to WAIT_PID thread. */
switch_to_thread (wait_ptid);
@@ -2380,7 +2365,6 @@ init_wait_for_inferior (void)
clear_proceed_status ();
stepping_past_singlestep_breakpoint = 0;
- deferred_step_ptid = null_ptid;
target_last_wait_ptid = minus_one_ptid;
@@ -3918,43 +3902,6 @@ handle_signal_stop (struct execution_control_state *ecs)
}
}
- if (!ptid_equal (deferred_step_ptid, null_ptid))
- {
- /* In non-stop mode, there's never a deferred_step_ptid set. */
- gdb_assert (!non_stop);
-
- /* If we stopped for some other reason than single-stepping, ignore
- the fact that we were supposed to switch back. */
- if (ecs->event_thread->suspend.stop_signal == GDB_SIGNAL_TRAP)
- {
- if (debug_infrun)
- fprintf_unfiltered (gdb_stdlog,
- "infrun: handling deferred step\n");
-
- /* Pull the single step breakpoints out of the target. */
- if (singlestep_breakpoints_inserted_p)
- {
- if (!ptid_equal (ecs->ptid, inferior_ptid))
- context_switch (ecs->ptid);
- remove_single_step_breakpoints ();
- singlestep_breakpoints_inserted_p = 0;
- }
-
- ecs->event_thread->control.trap_expected = 0;
-
- context_switch (deferred_step_ptid);
- deferred_step_ptid = null_ptid;
- /* Suppress spurious "Switching to ..." message. */
- previous_inferior_ptid = inferior_ptid;
-
- resume (1, GDB_SIGNAL_0);
- prepare_to_wait (ecs);
- return;
- }
-
- deferred_step_ptid = null_ptid;
- }
-
/* See if a thread hit a thread-specific breakpoint that was meant for
another thread. If so, then step that thread past the breakpoint,
and continue it. */
diff --git a/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.c b/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.c
new file mode 100644
index 0000000..882ae82
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.c
@@ -0,0 +1,65 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 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 <pthread.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+pthread_barrier_t barrier;
+pthread_t child_thread;
+
+volatile unsigned int counter = 1;
+
+void *
+child_function (void *arg)
+{
+ pthread_barrier_wait (&barrier);
+
+ while (counter > 0)
+ {
+ counter++;
+
+ asm (" nop"); /* set breakpoint child here */
+ asm (" nop"); /* set breakpoint after step-over here */
+ usleep (1);
+ }
+
+ pthread_exit (NULL);
+}
+
+static int
+wait_threads (void)
+{
+ return 1; /* in wait_threads */
+}
+
+int
+main ()
+{
+ int res;
+ long i;
+
+ pthread_barrier_init (&barrier, NULL, 2);
+
+ res = pthread_create (&child_thread, NULL, child_function, NULL);
+ pthread_barrier_wait (&barrier);
+ wait_threads (); /* set wait-thread breakpoint here */
+
+ pthread_join (child_thread, NULL);
+
+ exit (EXIT_SUCCESS);
+}
diff --git a/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.exp b/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.exp
new file mode 100644
index 0000000..c17b346
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.exp
@@ -0,0 +1,62 @@
+# Copyright (C) 2014 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/>.
+
+# Test that when a step-over lands on a breakpoint, that breakpoint
+# hit is reported.
+
+standard_testfile
+set executable ${testfile}
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+ executable [list debug "incdir=${objdir}"]] != "" } {
+ return -1
+}
+
+# Cover both stepping and non-stepping execution commands.
+foreach command {"step" "next" "continue" } {
+ with_test_prefix $command {
+ clean_restart $executable
+
+ if ![runto_main] {
+ continue
+ }
+
+ gdb_breakpoint [gdb_get_line_number "set wait-thread breakpoint here"]
+ gdb_continue_to_breakpoint "run to wait-thread breakpoint"
+ gdb_test "info threads" "2 .*\\\* 1.*" "info threads shows all threads"
+
+ gdb_test_no_output "set scheduler-locking on"
+
+ delete_breakpoints
+
+ gdb_breakpoint [gdb_get_line_number "set breakpoint child here"]
+ gdb_test "thread 2" "Switching to .*"
+ gdb_continue_to_breakpoint "run to breakpoint in thread 2"
+ gdb_test "p counter = 0" " = 0" "unbreak loop in thread 2"
+
+ # Set a breakpoint exactly where the step-over will land.
+ gdb_breakpoint [gdb_get_line_number "breakpoint after step-over here"]
+
+ # Switch back to thread 1 and disable scheduler locking.
+ gdb_test "thread 1" "Switching to .*"
+ gdb_test_no_output "set scheduler-locking off"
+
+ # Thread 2 is still stopped at a breakpoint that needs to be
+ # stepped over before proceeding thread 1. However, right
+ # where the step-over lands there's another breakpoint
+ # installed, which should trap and be reported to the user.
+ gdb_test "$command" "step-over here.*"
+ }
+}
--
1.7.11.7
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 5/5] Make signal-while-stepping-over-bp-other-thread.exp run against remote targets too.
2014-03-07 1:10 [PATCH v3 0/5] Fix lost events, and handle multiple step-overs Pedro Alves
2014-03-07 1:10 ` [PATCH v3 4/5] Handle " Pedro Alves
2014-03-07 1:10 ` [PATCH v3 1/5] Fix missing breakpoint/watchpoint hits, eliminate deferred_step_ptid Pedro Alves
@ 2014-03-07 1:10 ` Pedro Alves
2014-03-07 1:10 ` [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set Pedro Alves
` (2 subsequent siblings)
5 siblings, 0 replies; 25+ messages in thread
From: Pedro Alves @ 2014-03-07 1:10 UTC (permalink / raw)
To: gdb-patches
Use pthread_kill instead of the host's "kill". The reason the test
wasn't written that way to begin with, is that done this way, before
the previous fixes to make GDB step-over all other threads before the
stepping thread, the test would fail...
Tested on x86_64 Fedora 17, native and gdbserver.
2014-02-25 Pedro Alves <palves@redhat.com>
* gdb.threads/signal-while-stepping-over-bp-other-thread.c (main):
Use pthread_kill to signal thread 2.
* gdb.threads/signal-while-stepping-over-bp-other-thread.exp:
Adjust to make the test send itself a signal rather than using the
host's "kill" command.
---
.../signal-while-stepping-over-bp-other-thread.c | 2 ++
.../signal-while-stepping-over-bp-other-thread.exp | 14 ++------------
2 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.c b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.c
index a4634f2..8839a6f 100644
--- a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.c
+++ b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.c
@@ -138,6 +138,8 @@ main ()
pthread_barrier_wait (&barrier);
callme (); /* set wait-thread-3 breakpoint here */
+ pthread_kill (child_thread_2, SIGUSR1);
+
pthread_join (child_thread_2, NULL);
pthread_join (child_thread_3, NULL);
diff --git a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
index c46dad2..2a30604 100644
--- a/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
+++ b/gdb/testsuite/gdb.threads/signal-while-stepping-over-bp-other-thread.exp
@@ -26,11 +26,6 @@ if [target_info exists gdb,nosignals] {
return -1
}
-# Test uses host "kill".
-if { [is_remote target] } {
- return -1
-}
-
if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
executable [list debug "incdir=${objdir}"]] != "" } {
return -1
@@ -67,11 +62,6 @@ gdb_breakpoint [gdb_get_line_number "set wait-thread-3 breakpoint here"]
gdb_continue_to_breakpoint "run to breakpoint"
gdb_test "info threads" "" "info threads with thread 3"
-set testpid [get_value "pid" "get pid of inferior"]
-if { $testpid == -1 } {
- return -1
-}
-
gdb_test "set scheduler-locking on"
gdb_breakpoint [gdb_get_line_number "set breakpoint child_two here"]
@@ -94,8 +84,8 @@ gdb_test "p *myp = 0" " = 0" "force loop break in thread 2"
# core.
gdb_test "handle SIGUSR1 print nostop pass" "" ""
-# Queue a signal in thread 2.
-remote_exec host "kill -SIGUSR1 ${testpid}"
+gdb_test "thread 1" "" "switch to thread 1 to queue signal in thread 2"
+gdb_test "next 2" "pthread_join .*" "queue signal in thread 2"
gdb_test "thread 3" "" "switch to thread 3 for stepping"
set my_number [get_value "my_number" "get my_number"]
--
1.7.11.7
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 0/5] Fix lost events, and handle multiple step-overs.
@ 2014-03-07 1:10 Pedro Alves
2014-03-07 1:10 ` [PATCH v3 4/5] Handle " Pedro Alves
` (5 more replies)
0 siblings, 6 replies; 25+ messages in thread
From: Pedro Alves @ 2014-03-07 1:10 UTC (permalink / raw)
To: gdb-patches
Here's v3. This version mainly addresses Doug's review of patch 2 of
v2. As the rest of the series would no longer apply and one patch
needed a minor corresponding tweak, I'm resending it all.
v2 was here:
https://sourceware.org/ml/gdb-patches/2014-03/msg00128.html
v1 was here:
https://sourceware.org/ml/gdb-patches/2014-02/msg00761.html.
What remains is related to GDB losing events - breakpoints and
watchpoints - in some situations, about teaching GDB that it might
need to step over breakpoints in multiple threads.
One nice side effect of patch #2 is that GDB no longer removes/inserts
_all_ breakpoints when stepping over one. GDB after that only removes
the breakpoint being stepped over. This eliminates a lot of z0/Z0 RSP
traffic if you have a lot of breakpoints, and have a conditional or
thread-specific breakpoint constantly triggering and not causing a
user-visible stop.
This changes heavily core run control code, which affects software
single-step targets too. For more comfortable development and
testing, I actually hacked on this against my software single-step on
x86 branch first.
Tested on x86_64 Fedora 17, against pristine mainline, and against a
series that implements software single-step on x86-64.
For convenience, you can find this series at:
git@github.com:palves/gdb.git fix_a_bunch_of_run_control_bugs_v3
and with software single-step on x86 on top:
git@github.com:palves/gdb.git fix_a_bunch_of_run_control_bugs_v3_sss
In absence of barring comments, I plan to push this in soon.
Changes in v3:
- Polish patch for PR breakpoints/7143, in response to Doug's review.
Changes in v2:
- Two patches of v1 applied.
- Order of some of the remaining patches changed.
- Bugs that exposed were fixed.
- One test moved to a different patch and got extended to cover
another scenario gdb was mishandling and this series fixes.
- Yao's review comments to v1's tests (to date) are addressed.
- Software single-step breakpoints are not longer converted to real
breakpoints.
Pedro Alves (5):
Fix missing breakpoint/watchpoint hits, eliminate deferred_step_ptid.
PR breakpoints/7143 - Watchpoint does not trigger when first set
Fix for even more missed events; eliminate thread-hop code.
Handle multiple step-overs.
Make signal-while-stepping-over-bp-other-thread.exp run against
remote targets too.
gdb/breakpoint.c | 25 +-
gdb/breakpoint.h | 13 +
gdb/inferior.h | 6 +
gdb/infrun.c | 765 ++++++++++-----------
gdb/testsuite/gdb.base/watchpoint.exp | 13 +-
gdb/testsuite/gdb.cp/annota2.exp | 3 -
gdb/testsuite/gdb.cp/annota3.exp | 3 -
gdb/testsuite/gdb.threads/multiple-step-overs.c | 105 +++
gdb/testsuite/gdb.threads/multiple-step-overs.exp | 80 +++
.../signal-while-stepping-over-bp-other-thread.c | 2 +
.../signal-while-stepping-over-bp-other-thread.exp | 16 +-
.../gdb.threads/step-over-lands-on-breakpoint.c | 65 ++
.../gdb.threads/step-over-lands-on-breakpoint.exp | 62 ++
.../gdb.threads/step-over-trips-on-watchpoint.c | 67 ++
.../gdb.threads/step-over-trips-on-watchpoint.exp | 90 +++
15 files changed, 877 insertions(+), 438 deletions(-)
create mode 100644 gdb/testsuite/gdb.threads/multiple-step-overs.c
create mode 100644 gdb/testsuite/gdb.threads/multiple-step-overs.exp
create mode 100644 gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.c
create mode 100644 gdb/testsuite/gdb.threads/step-over-lands-on-breakpoint.exp
create mode 100644 gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.c
create mode 100644 gdb/testsuite/gdb.threads/step-over-trips-on-watchpoint.exp
--
1.7.11.7
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set
2014-03-07 1:10 ` [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set Pedro Alves
@ 2014-03-16 3:41 ` Doug Evans
2014-03-17 17:11 ` Pedro Alves
0 siblings, 1 reply; 25+ messages in thread
From: Doug Evans @ 2014-03-16 3:41 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On Thu, Mar 6, 2014 at 5:10 PM, Pedro Alves <palves@redhat.com> wrote:
> [...]
> gdb/
> 2014-03-06 Pedro Alves <palves@redhat.com>
>
> PR breakpoints/7143
> * breakpoint.c (should_be_inserted): Don't insert breakpoints that
> are being stepped over.
> (breakpoint_address_match): Make extern.
> * breakpoint.h (breakpoint_address_match): New declaration.
> * inferior.h (stepping_past_instruction_at): New declaration.
> * infrun.c (struct step_over_info): New type.
> (step_over_info): New global.
> (set_step_over_info, clear_step_over_info)
> (stepping_past_instruction_at): New functions.
> (handle_inferior_event): Clear the step-over info when
> trap_expected is cleared.
> (resume): Remove now stale comment.
> (clear_proceed_status): Clear step-over info.
> (proceed): Adjust step-over handling to set or clear the step-over
> info instead of removing all breakpoints.
> (handle_signal_stop): When setting up a thread-hop, don't remove
> breakpoints here.
> (stop_stepping): Clear step-over info.
> (keep_going): Adjust step-over handling to set or clear step-over
> info and then always inserting breakpoints, instead of removing
> all breakpoints when stepping over one.
>
> gdb/testsuite/
> 2014-03-06 Pedro Alves <palves@redhat.com>
>
> PR breakpoints/7143
> * gdb.base/watchpoint.exp: Mention bugzilla bug number instead of
> old gnats gdb/38. Remove kfail. Adjust to use gdb_test instead
> of gdb_test_multiple.
> * gdb.cp/annota2.exp: Remove kfail for gdb/38.
> * gdb.cp/annota3.exp: Remove kfail for gdb/38.
Hi. Ok by me with one nit.
I do like the functions that set/clear the step-over info.
> [...]
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index bd55505..b7c02d7 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -977,6 +977,76 @@ static CORE_ADDR singlestep_pc;
> static ptid_t saved_singlestep_ptid;
> static int stepping_past_singlestep_breakpoint;
>
> +/* Info about an instruction that is being stepped over. Invalid if
> + ASPACE is NULL. */
> +
> +struct step_over_info
> +{
> + /* The instruction's address space. */
> + struct address_space *aspace;
> +
> + /* The instruction's address. */
> + CORE_ADDR address;
> +};
> +
> +/* The step-over info of the location that is being stepped over.
> +
> + Note that with async/breakpoint always-inserted mode, a user might
> + set a new breakpoint/watchpoint/etc. exactly while a breakpoint is
> + being stepped over. As setting a new breakpoint inserts all
> + breakpoints, we need to make sure the breakpoint being stepped over
> + isn't inserted then. We do that by only clearing the step-over
> + info when the step-over is actually finished (or aborted).
> +
> + Presently GDB can only step over a breakpoint at any given time.
I realize this is nitpicky, but can you reword this as:
Presently GDB can only step over one breakpoint at any given time.
I realize it's saying the same thing, but when I first read it I had
to read it a couple of times to make sure I read it right. Maybe it's
just me.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set
2014-03-16 3:41 ` Doug Evans
@ 2014-03-17 17:11 ` Pedro Alves
2014-03-19 16:52 ` Doug Evans
2014-03-20 13:57 ` Pedro Alves
0 siblings, 2 replies; 25+ messages in thread
From: Pedro Alves @ 2014-03-17 17:11 UTC (permalink / raw)
To: Doug Evans; +Cc: gdb-patches
On 03/16/2014 03:41 AM, Doug Evans wrote:
> Hi. Ok by me with one nit.
Thanks. FAOD, are you planning on replying to the other patches
in the series?
>> + Presently GDB can only step over a breakpoint at any given time.
>
> I realize this is nitpicky, but can you reword this as:
Sure thing.
>
> Presently GDB can only step over one breakpoint at any given time.
>
> I realize it's saying the same thing, but when I first read it I had
> to read it a couple of times to make sure I read it right. Maybe it's
> just me.
I agree.
--
Pedro Alves
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set
2014-03-17 17:11 ` Pedro Alves
@ 2014-03-19 16:52 ` Doug Evans
2014-03-20 13:58 ` Pedro Alves
2014-03-20 13:57 ` Pedro Alves
1 sibling, 1 reply; 25+ messages in thread
From: Doug Evans @ 2014-03-19 16:52 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On Mon, Mar 17, 2014 at 10:11 AM, Pedro Alves <palves@redhat.com> wrote:
> On 03/16/2014 03:41 AM, Doug Evans wrote:
>
>> Hi. Ok by me with one nit.
>
> Thanks. FAOD, are you planning on replying to the other patches
> in the series?
I briefly looked at them, but nothing stood out worth spending time on.
Thanks.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set
2014-03-17 17:11 ` Pedro Alves
2014-03-19 16:52 ` Doug Evans
@ 2014-03-20 13:57 ` Pedro Alves
2014-06-17 19:18 ` Regression for watchpoint-fork.exp [Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set] Jan Kratochvil
1 sibling, 1 reply; 25+ messages in thread
From: Pedro Alves @ 2014-03-20 13:57 UTC (permalink / raw)
Cc: Doug Evans, gdb-patches
On 03/17/2014 05:11 PM, Pedro Alves wrote:
> On 03/16/2014 03:41 AM, Doug Evans wrote:
>
>> Hi. Ok by me with one nit.
>
> Thanks. FAOD, are you planning on replying to the other patches
> in the series?
>
>>> + Presently GDB can only step over a breakpoint at any given time.
>>
>> I realize this is nitpicky, but can you reword this as:
>
> Sure thing.
Here's what I pushed (nothing else changed).
Many thanks for the review.
-----
PR breakpoints/7143 - Watchpoint does not trigger when first
set
Say the program is stopped at a breakpoint, and the user sets a
watchpoint. When the program is next resumed, GDB will first step
over the breakpoint, as explained in the manual:
@value {GDBN} normally ignores breakpoints when it resumes
execution, until at least one instruction has been executed. If it
it did not do this, you would be unable to proceed past a breakpoint
without first disabling the breakpoint. This rule applies whether
or not the breakpoint already existed when your program stopped.
However, GDB currently also removes watchpoints, catchpoints, etc.,
and that means that the first instruction off the breakpoint does not
trigger the watchpoint, catchpoint, etc.
testsuite/gdb.base/watchpoint.exp has a kfail for this.
The PR proposes installing watchpoints only when stepping over a
breakpoint, but that misses catchpoints, etc.
A better fix would instead work from the opposite direction -- remove
only real breakpoints, leaving all other kinds of breakpoints
inserted.
But, going further, it's really a waste to constantly remove/insert
all breakpoints when stepping over a single breakpoint (generating a
pair of RSP z/Z packets for each breakpoint), so the fix goes a step
further and makes GDB remove _only_ the breakpoint being stepped over,
leaving all others installed. This then has the added benefit of
reducing breakpoint-related RSP traffic substancialy when there are
many breakpoints set.
gdb/
2014-03-20 Pedro Alves <palves@redhat.com>
PR breakpoints/7143
* breakpoint.c (should_be_inserted): Don't insert breakpoints that
are being stepped over.
(breakpoint_address_match): Make extern.
* breakpoint.h (breakpoint_address_match): New declaration.
* inferior.h (stepping_past_instruction_at): New declaration.
* infrun.c (struct step_over_info): New type.
(step_over_info): New global.
(set_step_over_info, clear_step_over_info)
(stepping_past_instruction_at): New functions.
(handle_inferior_event): Clear the step-over info when
trap_expected is cleared.
(resume): Remove now stale comment.
(clear_proceed_status): Clear step-over info.
(proceed): Adjust step-over handling to set or clear the step-over
info instead of removing all breakpoints.
(handle_signal_stop): When setting up a thread-hop, don't remove
breakpoints here.
(stop_stepping): Clear step-over info.
(keep_going): Adjust step-over handling to set or clear step-over
info and then always inserting breakpoints, instead of removing
all breakpoints when stepping over one.
gdb/testsuite/
2014-03-20 Pedro Alves <palves@redhat.com>
PR breakpoints/7143
* gdb.base/watchpoint.exp: Mention bugzilla bug number instead of
old gnats gdb/38. Remove kfail. Adjust to use gdb_test instead
of gdb_test_multiple.
* gdb.cp/annota2.exp: Remove kfail for gdb/38.
* gdb.cp/annota3.exp: Remove kfail for gdb/38.
---
gdb/ChangeLog | 25 +++++
gdb/breakpoint.c | 20 ++--
gdb/breakpoint.h | 10 ++
gdb/inferior.h | 6 +
gdb/infrun.c | 199 +++++++++++++++++++++-------------
gdb/testsuite/ChangeLog | 9 ++
gdb/testsuite/gdb.base/watchpoint.exp | 13 +--
gdb/testsuite/gdb.cp/annota2.exp | 3 -
gdb/testsuite/gdb.cp/annota3.exp | 3 -
9 files changed, 189 insertions(+), 99 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 18244d2..7f99a5a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,30 @@
2014-03-20 Pedro Alves <palves@redhat.com>
+ PR breakpoints/7143
+ * breakpoint.c (should_be_inserted): Don't insert breakpoints that
+ are being stepped over.
+ (breakpoint_address_match): Make extern.
+ * breakpoint.h (breakpoint_address_match): New declaration.
+ * inferior.h (stepping_past_instruction_at): New declaration.
+ * infrun.c (struct step_over_info): New type.
+ (step_over_info): New global.
+ (set_step_over_info, clear_step_over_info)
+ (stepping_past_instruction_at): New functions.
+ (handle_inferior_event): Clear the step-over info when
+ trap_expected is cleared.
+ (resume): Remove now stale comment.
+ (clear_proceed_status): Clear step-over info.
+ (proceed): Adjust step-over handling to set or clear the step-over
+ info instead of removing all breakpoints.
+ (handle_signal_stop): When setting up a thread-hop, don't remove
+ breakpoints here.
+ (stop_stepping): Clear step-over info.
+ (keep_going): Adjust step-over handling to set or clear step-over
+ info and then always inserting breakpoints, instead of removing
+ all breakpoints when stepping over one.
+
+2014-03-20 Pedro Alves <palves@redhat.com>
+
* infrun.c (previous_inferior_ptid): Adjust comment.
(deferred_step_ptid): Delete.
(infrun_thread_ptid_changed, prepare_to_proceed)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1551b99..03b21cb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -165,11 +165,6 @@ static void describe_other_breakpoints (struct gdbarch *,
struct program_space *, CORE_ADDR,
struct obj_section *, int);
-static int breakpoint_address_match (struct address_space *aspace1,
- CORE_ADDR addr1,
- struct address_space *aspace2,
- CORE_ADDR addr2);
-
static int watchpoint_locations_match (struct bp_location *loc1,
struct bp_location *loc2);
@@ -2034,6 +2029,14 @@ should_be_inserted (struct bp_location *bl)
if (bl->pspace->breakpoints_not_allowed)
return 0;
+ /* Don't insert a breakpoint if we're trying to step past its
+ location. */
+ if ((bl->loc_type == bp_loc_software_breakpoint
+ || bl->loc_type == bp_loc_hardware_breakpoint)
+ && stepping_past_instruction_at (bl->pspace->aspace,
+ bl->address))
+ return 0;
+
return 1;
}
@@ -6792,12 +6795,9 @@ watchpoint_locations_match (struct bp_location *loc1,
&& loc1->length == loc2->length);
}
-/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
- same breakpoint location. In most targets, this can only be true
- if ASPACE1 matches ASPACE2. On targets that have global
- breakpoints, the address space doesn't really matter. */
+/* See breakpoint.h. */
-static int
+int
breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
struct address_space *aspace2, CORE_ADDR addr2)
{
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index bf1f52a..b4abcb8 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1135,6 +1135,16 @@ extern int hardware_watchpoint_inserted_in_range (struct address_space *,
extern int breakpoint_thread_match (struct address_space *,
CORE_ADDR, ptid_t);
+/* Returns true if {ASPACE1,ADDR1} and {ASPACE2,ADDR2} represent the
+ same breakpoint location. In most targets, this can only be true
+ if ASPACE1 matches ASPACE2. On targets that have global
+ breakpoints, the address space doesn't really matter. */
+
+extern int breakpoint_address_match (struct address_space *aspace1,
+ CORE_ADDR addr1,
+ struct address_space *aspace2,
+ CORE_ADDR addr2);
+
extern void until_break_command (char *, int, int);
/* Initialize a struct bp_location. */
diff --git a/gdb/inferior.h b/gdb/inferior.h
index b15f692..64a78ce 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -222,6 +222,12 @@ void set_step_info (struct frame_info *frame, struct symtab_and_line sal);
extern void clear_exit_convenience_vars (void);
+/* Returns true if we're trying to step past the instruction at
+ ADDRESS in ASPACE. */
+
+extern int stepping_past_instruction_at (struct address_space *aspace,
+ CORE_ADDR address);
+
/* From infcmd.c */
extern void post_create_inferior (struct target_ops *, int);
diff --git a/gdb/infrun.c b/gdb/infrun.c
index f189a86..ed8ec30 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -977,6 +977,76 @@ static CORE_ADDR singlestep_pc;
static ptid_t saved_singlestep_ptid;
static int stepping_past_singlestep_breakpoint;
+/* Info about an instruction that is being stepped over. Invalid if
+ ASPACE is NULL. */
+
+struct step_over_info
+{
+ /* The instruction's address space. */
+ struct address_space *aspace;
+
+ /* The instruction's address. */
+ CORE_ADDR address;
+};
+
+/* The step-over info of the location that is being stepped over.
+
+ Note that with async/breakpoint always-inserted mode, a user might
+ set a new breakpoint/watchpoint/etc. exactly while a breakpoint is
+ being stepped over. As setting a new breakpoint inserts all
+ breakpoints, we need to make sure the breakpoint being stepped over
+ isn't inserted then. We do that by only clearing the step-over
+ info when the step-over is actually finished (or aborted).
+
+ Presently GDB can only step over one breakpoint at any given time.
+ Given threads that can't run code in the same address space as the
+ breakpoint's can't really miss the breakpoint, GDB could be taught
+ to step-over at most one breakpoint per address space (so this info
+ could move to the address space object if/when GDB is extended).
+ The set of breakpoints being stepped over will normally be much
+ smaller than the set of all breakpoints, so a flag in the
+ breakpoint location structure would be wasteful. A separate list
+ also saves complexity and run-time, as otherwise we'd have to go
+ through all breakpoint locations clearing their flag whenever we
+ start a new sequence. Similar considerations weigh against storing
+ this info in the thread object. Plus, not all step overs actually
+ have breakpoint locations -- e.g., stepping past a single-step
+ breakpoint, or stepping to complete a non-continuable
+ watchpoint. */
+static struct step_over_info step_over_info;
+
+/* Record the address of the breakpoint/instruction we're currently
+ stepping over. */
+
+static void
+set_step_over_info (struct address_space *aspace, CORE_ADDR address)
+{
+ step_over_info.aspace = aspace;
+ step_over_info.address = address;
+}
+
+/* Called when we're not longer stepping over a breakpoint / an
+ instruction, so all breakpoints are free to be (re)inserted. */
+
+static void
+clear_step_over_info (void)
+{
+ step_over_info.aspace = NULL;
+ step_over_info.address = 0;
+}
+
+/* See inferior.h. */
+
+int
+stepping_past_instruction_at (struct address_space *aspace,
+ CORE_ADDR address)
+{
+ return (step_over_info.aspace != NULL
+ && breakpoint_address_match (aspace, address,
+ step_over_info.aspace,
+ step_over_info.address));
+}
+
\f
/* Displaced stepping. */
@@ -1852,8 +1922,10 @@ a command like `return' or `jump' to continue execution."));
remove_single_step_breakpoints ();
singlestep_breakpoints_inserted_p = 0;
- insert_breakpoints ();
+ clear_step_over_info ();
tp->control.trap_expected = 0;
+
+ insert_breakpoints ();
}
if (should_resume)
@@ -1894,12 +1966,7 @@ a command like `return' or `jump' to continue execution."));
hit, by single-stepping the thread with the breakpoint
removed. In which case, we need to single-step only this
thread, and keep others stopped, as they can miss this
- breakpoint if allowed to run.
-
- The current code actually removes all breakpoints when
- doing this, not just the one being stepped over, so if we
- let other threads run, we can actually miss any
- breakpoint, not just the one at PC. */
+ breakpoint if allowed to run. */
resume_ptid = inferior_ptid;
}
@@ -2031,6 +2098,8 @@ clear_proceed_status (void)
stop_after_trap = 0;
+ clear_step_over_info ();
+
observer_notify_about_to_proceed ();
if (stop_registers)
@@ -2217,22 +2286,23 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step)
tp = inferior_thread ();
if (force_step)
+ tp->control.trap_expected = 1;
+
+ /* If we need to step over a breakpoint, and we're not using
+ displaced stepping to do so, insert all breakpoints (watchpoints,
+ etc.) but the one we're stepping over, step one instruction, and
+ then re-insert the breakpoint when that step is finished. */
+ if (tp->control.trap_expected && !use_displaced_stepping (gdbarch))
{
- tp->control.trap_expected = 1;
- /* If displaced stepping is enabled, we can step over the
- breakpoint without hitting it, so leave all breakpoints
- inserted. Otherwise we need to disable all breakpoints, step
- one instruction, and then re-add them when that step is
- finished. */
- if (!use_displaced_stepping (gdbarch))
- remove_breakpoints ();
+ struct regcache *regcache = get_current_regcache ();
+
+ set_step_over_info (get_regcache_aspace (regcache),
+ regcache_read_pc (regcache));
}
+ else
+ clear_step_over_info ();
- /* We can insert breakpoints if we're not trying to step over one,
- or if we are stepping over one but we're using displaced stepping
- to do so. */
- if (! tp->control.trap_expected || use_displaced_stepping (gdbarch))
- insert_breakpoints ();
+ insert_breakpoints ();
if (!non_stop)
{
@@ -3992,9 +4062,6 @@ handle_signal_stop (struct execution_control_state *ecs)
if (thread_hop_needed)
{
- struct regcache *thread_regcache;
- int remove_status = 0;
-
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: thread_hop_needed\n");
@@ -4013,35 +4080,17 @@ handle_signal_stop (struct execution_control_state *ecs)
singlestep_breakpoints_inserted_p = 0;
}
- /* If the arch can displace step, don't remove the
- breakpoints. */
- thread_regcache = get_thread_regcache (ecs->ptid);
- if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
- remove_status = remove_breakpoints ();
-
- /* Did we fail to remove breakpoints? If so, try
- to set the PC past the bp. (There's at least
- one situation in which we can fail to remove
- the bp's: On HP-UX's that use ttrace, we can't
- change the address space of a vforking child
- process until the child exits (well, okay, not
- then either :-) or execs. */
- if (remove_status != 0)
- error (_("Cannot step over breakpoint hit in wrong thread"));
- else
- { /* Single step */
- if (!non_stop)
- {
- /* Only need to require the next event from this
- thread in all-stop mode. */
- waiton_ptid = ecs->ptid;
- infwait_state = infwait_thread_hop_state;
- }
-
- ecs->event_thread->stepping_over_breakpoint = 1;
- keep_going (ecs);
- return;
+ if (!non_stop)
+ {
+ /* Only need to require the next event from this thread
+ in all-stop mode. */
+ waiton_ptid = ecs->ptid;
+ infwait_state = infwait_thread_hop_state;
}
+
+ ecs->event_thread->stepping_over_breakpoint = 1;
+ keep_going (ecs);
+ return;
}
}
@@ -5695,6 +5744,8 @@ stop_stepping (struct execution_control_state *ecs)
if (debug_infrun)
fprintf_unfiltered (gdb_stdlog, "infrun: stop_stepping\n");
+ clear_step_over_info ();
+
/* Let callers know we don't want to wait for the inferior anymore. */
ecs->wait_some_more = 0;
}
@@ -5727,6 +5778,9 @@ keep_going (struct execution_control_state *ecs)
}
else
{
+ volatile struct gdb_exception e;
+ struct regcache *regcache = get_current_regcache ();
+
/* Either the trap was not expected, but we are continuing
anyway (if we got a signal, the user asked it be passed to
the child)
@@ -5740,33 +5794,30 @@ keep_going (struct execution_control_state *ecs)
already inserted breakpoints. Therefore, we don't
care if breakpoints were already inserted, or not. */
- if (ecs->event_thread->stepping_over_breakpoint)
+ /* If we need to step over a breakpoint, and we're not using
+ displaced stepping to do so, insert all breakpoints
+ (watchpoints, etc.) but the one we're stepping over, step one
+ instruction, and then re-insert the breakpoint when that step
+ is finished. */
+ if (ecs->event_thread->stepping_over_breakpoint
+ && !use_displaced_stepping (get_regcache_arch (regcache)))
{
- struct regcache *thread_regcache = get_thread_regcache (ecs->ptid);
-
- if (!use_displaced_stepping (get_regcache_arch (thread_regcache)))
- {
- /* Since we can't do a displaced step, we have to remove
- the breakpoint while we step it. To keep things
- simple, we remove them all. */
- remove_breakpoints ();
- }
+ set_step_over_info (get_regcache_aspace (regcache),
+ regcache_read_pc (regcache));
}
else
- {
- volatile struct gdb_exception e;
+ clear_step_over_info ();
- /* Stop stepping if inserting breakpoints fails. */
- TRY_CATCH (e, RETURN_MASK_ERROR)
- {
- insert_breakpoints ();
- }
- if (e.reason < 0)
- {
- exception_print (gdb_stderr, e);
- stop_stepping (ecs);
- return;
- }
+ /* Stop stepping if inserting breakpoints fails. */
+ TRY_CATCH (e, RETURN_MASK_ERROR)
+ {
+ insert_breakpoints ();
+ }
+ if (e.reason < 0)
+ {
+ exception_print (gdb_stderr, e);
+ stop_stepping (ecs);
+ return;
}
ecs->event_thread->control.trap_expected
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 1986747..ee71b74 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,14 @@
2014-03-20 Pedro Alves <palves@redhat.com>
+ PR breakpoints/7143
+ * gdb.base/watchpoint.exp: Mention bugzilla bug number instead of
+ old gnats gdb/38. Remove kfail. Adjust to use gdb_test instead
+ of gdb_test_multiple.
+ * gdb.cp/annota2.exp: Remove kfail for gdb/38.
+ * gdb.cp/annota3.exp: Remove kfail for gdb/38.
+
+2014-03-20 Pedro Alves <palves@redhat.com>
+
* gdb.threads/step-over-lands-on-breakpoint.c: New file.
* gdb.threads/step-over-lands-on-breakpoint.exp: New file.
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index 80d75cb..1123824 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -550,21 +550,16 @@ proc test_complex_watchpoint {} {
proc test_watchpoint_and_breakpoint {} {
global gdb_prompt
- # This is a test for PR gdb/38, which involves setting a
+ # This is a test for PR breakpoints/7143, which involves setting a
# watchpoint right after you've reached a breakpoint.
if [runto func3] then {
gdb_breakpoint [gdb_get_line_number "second x assignment"]
gdb_continue_to_breakpoint "second x assignment"
gdb_test "watch x" ".*atchpoint \[0-9\]+: x"
- gdb_test_multiple "next" "next after watch x" {
- -re ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*$gdb_prompt $" {
- pass "next after watch x"
- }
- -re "\[0-9\]+\[\t \]+y = 1;\r\n$gdb_prompt $" {
- kfail "gdb/38" "next after watch x"
- }
- }
+ gdb_test "next" \
+ ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*" \
+ "next after watch x"
gdb_test_no_output "delete \$bpnum" "delete watch x"
}
diff --git a/gdb/testsuite/gdb.cp/annota2.exp b/gdb/testsuite/gdb.cp/annota2.exp
index 00a6067..6fbf4b5 100644
--- a/gdb/testsuite/gdb.cp/annota2.exp
+++ b/gdb/testsuite/gdb.cp/annota2.exp
@@ -165,9 +165,6 @@ gdb_test_multiple "next" "watch triggered on a.x" {
-re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n${frames_invalid}${breakpoints_invalid}\r\n\032\032watchpoint 3\r\n.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n\032\032frame-begin 0 $hex\r\n\r\n\032\032frame-function-name\r\nmain\r\n\032\032frame-args\r\n \\(\\)\r\n\032\032frame-source-begin\r\n at \r\n\032\032frame-source-file\r\n.*$srcfile\r\n\032\032frame-source-file-end\r\n:\r\n\032\032frame-source-line\r\n$decimal\r\n\032\032frame-source-end\r\n\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" {
pass "watch triggered on a.x"
}
- -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n${frames_invalid}\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032frame-end\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
- kfail "gdb/38" "watch triggered on a.x"
- }
}
diff --git a/gdb/testsuite/gdb.cp/annota3.exp b/gdb/testsuite/gdb.cp/annota3.exp
index 957d371..bbf9a1e 100644
--- a/gdb/testsuite/gdb.cp/annota3.exp
+++ b/gdb/testsuite/gdb.cp/annota3.exp
@@ -169,9 +169,6 @@ gdb_test_multiple "next" "watch triggered on a.x" {
-re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032watchpoint 3\r\n.*atchpoint 3: a.x\r\n\r\nOld value = 0\r\nNew value = 1\r\n\r\n(\032\032frame-begin 0 0x\[0-9a-z\]+\r\n|)main \\(\\) at .*$srcfile:$decimal\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032stopped\r\n.*$gdb_prompt$" {
pass "watch triggered on a.x"
}
- -re "\r\n\032\032post-prompt\r\n\r\n\032\032starting\r\n\r\n\032\032source .*$srcfile.*beg:$hex\r\n\r\n\032\032stopped\r\n$gdb_prompt$" {
- kfail "gdb/38" "watch triggered on a.x"
- }
}
#
--
1.7.11.7
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set
2014-03-19 16:52 ` Doug Evans
@ 2014-03-20 13:58 ` Pedro Alves
0 siblings, 0 replies; 25+ messages in thread
From: Pedro Alves @ 2014-03-20 13:58 UTC (permalink / raw)
To: Doug Evans; +Cc: gdb-patches
On 03/19/2014 04:52 PM, Doug Evans wrote:
> On Mon, Mar 17, 2014 at 10:11 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 03/16/2014 03:41 AM, Doug Evans wrote:
>>
>>> Hi. Ok by me with one nit.
>>
>> Thanks. FAOD, are you planning on replying to the other patches
>> in the series?
>
> I briefly looked at them, but nothing stood out worth spending time on.
> Thanks.
Thank you. The series is now all pushed.
--
Pedro Alves
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PUSHED] Re: [PATCH v3 0/5] Fix lost events, and handle multiple step-overs.
2014-03-07 1:10 [PATCH v3 0/5] Fix lost events, and handle multiple step-overs Pedro Alves
` (4 preceding siblings ...)
2014-03-07 1:10 ` [PATCH v3 3/5] Fix for even more missed events; eliminate thread-hop code Pedro Alves
@ 2014-03-20 13:59 ` Pedro Alves
5 siblings, 0 replies; 25+ messages in thread
From: Pedro Alves @ 2014-03-20 13:59 UTC (permalink / raw)
To: gdb-patches
FYI, this is now all in.
--
Pedro Alves
^ permalink raw reply [flat|nested] 25+ messages in thread
* Regression for watchpoint-fork.exp [Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set]
2014-03-20 13:57 ` Pedro Alves
@ 2014-06-17 19:18 ` Jan Kratochvil
2014-06-18 10:43 ` Pedro Alves
2014-06-19 13:43 ` Jan Kratochvil
0 siblings, 2 replies; 25+ messages in thread
From: Jan Kratochvil @ 2014-06-17 19:18 UTC (permalink / raw)
To: Pedro Alves; +Cc: Doug Evans, gdb-patches
On Thu, 20 Mar 2014 14:57:36 +0100, Pedro Alves wrote:
> Here's what I pushed (nothing else changed).
31e77af205cf6564c2bf4c18400b4ca16bdf92cd is the first bad commit
commit 31e77af205cf6564c2bf4c18400b4ca16bdf92cd
Author: Pedro Alves <palves@redhat.com>
Date: Thu Mar 20 13:26:32 2014 +0000
PR breakpoints/7143 - Watchpoint does not trigger when first set
PASS kernel-3.14.6-200.fc20.x86_64
FAIL kernel-3.13.10-200.dentrybuflen.fc20.x86_64
continue^M
Continuing.^M
main () at ./gdb.threads/watchpoint-fork-st.c:50^M
50 forkoff (1);^M
Couldn't write debug register: Invalid argument.^M
(gdb) FAIL: gdb.threads/watchpoint-fork.exp: parent: singlethreaded: breakpoint after the first fork
Being tested for default linux-nat.
That 'dentrybuflen' patch should not matter. I did not check which Linux
kernels are / are not affected by the regression but it would be probably
better to make the GDB behavior more Linux kernel compatible.
Jan
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Regression for watchpoint-fork.exp [Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set]
2014-06-17 19:18 ` Regression for watchpoint-fork.exp [Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set] Jan Kratochvil
@ 2014-06-18 10:43 ` Pedro Alves
2014-06-19 13:43 ` Jan Kratochvil
1 sibling, 0 replies; 25+ messages in thread
From: Pedro Alves @ 2014-06-18 10:43 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
On 06/17/2014 08:18 PM, Jan Kratochvil wrote:
> On Thu, 20 Mar 2014 14:57:36 +0100, Pedro Alves wrote:
>> Here's what I pushed (nothing else changed).
>
> 31e77af205cf6564c2bf4c18400b4ca16bdf92cd is the first bad commit
> commit 31e77af205cf6564c2bf4c18400b4ca16bdf92cd
> Author: Pedro Alves <palves@redhat.com>
> Date: Thu Mar 20 13:26:32 2014 +0000
> PR breakpoints/7143 - Watchpoint does not trigger when first set
>
> PASS kernel-3.14.6-200.fc20.x86_64
> FAIL kernel-3.13.10-200.dentrybuflen.fc20.x86_64
>
> continue^M
> Continuing.^M
> main () at ./gdb.threads/watchpoint-fork-st.c:50^M
> 50 forkoff (1);^M
> Couldn't write debug register: Invalid argument.^M
Odd.
> (gdb) FAIL: gdb.threads/watchpoint-fork.exp: parent: singlethreaded: breakpoint after the first fork
>
> Being tested for default linux-nat.
>
> That 'dentrybuflen' patch should not matter. I did not check which Linux
> kernels are / are not affected by the regression but it would be probably
> better to make the GDB behavior more Linux kernel compatible.
Yes, though off hand I'm clueless on what's going on.
--
Pedro Alves
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Regression for watchpoint-fork.exp [Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set]
2014-06-17 19:18 ` Regression for watchpoint-fork.exp [Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set] Jan Kratochvil
2014-06-18 10:43 ` Pedro Alves
@ 2014-06-19 13:43 ` Jan Kratochvil
2014-06-19 15:02 ` Pedro Alves
1 sibling, 1 reply; 25+ messages in thread
From: Jan Kratochvil @ 2014-06-19 13:43 UTC (permalink / raw)
To: Pedro Alves; +Cc: Doug Evans, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 2831 bytes --]
On Tue, 17 Jun 2014 21:18:50 +0200, Jan Kratochvil wrote:
> On Thu, 20 Mar 2014 14:57:36 +0100, Pedro Alves wrote:
> > Here's what I pushed (nothing else changed).
>
> 31e77af205cf6564c2bf4c18400b4ca16bdf92cd is the first bad commit
> commit 31e77af205cf6564c2bf4c18400b4ca16bdf92cd
> Author: Pedro Alves <palves@redhat.com>
> Date: Thu Mar 20 13:26:32 2014 +0000
> PR breakpoints/7143 - Watchpoint does not trigger when first set
>
> PASS kernel-3.14.6-200.fc20.x86_64
> FAIL kernel-3.13.10-200.dentrybuflen.fc20.x86_64
That was a red herring, in fact it was due to different GCC.
(gdb) disas marker
Dump of assembler code for function marker:
0x0000000000400826 <+0>: push %rbp
0x0000000000400827 <+1>: mov %rsp,%rbp
=> 0x000000000040082a <+4>: pop %rbp
0x000000000040082b <+5>: retq
End of assembler dump.
ptrace(PTRACE_POKEUSER, 24574, offsetof(struct user, u_debugreg), 0x40082a) = -1 EINVAL (Invalid argument)
New GDB in the 'hbreak' case does not align the breakpoint address.
Attaching gzipped gdb.threads/watchpoint-fork-parent-st
from gcc-4.9.0-9.fc21.x86_64.
Jan
../gdb gdb.threads/watchpoint-fork-parent-st -ex 'set pagination off' -ex start -ex 'hbreak marker' -ex 'watch var' -ex c -ex c -ex c
GNU gdb (GDB) 7.8.50.20140619-cvs
Copyright (C) 2014 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from gdb.threads/watchpoint-fork-parent-st...done.
Temporary breakpoint 1 at 0x400836: file ./gdb.threads/watchpoint-fork-st.c, line 43.
Starting program: /home/jkratoch/redhat/gdb-test-f20-noasan/gdb/testsuite/gdb.threads/watchpoint-fork-parent-st
Temporary breakpoint 1, main () at ./gdb.threads/watchpoint-fork-st.c:43
43 setbuf (stdout, NULL);
Hardware assisted breakpoint 2 at 0x40082a: file ./gdb.threads/watchpoint-fork-st.c, line 33.
Hardware watchpoint 3: var
Continuing.
main: 26352
Breakpoint 2, marker () at ./gdb.threads/watchpoint-fork-st.c:33
33 }
Continuing.
Hardware watchpoint 3: var
Old value = 0
New value = 1
main () at ./gdb.threads/watchpoint-fork-st.c:50
50 forkoff (1);
Continuing.
main () at ./gdb.threads/watchpoint-fork-st.c:50
50 forkoff (1);
Couldn't write debug register: Invalid argument.
(gdb) _
[-- Attachment #2: watchpoint-fork-parent-st.gz --]
[-- Type: application/gzip, Size: 4723 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Regression for watchpoint-fork.exp [Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set]
2014-06-19 13:43 ` Jan Kratochvil
@ 2014-06-19 15:02 ` Pedro Alves
2014-06-19 16:56 ` Pedro Alves
0 siblings, 1 reply; 25+ messages in thread
From: Pedro Alves @ 2014-06-19 15:02 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
On 06/19/2014 02:43 PM, Jan Kratochvil wrote:
> On Tue, 17 Jun 2014 21:18:50 +0200, Jan Kratochvil wrote:
>> On Thu, 20 Mar 2014 14:57:36 +0100, Pedro Alves wrote:
>>> Here's what I pushed (nothing else changed).
>>
>> 31e77af205cf6564c2bf4c18400b4ca16bdf92cd is the first bad commit
>> commit 31e77af205cf6564c2bf4c18400b4ca16bdf92cd
>> Author: Pedro Alves <palves@redhat.com>
>> Date: Thu Mar 20 13:26:32 2014 +0000
>> PR breakpoints/7143 - Watchpoint does not trigger when first set
>>
>> PASS kernel-3.14.6-200.fc20.x86_64
>> FAIL kernel-3.13.10-200.dentrybuflen.fc20.x86_64
>
> That was a red herring, in fact it was due to different GCC.
>
> (gdb) disas marker
> Dump of assembler code for function marker:
> 0x0000000000400826 <+0>: push %rbp
> 0x0000000000400827 <+1>: mov %rsp,%rbp
> => 0x000000000040082a <+4>: pop %rbp
> 0x000000000040082b <+5>: retq
> End of assembler dump.
>
> ptrace(PTRACE_POKEUSER, 24574, offsetof(struct user, u_debugreg), 0x40082a) = -1 EINVAL (Invalid argument)
>
> New GDB in the 'hbreak' case does not align the breakpoint address.
Hmm, I'm confused. Why would the breakpoint address need to be aligned?
And aligned to what?
> Attaching gzipped gdb.threads/watchpoint-fork-parent-st
> from gcc-4.9.0-9.fc21.x86_64.
Thanks, I can reproduce it.
--
Pedro Alves
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Regression for watchpoint-fork.exp [Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set]
2014-06-19 15:02 ` Pedro Alves
@ 2014-06-19 16:56 ` Pedro Alves
2014-06-19 17:00 ` Jan Kratochvil
0 siblings, 1 reply; 25+ messages in thread
From: Pedro Alves @ 2014-06-19 16:56 UTC (permalink / raw)
To: Pedro Alves, Jan Kratochvil; +Cc: Doug Evans, gdb-patches
On 06/19/2014 04:02 PM, Pedro Alves wrote:
>> Attaching gzipped gdb.threads/watchpoint-fork-parent-st
>> from gcc-4.9.0-9.fc21.x86_64.
>
> Thanks, I can reproduce it.
Hmm, I suspect this might be related to kernel-side validation
of DR_CONTROL vs DR0-3, like what we already handle in amd64_linux_prepare_to_resume.
And indeed this below makes the error go away. Not exactly sure why yet.
diff --git c/gdb/amd64-linux-nat.c w/gdb/amd64-linux-nat.c
index 06199af..5972415 100644
--- c/gdb/amd64-linux-nat.c
+++ w/gdb/amd64-linux-nat.c
@@ -415,6 +415,8 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
Ensure DR_CONTROL gets written as the very last register here. */
+ amd64_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
if (state->dr_ref_count[i] > 0)
{
--
Pedro Alves
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Regression for watchpoint-fork.exp [Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set]
2014-06-19 16:56 ` Pedro Alves
@ 2014-06-19 17:00 ` Jan Kratochvil
2014-06-20 16:53 ` [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument Pedro Alves
0 siblings, 1 reply; 25+ messages in thread
From: Jan Kratochvil @ 2014-06-19 17:00 UTC (permalink / raw)
To: Pedro Alves; +Cc: Doug Evans, gdb-patches
On Thu, 19 Jun 2014 18:56:46 +0200, Pedro Alves wrote:
> On 06/19/2014 04:02 PM, Pedro Alves wrote:
>
> >> Attaching gzipped gdb.threads/watchpoint-fork-parent-st
> >> from gcc-4.9.0-9.fc21.x86_64.
> >
> > Thanks, I can reproduce it.
>
> Hmm, I suspect this might be related to kernel-side validation
> of DR_CONTROL vs DR0-3, like what we already handle in amd64_linux_prepare_to_resume.
> And indeed this below makes the error go away. Not exactly sure why yet.
Because there is a data watchpoint of size 4 bytes at that DRx slot and it is
being updated by that 'hbreak' unaligned watchpoint (after the data watchpoint
is no longer valid).
But DR_CONTROL is updated only afterwards.
Your patch seems to be right.
Thanks,
Jan
>
> diff --git c/gdb/amd64-linux-nat.c w/gdb/amd64-linux-nat.c
> index 06199af..5972415 100644
> --- c/gdb/amd64-linux-nat.c
> +++ w/gdb/amd64-linux-nat.c
> @@ -415,6 +415,8 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
>
> Ensure DR_CONTROL gets written as the very last register here. */
>
> + amd64_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
> +
> for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
> if (state->dr_ref_count[i] > 0)
> {
>
> --
> Pedro Alves
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.
2014-06-19 17:00 ` Jan Kratochvil
@ 2014-06-20 16:53 ` Pedro Alves
2014-06-20 17:45 ` Tom Tromey
2014-06-22 18:31 ` Jan Kratochvil
0 siblings, 2 replies; 25+ messages in thread
From: Pedro Alves @ 2014-06-20 16:53 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
On 06/19/2014 06:00 PM, Jan Kratochvil wrote:
> On Thu, 19 Jun 2014 18:56:46 +0200, Pedro Alves wrote:
>> On 06/19/2014 04:02 PM, Pedro Alves wrote:
>>
>>>> Attaching gzipped gdb.threads/watchpoint-fork-parent-st
>>>> from gcc-4.9.0-9.fc21.x86_64.
>>>
>>> Thanks, I can reproduce it.
>>
>> Hmm, I suspect this might be related to kernel-side validation
>> of DR_CONTROL vs DR0-3, like what we already handle in amd64_linux_prepare_to_resume.
>> And indeed this below makes the error go away. Not exactly sure why yet.
>
> Because there is a data watchpoint of size 4 bytes at that DRx slot and it is
> being updated by that 'hbreak' unaligned watchpoint (after the data watchpoint
> is no longer valid).
Indeed. I could reproduce this with 7.7, even.
This is not a 7.8 regression, but I think it can go in safely to the branch.
> But DR_CONTROL is updated only afterwards.
>
> Your patch seems to be right.
Here's a proper patch, with new test included. I've made the test
more exhaustive more generic than the bug in question, in hope of
catching a whole class of issues over all targets, not just this case.
Let me know what you think.
I _still_ had to touch 3 different places with the same code... I'm
very much looking forward to having all that merged.
8<----------
From afc7d46dfeddf7edfd9d28500a8277704e8cf3e1 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 20 Jun 2014 17:41:25 +0100
Subject: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid
argument.
This patch fixes this on x86 Linux:
(gdb) watch *buf@2
Hardware watchpoint 8: *buf@2
(gdb) si
0x00000000004005a7 34 for (i = 0; i < 100000; i++); /* stepi line */
(gdb) del
Delete all breakpoints? (y or n) y
(gdb) watch *(buf+1)@1
Hardware watchpoint 9: *(buf+1)@1
(gdb) si
0x00000000004005a7 in main () at ../../../src/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c:34
34 for (i = 0; i < 100000; i++); /* stepi line */
Couldn't write debug register: Invalid argument.
(gdb)
In the example above the debug registers are being switched from this
state:
CONTROL (DR7): 0000000000050101 STATUS (DR6): 0000000000000000
DR0: addr=0x0000000000601040, ref.count=1 DR1: addr=0x0000000000000000, ref.count=0
DR2: addr=0x0000000000000000, ref.count=0 DR3: addr=0x0000000000000000, ref.count=0
to this:
CONTROL (DR7): 0000000000010101 STATUS (DR6): 0000000000000000
DR0: addr=0x0000000000601041, ref.count=1 DR1: addr=0x0000000000000000, ref.count=0
DR2: addr=0x0000000000000000, ref.count=0 DR3: addr=0x0000000000000000, ref.count=0
That is, before, DR7 was setup for watching a 2 byte region starting
at what's in DR0 (0x601040).
And after, DR7 is setup for watching a 1 byte region starting at
what's in DR0 (0x601041).
We always write DR0..DR3 before DR7, because if we enable a slot's
bits in DR7, you need to have already written the corresponding
DR0..DR3 registers -- the kernel rejects the DR7 write with EINVAL
otherwise.
The error shown above is the opposite scenario. When we try to write
0x601041 to DR0, DR7's bits still indicate intent of watching a 2-byte
region. That DR0/DR7 combination is invalid, because 0x601041 is
unaligned. To watch two bytes, we'd have to use two slots. So the
kernel errors out with EINVAL.
Fix this by always first clearing DR7, then writing DR0..DR3, and then
setting DR7's bits.
A little optimization -- if we're disabling the last watchpoint, then
we can clear DR7 just once. The changes to nat/i386-dregs.c make that
easier to detect, and as bonus, they make it a little easier to make
sense of DR7 in the debug logs, as we no longer need to remember we're
seeing stale bits.
Tested on x86_64 Fedora 20, native and GDBserver.
This adds an exhaustive test that switches between many different
combinations of watchpoint types and addresses and widths.
gdb/
2014-06-20 Pedro Alves <palves@redhat.com>
* amd64-linux-nat.c (amd64_linux_prepare_to_resume): Clear
DR_CONTROL before setting DR0..DR3.
* i386-linux-nat.c (i386_linux_prepare_to_resume): Likewise.
* nat/i386-dregs.c (I386_DR_LOCAL_DISABLE): New macro.
(i386_remove_aligned_watchpoint): Clear all bits of DR_CONTROL
related to the debug register slot being disabled. If all slots
are vacant, clear local slowdown as well, and assert DR_CONTROL is
0.
gdb/gdbserver/
2014-06-20 Pedro Alves <palves@redhat.com>
* linux-x86-low.c (x86_linux_prepare_to_resume): Clear DR_CONTROL
before setting DR0..DR3
gdb/testsuite/
2014-06-20 Pedro Alves <palves@redhat.com>
* gdb.base/watchpoint-reuse-slot.c: New file.
* gdb.base/watchpoint-reuse-slot.exp: New file.
---
gdb/amd64-linux-nat.c | 10 +-
gdb/gdbserver/linux-x86-low.c | 5 +-
gdb/i386-linux-nat.c | 5 +-
gdb/nat/i386-dregs.c | 28 ++++
gdb/testsuite/gdb.base/watchpoint-reuse-slot.c | 37 +++++
gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 187 +++++++++++++++++++++++
6 files changed, 269 insertions(+), 3 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
create mode 100644 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
index 0cdc1f0..0cc146b 100644
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -415,6 +415,11 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
Ensure DR_CONTROL gets written as the very last register here. */
+ /* Clear DR_CONTROL first. In some cases, setting DR0-3 to a
+ value that doesn't match what is enabled in DR_CONTROL
+ returns in EINVAL. */
+ amd64_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
if (state->dr_ref_count[i] > 0)
{
@@ -427,7 +432,10 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
clear_status = 1;
}
- amd64_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
+ /* If DR_CONTROL is supposed to be zero, we've already set it
+ above. */
+ if (state->dr_control_mirror != 0)
+ amd64_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
lwp->arch_private->debug_registers_changed = 0;
}
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 94451da..9ba71e5 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -788,6 +788,8 @@ x86_linux_prepare_to_resume (struct lwp_info *lwp)
struct i386_debug_reg_state *state
= &proc->private->arch_private->debug_reg_state;
+ x86_linux_dr_set (ptid, DR_CONTROL, 0);
+
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
if (state->dr_ref_count[i] > 0)
{
@@ -800,7 +802,8 @@ x86_linux_prepare_to_resume (struct lwp_info *lwp)
clear_status = 1;
}
- x86_linux_dr_set (ptid, DR_CONTROL, state->dr_control_mirror);
+ if (state->dr_control_mirror != 0)
+ x86_linux_dr_set (ptid, DR_CONTROL, state->dr_control_mirror);
lwp->arch_private->debug_registers_changed = 0;
}
diff --git a/gdb/i386-linux-nat.c b/gdb/i386-linux-nat.c
index 6667c17..ff96501 100644
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -778,6 +778,8 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
/* See amd64_linux_prepare_to_resume for Linux kernel note on
i386_linux_dr_set calls ordering. */
+ i386_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
if (state->dr_ref_count[i] > 0)
{
@@ -790,7 +792,8 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
clear_status = 1;
}
- i386_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
+ if (state->dr_control_mirror != 0)
+ i386_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
lwp->arch_private->debug_registers_changed = 0;
}
diff --git a/gdb/nat/i386-dregs.c b/gdb/nat/i386-dregs.c
index 214616c..54ecf23 100644
--- a/gdb/nat/i386-dregs.c
+++ b/gdb/nat/i386-dregs.c
@@ -141,6 +141,13 @@
(1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))); \
} while (0)
+/* Locally disable the break/watchpoint in the I'th debug register. */
+#define I386_DR_LOCAL_DISABLE(state, i) \
+ do { \
+ (state)->dr_control_mirror &= \
+ ~(1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))); \
+ } while (0)
+
/* Globally enable the break/watchpoint in the I'th debug register. */
#define I386_DR_GLOBAL_ENABLE(state, i) \
do { \
@@ -348,6 +355,7 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
CORE_ADDR addr, unsigned len_rw_bits)
{
int i, retval = -1;
+ int all_vacant = 1;
ALL_DEBUG_REGISTERS (i)
{
@@ -360,11 +368,31 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
/* Reset our mirror. */
state->dr_mirror[i] = 0;
I386_DR_DISABLE (state, i);
+ /* Even though not strictly necessary, clear out all
+ bits in DR_CONTROL related to this debug register.
+ Debug output is clearer when we don't have stale bits
+ in place. This also allows the assertion below. */
+ I386_DR_LOCAL_DISABLE (state, i);
+ I386_DR_SET_RW_LEN (state, i, 0);
}
retval = 0;
}
+
+ if (!I386_DR_VACANT (state, i))
+ all_vacant = 0;
}
+ if (all_vacant)
+ {
+ /* Even though not strictly necessary, clear out all of
+ DR_CONTROL, so that when we have no debug registers in use,
+ we end up with DR_CONTROL == 0. The Linux support relies on
+ this for an optimization. Plus, it makes for clearer debug
+ output. */
+ state->dr_control_mirror &= ~DR_LOCAL_SLOWDOWN;
+
+ gdb_assert (state->dr_control_mirror == 0);
+ }
return retval;
}
diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
new file mode 100644
index 0000000..a5a8f5c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 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/>. */
+
+union aligned_buf
+{
+ char byte[12];
+
+ /* So that testing consistently starts on an aligned address. */
+ unsigned long long force_align;
+};
+
+union aligned_buf buf;
+
+int
+main (void)
+{
+ volatile int i = 0;
+
+ /* Must be a single line. */
+ for (i = 0; i < 100000; i++); /* stepi line */
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
new file mode 100644
index 0000000..3376e1f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
@@ -0,0 +1,187 @@
+# Copyright 2014 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/>.
+
+# Test alternating between watchpoint types, watching a sliding window
+# of addresses (thus alternating between aligned and unaligned
+# addresses). Only a single watchpoint exists at any given time. On
+# targets that only update the debug registers on resume, this
+# stresses the debug register setup code, both in GDB and in the
+# target/kernel as one watchpoint replaces the other in a single
+# operation. (Note that we don't have any of these watchpoints
+# trigger.)
+
+if [target_info exists gdb,no_hardware_watchpoints] {
+ unsupported "no target support"
+ return
+}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+ return -1
+}
+
+if ![runto_main] then {
+ fail "Can't run to main"
+ return 0
+}
+
+# The line we'll be stepping.
+set srcline [gdb_get_line_number "stepi line"]
+
+# The address the program is stopped at currently.
+set cur_addr ""
+
+# Get the current PC.
+
+proc get_pc {} {
+ global hex gdb_prompt
+
+ set addr ""
+ set test "get PC"
+ gdb_test_multiple "p /x \$pc" "$test" {
+ -re " = ($hex).*$gdb_prompt $" {
+ set addr $expect_out(1,string)
+ pass "$test"
+ }
+ }
+
+ return $addr
+}
+
+
+# Issue a stepi, and make sure the program advanced past the current
+# instruction (stored in the CUR_ADDR global).
+
+proc stepi {} {
+ global hex gdb_prompt cur_addr
+
+ set srcline " for (i = 0; i < 100000; i++); /* stepi line */"
+ set test "stepi advanced"
+ gdb_test_multiple "stepi" $test {
+ -re "($hex).*[string_to_regexp $srcline]\r\n$gdb_prompt $" {
+ set addr $expect_out(1,string)
+ if {$addr != $cur_addr} {
+ pass $test
+ } else {
+ fail $test
+ }
+ set cur_addr addr
+ }
+ }
+}
+
+gdb_breakpoint $srcline
+gdb_continue_to_breakpoint "stepi line"
+
+# The test tries various sequences of different types of watchpoints.
+# Probe for support first.
+
+# So we get an immediate warning/error if the target doesn't support a
+# given watchpoint type.
+gdb_test_no_output "set breakpoint always-inserted on"
+
+# The list of supported commands. Below we'll probe for support and
+# add elements to this list.
+set cmds {}
+
+foreach cmd {"watch" "awatch" "rwatch"} {
+ set test $cmd
+ gdb_test_multiple "$cmd buf.byte\[0\]" $test {
+ -re "You may have requested too many.*$gdb_prompt $" {
+ unsupported $test
+ }
+ -re "$gdb_prompt $" {
+ pass $test
+ lappend cmds $cmd
+ }
+ }
+
+ delete_breakpoints
+}
+
+set test "hbreak"
+gdb_test_multiple "hbreak main" $test {
+ -re "You may have requested too many.*$gdb_prompt $" {
+ pass $test
+ }
+ -re "$gdb_prompt $" {
+ pass $test
+ lappend cmds "hbreak"
+ }
+}
+
+delete_breakpoints
+
+set cur_addr [get_pc]
+
+# Watch WIDTH bytes at BASE + OFFSET. CMD specifices the specific
+# type of watchpoint to use. If CMD is "hbreak", WIDTH is ignored.
+
+proc watch_command {cmd base offset width} {
+ global srcfile srcline hex
+
+ if {$cmd == "hbreak"} {
+ set expr "*(buf.byte + $base + $offset)"
+ gdb_test "hbreak $expr" "Hardware assisted breakpoint \[0-9\]+ at $hex"
+ } elseif {$cmd == "watch"} {
+ set expr "*(buf.byte + $base + $offset)@$width"
+ gdb_test "$cmd $expr" \
+ "Hardware watchpoint \[0-9\]+: [string_to_regexp $expr]"
+ } elseif {$cmd == "awatch"} {
+ set expr "*(buf.byte + $base + $offset)@$width"
+ gdb_test "$cmd $expr" \
+ "Hardware access \\(read/write\\) watchpoint \[0-9\]+: [string_to_regexp $expr]"
+ } elseif {$cmd == "rwatch"} {
+ set expr "*(buf.byte + $base + $offset)@$width"
+ gdb_test "$cmd $expr" \
+ "Hardware read watchpoint \[0-9\]+: [string_to_regexp $expr]"
+ }
+}
+
+# Run test proper. See intro for description.
+
+foreach always_inserted {"off" "on" } {
+ gdb_test_no_output "set breakpoint always-inserted $always_inserted"
+ foreach cmd1 $cmds {
+ foreach cmd2 $cmds {
+ for {set width 1} {$width < 4} {incr width} {
+
+ if {$cmd1 == "hbreak" && $cmd2 == "hbreak" && $width > 1} {
+ # hbreak ignores with $width, no use testing more
+ # than once.
+ continue
+ }
+
+ for {set x 0} {$x < 4} {incr x} {
+ set prefix "always-inserted $always_inserted: "
+ append prefix "$cmd1 x $cmd2: "
+ with_test_prefix "$prefix: width $width, iter $x" {
+ with_test_prefix "base + 0" {
+ watch_command $cmd1 $x 0 $width
+ stepi
+ gdb_test_no_output "delete \$bpnum"
+ }
+ with_test_prefix "base + 1" {
+ watch_command $cmd2 $x 1 $width
+ stepi
+ gdb_test_no_output "delete \$bpnum"
+ }
+ }
+ }
+ }
+ }
+ }
+}
--
1.9.3
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.
2014-06-20 16:53 ` [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument Pedro Alves
@ 2014-06-20 17:45 ` Tom Tromey
2014-06-20 17:52 ` Pedro Alves
2014-06-22 18:31 ` Jan Kratochvil
1 sibling, 1 reply; 25+ messages in thread
From: Tom Tromey @ 2014-06-20 17:45 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
Pedro> I _still_ had to touch 3 different places with the same code... I'm
Pedro> very much looking forward to having all that merged.
Yeah. In the meantime perhaps copying the comments into all 3 copies
would make it simpler for the eventual merge. I noticed that the
comments are not in linux-x86-low.c or i386-linux-nat.c.
Tom
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.
2014-06-20 17:45 ` Tom Tromey
@ 2014-06-20 17:52 ` Pedro Alves
2014-06-20 17:53 ` Tom Tromey
0 siblings, 1 reply; 25+ messages in thread
From: Pedro Alves @ 2014-06-20 17:52 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 06/20/2014 06:45 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Pedro> I _still_ had to touch 3 different places with the same code... I'm
> Pedro> very much looking forward to having all that merged.
>
> Yeah. In the meantime perhaps copying the comments into all 3 copies
> would make it simpler for the eventual merge. I noticed that the
> comments are not in linux-x86-low.c or i386-linux-nat.c.
That's following the existing practice, actually.
amd64-linux-nat.c has:
/* On Linux kernel before 2.6.33 commit
72f674d203cd230426437cdcf7dd6f681dad8b0d
if you enable a breakpoint by the DR_CONTROL bits you need to have
already written the corresponding DR_FIRSTADDR...DR_LASTADDR registers.
Ensure DR_CONTROL gets written as the very last register here. */
while i386-linux-nat.c just has:
/* See amd64_linux_prepare_to_resume for Linux kernel note on
i386_linux_dr_set calls ordering. */
I don't think trying to keep this in sync now would help, and at least
this way we have one place that is master. Gary's recent series showed
how when these comments are duplicated they end up diverging. :-/
--
Pedro Alves
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.
2014-06-20 17:52 ` Pedro Alves
@ 2014-06-20 17:53 ` Tom Tromey
0 siblings, 0 replies; 25+ messages in thread
From: Tom Tromey @ 2014-06-20 17:53 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
Pedro> That's following the existing practice, actually.
Thanks, I wasn't aware of this.
That's sufficient for me.
Tom
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.
2014-06-20 16:53 ` [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument Pedro Alves
2014-06-20 17:45 ` Tom Tromey
@ 2014-06-22 18:31 ` Jan Kratochvil
2014-06-23 17:41 ` Pedro Alves
1 sibling, 1 reply; 25+ messages in thread
From: Jan Kratochvil @ 2014-06-22 18:31 UTC (permalink / raw)
To: Pedro Alves; +Cc: Doug Evans, gdb-patches
On Fri, 20 Jun 2014 18:53:26 +0200, Pedro Alves wrote:
> I could reproduce this with 7.7, even.
>
> This is not a 7.8 regression, but I think it can go in safely to the branch.
While I agree it still is a testsuite results regression.
[...]
> --- a/gdb/nat/i386-dregs.c
> +++ b/gdb/nat/i386-dregs.c
> @@ -141,6 +141,13 @@
> (1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))); \
> } while (0)
>
> +/* Locally disable the break/watchpoint in the I'th debug register. */
> +#define I386_DR_LOCAL_DISABLE(state, i) \
> + do { \
> + (state)->dr_control_mirror &= \
> + ~(1 << (DR_LOCAL_ENABLE_SHIFT + DR_ENABLE_SIZE * (i))); \
> + } while (0)
> +
> /* Globally enable the break/watchpoint in the I'th debug register. */
> #define I386_DR_GLOBAL_ENABLE(state, i) \
> do { \
> @@ -348,6 +355,7 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
> CORE_ADDR addr, unsigned len_rw_bits)
> {
> int i, retval = -1;
> + int all_vacant = 1;
>
> ALL_DEBUG_REGISTERS (i)
> {
> @@ -360,11 +368,31 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
> /* Reset our mirror. */
> state->dr_mirror[i] = 0;
> I386_DR_DISABLE (state, i);
> + /* Even though not strictly necessary, clear out all
> + bits in DR_CONTROL related to this debug register.
> + Debug output is clearer when we don't have stale bits
> + in place. This also allows the assertion below. */
> + I386_DR_LOCAL_DISABLE (state, i);
This is redundant, I386_DR_DISABLE already does both GLOBAL_DISABLE-like and
LOCAL_DISABLE-like.
> + I386_DR_SET_RW_LEN (state, i, 0);
> }
> retval = 0;
> }
> +
> + if (!I386_DR_VACANT (state, i))
> + all_vacant = 0;
> }
>
> + if (all_vacant)
> + {
> + /* Even though not strictly necessary, clear out all of
> + DR_CONTROL, so that when we have no debug registers in use,
> + we end up with DR_CONTROL == 0. The Linux support relies on
> + this for an optimization. Plus, it makes for clearer debug
> + output. */
> + state->dr_control_mirror &= ~DR_LOCAL_SLOWDOWN;
> +
> + gdb_assert (state->dr_control_mirror == 0);
> + }
> return retval;
> }
>
[...]
Thanks,
Jan
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.
2014-06-22 18:31 ` Jan Kratochvil
@ 2014-06-23 17:41 ` Pedro Alves
2014-06-23 17:44 ` Pedro Alves
0 siblings, 1 reply; 25+ messages in thread
From: Pedro Alves @ 2014-06-23 17:41 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
On 06/22/2014 07:31 PM, Jan Kratochvil wrote:
> This is redundant, I386_DR_DISABLE already does both GLOBAL_DISABLE-like and
> LOCAL_DISABLE-like.
Indeed! I've removed that, and pushed the patch, as below. The 7.8 branch
doesn't have the shared nat/i386-dregs.c file, so it needed a tweaked version...
8<-------------
From 8e9db26e299e5c9d03102d7c9551113db18719b1 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 23 Jun 2014 16:44:04 +0100
Subject: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid
argument.
This patch fixes this on x86 Linux:
(gdb) watch *buf@2
Hardware watchpoint 8: *buf@2
(gdb) si
0x00000000004005a7 34 for (i = 0; i < 100000; i++); /* stepi line */
(gdb) del
Delete all breakpoints? (y or n) y
(gdb) watch *(buf+1)@1
Hardware watchpoint 9: *(buf+1)@1
(gdb) si
0x00000000004005a7 in main () at ../../../src/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c:34
34 for (i = 0; i < 100000; i++); /* stepi line */
Couldn't write debug register: Invalid argument.
(gdb)
In the example above the debug registers are being switched from this
state:
CONTROL (DR7): 0000000000050101 STATUS (DR6): 0000000000000000
DR0: addr=0x0000000000601040, ref.count=1 DR1: addr=0x0000000000000000, ref.count=0
DR2: addr=0x0000000000000000, ref.count=0 DR3: addr=0x0000000000000000, ref.count=0
to this:
CONTROL (DR7): 0000000000010101 STATUS (DR6): 0000000000000000
DR0: addr=0x0000000000601041, ref.count=1 DR1: addr=0x0000000000000000, ref.count=0
DR2: addr=0x0000000000000000, ref.count=0 DR3: addr=0x0000000000000000, ref.count=0
That is, before, DR7 was setup for watching a 2 byte region starting
at what's in DR0 (0x601040).
And after, DR7 is setup for watching a 1 byte region starting at
what's in DR0 (0x601041).
We always write DR0..DR3 before DR7, because if we enable a slot's
bits in DR7, you need to have already written the corresponding
DR0..DR3 registers -- the kernel rejects the DR7 write with EINVAL
otherwise.
The error shown above is the opposite scenario. When we try to write
0x601041 to DR0, DR7's bits still indicate intent of watching a 2-byte
region. That DR0/DR7 combination is invalid, because 0x601041 is
unaligned. To watch two bytes, we'd have to use two slots. So the
kernel errors out with EINVAL.
Fix this by always first clearing DR7, then writing DR0..DR3, and then
setting DR7's bits.
A little optimization -- if we're disabling the last watchpoint, then
we can clear DR7 just once. The changes to nat/i386-dregs.c make that
easier to detect, and as bonus, they make it a little easier to make
sense of DR7 in the debug logs, as we no longer need to remember we're
seeing stale bits.
Tested on x86_64 Fedora 20, native and GDBserver.
This adds an exhaustive test that switches between many different
combinations of watchpoint types and addresses and widths.
gdb/
2014-06-23 Pedro Alves <palves@redhat.com>
* amd64-linux-nat.c (amd64_linux_prepare_to_resume): Clear
DR_CONTROL before setting DR0..DR3.
* i386-linux-nat.c (i386_linux_prepare_to_resume): Likewise.
* nat/i386-dregs.c (i386_remove_aligned_watchpoint): Clear all
bits of DR_CONTROL related to the debug register slot being
disabled. If all slots are vacant, clear local slowdown as well,
and assert DR_CONTROL is 0.
gdb/gdbserver/
2014-06-23 Pedro Alves <palves@redhat.com>
* linux-x86-low.c (x86_linux_prepare_to_resume): Clear DR_CONTROL
before setting DR0..DR3.
gdb/testsuite/
2014-06-23 Pedro Alves <palves@redhat.com>
* gdb.base/watchpoint-reuse-slot.c: New file.
* gdb.base/watchpoint-reuse-slot.exp: New file.
---
gdb/ChangeLog | 10 ++
gdb/gdbserver/ChangeLog | 5 +
gdb/testsuite/ChangeLog | 5 +
gdb/amd64-linux-nat.c | 10 +-
gdb/gdbserver/linux-x86-low.c | 5 +-
gdb/i386-linux-nat.c | 5 +-
gdb/nat/i386-dregs.c | 20 +++
gdb/testsuite/gdb.base/watchpoint-reuse-slot.c | 37 +++++
gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 187 +++++++++++++++++++++++
9 files changed, 281 insertions(+), 3 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
create mode 100644 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 47a5fa5..be5669f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2014-06-23 Pedro Alves <palves@redhat.com>
+
+ * amd64-linux-nat.c (amd64_linux_prepare_to_resume): Clear
+ DR_CONTROL before setting DR0..DR3.
+ * i386-linux-nat.c (i386_linux_prepare_to_resume): Likewise.
+ * nat/i386-dregs.c (i386_remove_aligned_watchpoint): Clear all
+ bits of DR_CONTROL related to the debug register slot being
+ disabled. If all slots are vacant, clear local slowdown as well,
+ and assert DR_CONTROL is 0.
+
2014-06-23 Siva Chandra Reddy <sivachandra@google.com>
* python/lib/gdb/command/xmethods.py
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 085677b..cd5aa0f 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-23 Pedro Alves <palves@redhat.com>
+
+ * linux-x86-low.c (x86_linux_prepare_to_resume): Clear DR_CONTROL
+ before setting DR0..DR3.
+
2014-06-20 Gary Benson <gbenson@redhat.com>
* configure.ac (AC_REPLACE_FUNCS) <vasprintf, vsnprintf>: Removed.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 57c5adc..2c8f159 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-23 Pedro Alves <palves@redhat.com>
+
+ * gdb.base/watchpoint-reuse-slot.c: New file.
+ * gdb.base/watchpoint-reuse-slot.exp: New file.
+
2014-06-23 Siva Chandra Reddy <sivachandra@google.com>
* gdb.python/py-xmethods.exp: Use "progspace" instead of the
diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
index 0cdc1f0..fa677c8 100644
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -415,6 +415,11 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
Ensure DR_CONTROL gets written as the very last register here. */
+ /* Clear DR_CONTROL first. In some cases, setting DR0-3 to a
+ value that doesn't match what is enabled in DR_CONTROL
+ results in EINVAL. */
+ amd64_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
if (state->dr_ref_count[i] > 0)
{
@@ -427,7 +432,10 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
clear_status = 1;
}
- amd64_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
+ /* If DR_CONTROL is supposed to be zero, we've already set it
+ above. */
+ if (state->dr_control_mirror != 0)
+ amd64_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
lwp->arch_private->debug_registers_changed = 0;
}
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 94451da..9ba71e5 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -788,6 +788,8 @@ x86_linux_prepare_to_resume (struct lwp_info *lwp)
struct i386_debug_reg_state *state
= &proc->private->arch_private->debug_reg_state;
+ x86_linux_dr_set (ptid, DR_CONTROL, 0);
+
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
if (state->dr_ref_count[i] > 0)
{
@@ -800,7 +802,8 @@ x86_linux_prepare_to_resume (struct lwp_info *lwp)
clear_status = 1;
}
- x86_linux_dr_set (ptid, DR_CONTROL, state->dr_control_mirror);
+ if (state->dr_control_mirror != 0)
+ x86_linux_dr_set (ptid, DR_CONTROL, state->dr_control_mirror);
lwp->arch_private->debug_registers_changed = 0;
}
diff --git a/gdb/i386-linux-nat.c b/gdb/i386-linux-nat.c
index 6667c17..ff96501 100644
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -778,6 +778,8 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
/* See amd64_linux_prepare_to_resume for Linux kernel note on
i386_linux_dr_set calls ordering. */
+ i386_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
if (state->dr_ref_count[i] > 0)
{
@@ -790,7 +792,8 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
clear_status = 1;
}
- i386_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
+ if (state->dr_control_mirror != 0)
+ i386_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
lwp->arch_private->debug_registers_changed = 0;
}
diff --git a/gdb/nat/i386-dregs.c b/gdb/nat/i386-dregs.c
index 214616c..1fa5c19 100644
--- a/gdb/nat/i386-dregs.c
+++ b/gdb/nat/i386-dregs.c
@@ -348,6 +348,7 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
CORE_ADDR addr, unsigned len_rw_bits)
{
int i, retval = -1;
+ int all_vacant = 1;
ALL_DEBUG_REGISTERS (i)
{
@@ -360,11 +361,30 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
/* Reset our mirror. */
state->dr_mirror[i] = 0;
I386_DR_DISABLE (state, i);
+ /* Even though not strictly necessary, clear out all
+ bits in DR_CONTROL related to this debug register.
+ Debug output is clearer when we don't have stale bits
+ in place. This also allows the assertion below. */
+ I386_DR_SET_RW_LEN (state, i, 0);
}
retval = 0;
}
+
+ if (!I386_DR_VACANT (state, i))
+ all_vacant = 0;
}
+ if (all_vacant)
+ {
+ /* Even though not strictly necessary, clear out all of
+ DR_CONTROL, so that when we have no debug registers in use,
+ we end up with DR_CONTROL == 0. The Linux support relies on
+ this for an optimization. Plus, it makes for clearer debug
+ output. */
+ state->dr_control_mirror &= ~DR_LOCAL_SLOWDOWN;
+
+ gdb_assert (state->dr_control_mirror == 0);
+ }
return retval;
}
diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
new file mode 100644
index 0000000..a5a8f5c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 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/>. */
+
+union aligned_buf
+{
+ char byte[12];
+
+ /* So that testing consistently starts on an aligned address. */
+ unsigned long long force_align;
+};
+
+union aligned_buf buf;
+
+int
+main (void)
+{
+ volatile int i = 0;
+
+ /* Must be a single line. */
+ for (i = 0; i < 100000; i++); /* stepi line */
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
new file mode 100644
index 0000000..aa30398
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
@@ -0,0 +1,187 @@
+# Copyright 2014 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/>.
+
+# Test alternating between watchpoint types, watching a sliding window
+# of addresses (thus alternating between aligned and unaligned
+# addresses). Only a single watchpoint exists at any given time. On
+# targets that only update the debug registers on resume, this
+# stresses the debug register setup code, both in GDB and in the
+# target/kernel as one watchpoint replaces the other in a single
+# operation. (Note that we don't have any of these watchpoints
+# trigger.)
+
+if [target_info exists gdb,no_hardware_watchpoints] {
+ unsupported "no target support"
+ return
+}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+ return -1
+}
+
+if ![runto_main] then {
+ fail "Can't run to main"
+ return 0
+}
+
+# The line we'll be stepping.
+set srcline [gdb_get_line_number "stepi line"]
+
+# The address the program is stopped at currently.
+set cur_addr ""
+
+# Get the current PC.
+
+proc get_pc {} {
+ global hex gdb_prompt
+
+ set addr ""
+ set test "get PC"
+ gdb_test_multiple "p /x \$pc" "$test" {
+ -re " = ($hex).*$gdb_prompt $" {
+ set addr $expect_out(1,string)
+ pass "$test"
+ }
+ }
+
+ return $addr
+}
+
+
+# Issue a stepi, and make sure the program advanced past the current
+# instruction (stored in the CUR_ADDR global).
+
+proc stepi {} {
+ global hex gdb_prompt cur_addr
+
+ set srcline " for (i = 0; i < 100000; i++); /* stepi line */"
+ set test "stepi advanced"
+ gdb_test_multiple "stepi" $test {
+ -re "($hex).*[string_to_regexp $srcline]\r\n$gdb_prompt $" {
+ set addr $expect_out(1,string)
+ if {$addr != $cur_addr} {
+ pass $test
+ } else {
+ fail $test
+ }
+ set cur_addr addr
+ }
+ }
+}
+
+gdb_breakpoint $srcline
+gdb_continue_to_breakpoint "stepi line"
+
+# The test tries various sequences of different types of watchpoints.
+# Probe for support first.
+
+# So we get an immediate warning/error if the target doesn't support a
+# given watchpoint type.
+gdb_test_no_output "set breakpoint always-inserted on"
+
+# The list of supported commands. Below we'll probe for support and
+# add elements to this list.
+set cmds {}
+
+foreach cmd {"watch" "awatch" "rwatch"} {
+ set test $cmd
+ gdb_test_multiple "$cmd buf.byte\[0\]" $test {
+ -re "You may have requested too many.*$gdb_prompt $" {
+ unsupported $test
+ }
+ -re "$gdb_prompt $" {
+ pass $test
+ lappend cmds $cmd
+ }
+ }
+
+ delete_breakpoints
+}
+
+set test "hbreak"
+gdb_test_multiple "hbreak main" $test {
+ -re "You may have requested too many.*$gdb_prompt $" {
+ pass $test
+ }
+ -re "$gdb_prompt $" {
+ pass $test
+ lappend cmds "hbreak"
+ }
+}
+
+delete_breakpoints
+
+set cur_addr [get_pc]
+
+# Watch WIDTH bytes at BASE + OFFSET. CMD specifices the specific
+# type of watchpoint to use. If CMD is "hbreak", WIDTH is ignored.
+
+proc watch_command {cmd base offset width} {
+ global srcfile srcline hex
+
+ if {$cmd == "hbreak"} {
+ set expr "*(buf.byte + $base + $offset)"
+ gdb_test "hbreak $expr" "Hardware assisted breakpoint \[0-9\]+ at $hex"
+ } elseif {$cmd == "watch"} {
+ set expr "*(buf.byte + $base + $offset)@$width"
+ gdb_test "$cmd $expr" \
+ "Hardware watchpoint \[0-9\]+: [string_to_regexp $expr]"
+ } elseif {$cmd == "awatch"} {
+ set expr "*(buf.byte + $base + $offset)@$width"
+ gdb_test "$cmd $expr" \
+ "Hardware access \\(read/write\\) watchpoint \[0-9\]+: [string_to_regexp $expr]"
+ } elseif {$cmd == "rwatch"} {
+ set expr "*(buf.byte + $base + $offset)@$width"
+ gdb_test "$cmd $expr" \
+ "Hardware read watchpoint \[0-9\]+: [string_to_regexp $expr]"
+ }
+}
+
+# Run test proper. See intro for description.
+
+foreach always_inserted {"off" "on" } {
+ gdb_test_no_output "set breakpoint always-inserted $always_inserted"
+ foreach cmd1 $cmds {
+ foreach cmd2 $cmds {
+ for {set width 1} {$width < 4} {incr width} {
+
+ if {$cmd1 == "hbreak" && $cmd2 == "hbreak" && $width > 1} {
+ # hbreak ignores WIDTH, no use testing more than
+ # once.
+ continue
+ }
+
+ for {set x 0} {$x < 4} {incr x} {
+ set prefix "always-inserted $always_inserted: "
+ append prefix "$cmd1 x $cmd2: "
+ with_test_prefix "$prefix: width $width, iter $x" {
+ with_test_prefix "base + 0" {
+ watch_command $cmd1 $x 0 $width
+ stepi
+ gdb_test_no_output "delete \$bpnum"
+ }
+ with_test_prefix "base + 1" {
+ watch_command $cmd2 $x 1 $width
+ stepi
+ gdb_test_no_output "delete \$bpnum"
+ }
+ }
+ }
+ }
+ }
+ }
+}
--
1.9.3
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument.
2014-06-23 17:41 ` Pedro Alves
@ 2014-06-23 17:44 ` Pedro Alves
0 siblings, 0 replies; 25+ messages in thread
From: Pedro Alves @ 2014-06-23 17:44 UTC (permalink / raw)
To: Pedro Alves, Jan Kratochvil; +Cc: Doug Evans, gdb-patches
On 06/23/2014 06:41 PM, Pedro Alves wrote:
> On 06/22/2014 07:31 PM, Jan Kratochvil wrote:
>
>> This is redundant, I386_DR_DISABLE already does both GLOBAL_DISABLE-like and
>> LOCAL_DISABLE-like.
>
> Indeed! I've removed that, and pushed the patch, as below. The 7.8 branch
> doesn't have the shared nat/i386-dregs.c file, so it needed a tweaked version...
... here's what I pushed to the 7.8 branch.
------------
From 02350eddb14a719e4c53a1fc93e3f5fd1e762504 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 23 Jun 2014 16:57:25 +0100
Subject: [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid
argument.
This patch fixes this on x86 Linux:
(gdb) watch *buf@2
Hardware watchpoint 8: *buf@2
(gdb) si
0x00000000004005a7 34 for (i = 0; i < 100000; i++); /* stepi line */
(gdb) del
Delete all breakpoints? (y or n) y
(gdb) watch *(buf+1)@1
Hardware watchpoint 9: *(buf+1)@1
(gdb) si
0x00000000004005a7 in main () at ../../../src/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c:34
34 for (i = 0; i < 100000; i++); /* stepi line */
Couldn't write debug register: Invalid argument.
(gdb)
In the example above the debug registers are being switched from this
state:
CONTROL (DR7): 0000000000050101 STATUS (DR6): 0000000000000000
DR0: addr=0x0000000000601040, ref.count=1 DR1: addr=0x0000000000000000, ref.count=0
DR2: addr=0x0000000000000000, ref.count=0 DR3: addr=0x0000000000000000, ref.count=0
to this:
CONTROL (DR7): 0000000000010101 STATUS (DR6): 0000000000000000
DR0: addr=0x0000000000601041, ref.count=1 DR1: addr=0x0000000000000000, ref.count=0
DR2: addr=0x0000000000000000, ref.count=0 DR3: addr=0x0000000000000000, ref.count=0
That is, before, DR7 was setup for watching a 2 byte region starting
at what's in DR0 (0x601040).
And after, DR7 is setup for watching a 1 byte region starting at
what's in DR0 (0x601041).
We always write DR0..DR3 before DR7, because if we enable a slot's
bits in DR7, you need to have already written the corresponding
DR0..DR3 registers -- the kernel rejects the DR7 write with EINVAL
otherwise.
The error shown above is the opposite scenario. When we try to write
0x601041 to DR0, DR7's bits still indicate intent of watching a 2-byte
region. That DR0/DR7 combination is invalid, because 0x601041 is
unaligned. To watch two bytes, we'd have to use two slots. So the
kernel errors out with EINVAL.
Fix this by always first clearing DR7, then writing DR0..DR3, and then
setting DR7's bits.
A little optimization -- if we're disabling the last watchpoint, then
we can clear DR7 just once. The changes to nat/i386-dregs.c make that
easier to detect, and as bonus, they make it a little easier to make
sense of DR7 in the debug logs, as we no longer need to remember we're
seeing stale bits.
Tested on x86_64 Fedora 20, native and GDBserver.
This adds an exhaustive test that switches between many different
combinations of watchpoint types and addresses and widths.
gdb/
2014-06-23 Pedro Alves <palves@redhat.com>
* amd64-linux-nat.c (amd64_linux_prepare_to_resume): Clear
DR_CONTROL before setting DR0..DR3.
* i386-linux-nat.c (i386_linux_prepare_to_resume): Likewise.
* i386-nat.c (i386_remove_aligned_watchpoint): Clear all bits of
DR_CONTROL related to the debug register slot being disabled. If
all slots are vacant, clear local slowdown as well, and assert
DR_CONTROL is 0.
gdb/gdbserver/
2014-06-23 Pedro Alves <palves@redhat.com>
* linux-x86-low.c (x86_linux_prepare_to_resume): Clear DR_CONTROL
before setting DR0..DR3.
* i386-low.c (i386_remove_aligned_watchpoint): Clear all bits of
DR_CONTROL related to the debug register slot being disabled. If
all slots are vacant, clear local slowdown as well, and assert
DR_CONTROL is 0.
gdb/testsuite/
2014-06-23 Pedro Alves <palves@redhat.com>
* gdb.base/watchpoint-reuse-slot.c: New file.
* gdb.base/watchpoint-reuse-slot.exp: New file.
---
gdb/ChangeLog | 10 ++
gdb/gdbserver/ChangeLog | 9 ++
gdb/testsuite/ChangeLog | 5 +
gdb/amd64-linux-nat.c | 10 +-
gdb/gdbserver/i386-low.c | 20 +++
gdb/gdbserver/linux-x86-low.c | 5 +-
gdb/i386-linux-nat.c | 5 +-
gdb/i386-nat.c | 20 +++
gdb/testsuite/gdb.base/watchpoint-reuse-slot.c | 37 +++++
gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp | 187 +++++++++++++++++++++++
10 files changed, 305 insertions(+), 3 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
create mode 100644 gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 5eebf91..b988630 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2014-06-23 Pedro Alves <palves@redhat.com>
+
+ * amd64-linux-nat.c (amd64_linux_prepare_to_resume): Clear
+ DR_CONTROL before setting DR0..DR3.
+ * i386-linux-nat.c (i386_linux_prepare_to_resume): Likewise.
+ * i386-nat.c (i386_remove_aligned_watchpoint): Clear all bits of
+ DR_CONTROL related to the debug register slot being disabled. If
+ all slots are vacant, clear local slowdown as well, and assert
+ DR_CONTROL is 0.
+
2014-06-19 Pedro Alves <palves@redhat.com>
* gdbthread.h (ALL_THREADS): Delete.
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 71c7bcb..8da676a 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,12 @@
+2014-06-23 Pedro Alves <palves@redhat.com>
+
+ * linux-x86-low.c (x86_linux_prepare_to_resume): Clear DR_CONTROL
+ before setting DR0..DR3.
+ * i386-low.c (i386_remove_aligned_watchpoint): Clear all bits of
+ DR_CONTROL related to the debug register slot being disabled. If
+ all slots are vacant, clear local slowdown as well, and assert
+ DR_CONTROL is 0.
+
2014-06-05 Joel Brobecker <brobecker@adacore.com>
* development.sh: Delete.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b4c4ff8..439e650 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-23 Pedro Alves <palves@redhat.com>
+
+ * gdb.base/watchpoint-reuse-slot.c: New file.
+ * gdb.base/watchpoint-reuse-slot.exp: New file.
+
2014-06-23 Jan Kratochvil <jan.kratochvil@redhat.com>
* gdb.arch/amd64-stap-special-operands.exp: Use is_lp64_target.
diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
index 06199af..1bf1b56 100644
--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -415,6 +415,11 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
Ensure DR_CONTROL gets written as the very last register here. */
+ /* Clear DR_CONTROL first. In some cases, setting DR0-3 to a
+ value that doesn't match what is enabled in DR_CONTROL
+ results in EINVAL. */
+ amd64_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
if (state->dr_ref_count[i] > 0)
{
@@ -427,7 +432,10 @@ amd64_linux_prepare_to_resume (struct lwp_info *lwp)
clear_status = 1;
}
- amd64_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
+ /* If DR_CONTROL is supposed to be zero, we've already set it
+ above. */
+ if (state->dr_control_mirror != 0)
+ amd64_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
lwp->arch_private->debug_registers_changed = 0;
}
diff --git a/gdb/gdbserver/i386-low.c b/gdb/gdbserver/i386-low.c
index de2d11a..83ab718 100644
--- a/gdb/gdbserver/i386-low.c
+++ b/gdb/gdbserver/i386-low.c
@@ -313,6 +313,7 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
CORE_ADDR addr, unsigned len_rw_bits)
{
int i, retval = -1;
+ int all_vacant = 1;
ALL_DEBUG_REGISTERS (i)
{
@@ -325,11 +326,30 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
/* Reset our mirror. */
state->dr_mirror[i] = 0;
I386_DR_DISABLE (state, i);
+ /* Even though not strictly necessary, clear out all
+ bits in DR_CONTROL related to this debug register.
+ Debug output is clearer when we don't have stale bits
+ in place. This also allows the assertion below. */
+ I386_DR_SET_RW_LEN (state, i, 0);
}
retval = 0;
}
+
+ if (!I386_DR_VACANT (state, i))
+ all_vacant = 0;
}
+ if (all_vacant)
+ {
+ /* Even though not strictly necessary, clear out all of
+ DR_CONTROL, so that when we have no debug registers in use,
+ we end up with DR_CONTROL == 0. The Linux support relies on
+ this for an optimization. Plus, it makes for clearer debug
+ output. */
+ state->dr_control_mirror &= ~DR_LOCAL_SLOWDOWN;
+
+ gdb_assert (state->dr_control_mirror == 0);
+ }
return retval;
}
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index e478394..066eafe 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -777,6 +777,8 @@ x86_linux_prepare_to_resume (struct lwp_info *lwp)
struct i386_debug_reg_state *state
= &proc->private->arch_private->debug_reg_state;
+ x86_linux_dr_set (ptid, DR_CONTROL, 0);
+
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
if (state->dr_ref_count[i] > 0)
{
@@ -789,7 +791,8 @@ x86_linux_prepare_to_resume (struct lwp_info *lwp)
clear_status = 1;
}
- x86_linux_dr_set (ptid, DR_CONTROL, state->dr_control_mirror);
+ if (state->dr_control_mirror != 0)
+ x86_linux_dr_set (ptid, DR_CONTROL, state->dr_control_mirror);
lwp->arch_private->debug_registers_changed = 0;
}
diff --git a/gdb/i386-linux-nat.c b/gdb/i386-linux-nat.c
index bb2b911..a8d81fb 100644
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -778,6 +778,8 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
/* See amd64_linux_prepare_to_resume for Linux kernel note on
i386_linux_dr_set calls ordering. */
+ i386_linux_dr_set (lwp->ptid, DR_CONTROL, 0);
+
for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
if (state->dr_ref_count[i] > 0)
{
@@ -790,7 +792,8 @@ i386_linux_prepare_to_resume (struct lwp_info *lwp)
clear_status = 1;
}
- i386_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
+ if (state->dr_control_mirror != 0)
+ i386_linux_dr_set (lwp->ptid, DR_CONTROL, state->dr_control_mirror);
lwp->arch_private->debug_registers_changed = 0;
}
diff --git a/gdb/i386-nat.c b/gdb/i386-nat.c
index 2f80a6e..d2a038f 100644
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -472,6 +472,7 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
CORE_ADDR addr, unsigned len_rw_bits)
{
int i, retval = -1;
+ int all_vacant = 1;
ALL_DEBUG_REGISTERS(i)
{
@@ -484,11 +485,30 @@ i386_remove_aligned_watchpoint (struct i386_debug_reg_state *state,
/* Reset our mirror. */
state->dr_mirror[i] = 0;
I386_DR_DISABLE (state, i);
+ /* Even though not strictly necessary, clear out all
+ bits in DR_CONTROL related to this debug register.
+ Debug output is clearer when we don't have stale bits
+ in place. This also allows the assertion below. */
+ I386_DR_SET_RW_LEN (state, i, 0);
}
retval = 0;
}
+
+ if (!I386_DR_VACANT (state, i))
+ all_vacant = 0;
}
+ if (all_vacant)
+ {
+ /* Even though not strictly necessary, clear out all of
+ DR_CONTROL, so that when we have no debug registers in use,
+ we end up with DR_CONTROL == 0. The Linux support relies on
+ this for an optimization. Plus, it makes for clearer debug
+ output. */
+ state->dr_control_mirror &= ~DR_LOCAL_SLOWDOWN;
+
+ gdb_assert (state->dr_control_mirror == 0);
+ }
return retval;
}
diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
new file mode 100644
index 0000000..a5a8f5c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.c
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 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/>. */
+
+union aligned_buf
+{
+ char byte[12];
+
+ /* So that testing consistently starts on an aligned address. */
+ unsigned long long force_align;
+};
+
+union aligned_buf buf;
+
+int
+main (void)
+{
+ volatile int i = 0;
+
+ /* Must be a single line. */
+ for (i = 0; i < 100000; i++); /* stepi line */
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
new file mode 100644
index 0000000..aa30398
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-reuse-slot.exp
@@ -0,0 +1,187 @@
+# Copyright 2014 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/>.
+
+# Test alternating between watchpoint types, watching a sliding window
+# of addresses (thus alternating between aligned and unaligned
+# addresses). Only a single watchpoint exists at any given time. On
+# targets that only update the debug registers on resume, this
+# stresses the debug register setup code, both in GDB and in the
+# target/kernel as one watchpoint replaces the other in a single
+# operation. (Note that we don't have any of these watchpoints
+# trigger.)
+
+if [target_info exists gdb,no_hardware_watchpoints] {
+ unsupported "no target support"
+ return
+}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+ return -1
+}
+
+if ![runto_main] then {
+ fail "Can't run to main"
+ return 0
+}
+
+# The line we'll be stepping.
+set srcline [gdb_get_line_number "stepi line"]
+
+# The address the program is stopped at currently.
+set cur_addr ""
+
+# Get the current PC.
+
+proc get_pc {} {
+ global hex gdb_prompt
+
+ set addr ""
+ set test "get PC"
+ gdb_test_multiple "p /x \$pc" "$test" {
+ -re " = ($hex).*$gdb_prompt $" {
+ set addr $expect_out(1,string)
+ pass "$test"
+ }
+ }
+
+ return $addr
+}
+
+
+# Issue a stepi, and make sure the program advanced past the current
+# instruction (stored in the CUR_ADDR global).
+
+proc stepi {} {
+ global hex gdb_prompt cur_addr
+
+ set srcline " for (i = 0; i < 100000; i++); /* stepi line */"
+ set test "stepi advanced"
+ gdb_test_multiple "stepi" $test {
+ -re "($hex).*[string_to_regexp $srcline]\r\n$gdb_prompt $" {
+ set addr $expect_out(1,string)
+ if {$addr != $cur_addr} {
+ pass $test
+ } else {
+ fail $test
+ }
+ set cur_addr addr
+ }
+ }
+}
+
+gdb_breakpoint $srcline
+gdb_continue_to_breakpoint "stepi line"
+
+# The test tries various sequences of different types of watchpoints.
+# Probe for support first.
+
+# So we get an immediate warning/error if the target doesn't support a
+# given watchpoint type.
+gdb_test_no_output "set breakpoint always-inserted on"
+
+# The list of supported commands. Below we'll probe for support and
+# add elements to this list.
+set cmds {}
+
+foreach cmd {"watch" "awatch" "rwatch"} {
+ set test $cmd
+ gdb_test_multiple "$cmd buf.byte\[0\]" $test {
+ -re "You may have requested too many.*$gdb_prompt $" {
+ unsupported $test
+ }
+ -re "$gdb_prompt $" {
+ pass $test
+ lappend cmds $cmd
+ }
+ }
+
+ delete_breakpoints
+}
+
+set test "hbreak"
+gdb_test_multiple "hbreak main" $test {
+ -re "You may have requested too many.*$gdb_prompt $" {
+ pass $test
+ }
+ -re "$gdb_prompt $" {
+ pass $test
+ lappend cmds "hbreak"
+ }
+}
+
+delete_breakpoints
+
+set cur_addr [get_pc]
+
+# Watch WIDTH bytes at BASE + OFFSET. CMD specifices the specific
+# type of watchpoint to use. If CMD is "hbreak", WIDTH is ignored.
+
+proc watch_command {cmd base offset width} {
+ global srcfile srcline hex
+
+ if {$cmd == "hbreak"} {
+ set expr "*(buf.byte + $base + $offset)"
+ gdb_test "hbreak $expr" "Hardware assisted breakpoint \[0-9\]+ at $hex"
+ } elseif {$cmd == "watch"} {
+ set expr "*(buf.byte + $base + $offset)@$width"
+ gdb_test "$cmd $expr" \
+ "Hardware watchpoint \[0-9\]+: [string_to_regexp $expr]"
+ } elseif {$cmd == "awatch"} {
+ set expr "*(buf.byte + $base + $offset)@$width"
+ gdb_test "$cmd $expr" \
+ "Hardware access \\(read/write\\) watchpoint \[0-9\]+: [string_to_regexp $expr]"
+ } elseif {$cmd == "rwatch"} {
+ set expr "*(buf.byte + $base + $offset)@$width"
+ gdb_test "$cmd $expr" \
+ "Hardware read watchpoint \[0-9\]+: [string_to_regexp $expr]"
+ }
+}
+
+# Run test proper. See intro for description.
+
+foreach always_inserted {"off" "on" } {
+ gdb_test_no_output "set breakpoint always-inserted $always_inserted"
+ foreach cmd1 $cmds {
+ foreach cmd2 $cmds {
+ for {set width 1} {$width < 4} {incr width} {
+
+ if {$cmd1 == "hbreak" && $cmd2 == "hbreak" && $width > 1} {
+ # hbreak ignores WIDTH, no use testing more than
+ # once.
+ continue
+ }
+
+ for {set x 0} {$x < 4} {incr x} {
+ set prefix "always-inserted $always_inserted: "
+ append prefix "$cmd1 x $cmd2: "
+ with_test_prefix "$prefix: width $width, iter $x" {
+ with_test_prefix "base + 0" {
+ watch_command $cmd1 $x 0 $width
+ stepi
+ gdb_test_no_output "delete \$bpnum"
+ }
+ with_test_prefix "base + 1" {
+ watch_command $cmd2 $x 1 $width
+ stepi
+ gdb_test_no_output "delete \$bpnum"
+ }
+ }
+ }
+ }
+ }
+ }
+}
--
1.9.3
--
Pedro Alves
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2014-06-23 17:44 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-07 1:10 [PATCH v3 0/5] Fix lost events, and handle multiple step-overs Pedro Alves
2014-03-07 1:10 ` [PATCH v3 4/5] Handle " Pedro Alves
2014-03-07 1:10 ` [PATCH v3 1/5] Fix missing breakpoint/watchpoint hits, eliminate deferred_step_ptid Pedro Alves
2014-03-07 1:10 ` [PATCH v3 5/5] Make signal-while-stepping-over-bp-other-thread.exp run against remote targets too Pedro Alves
2014-03-07 1:10 ` [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set Pedro Alves
2014-03-16 3:41 ` Doug Evans
2014-03-17 17:11 ` Pedro Alves
2014-03-19 16:52 ` Doug Evans
2014-03-20 13:58 ` Pedro Alves
2014-03-20 13:57 ` Pedro Alves
2014-06-17 19:18 ` Regression for watchpoint-fork.exp [Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set] Jan Kratochvil
2014-06-18 10:43 ` Pedro Alves
2014-06-19 13:43 ` Jan Kratochvil
2014-06-19 15:02 ` Pedro Alves
2014-06-19 16:56 ` Pedro Alves
2014-06-19 17:00 ` Jan Kratochvil
2014-06-20 16:53 ` [PATCH] x86 Linux watchpoints: Couldn't write debug register: Invalid, argument Pedro Alves
2014-06-20 17:45 ` Tom Tromey
2014-06-20 17:52 ` Pedro Alves
2014-06-20 17:53 ` Tom Tromey
2014-06-22 18:31 ` Jan Kratochvil
2014-06-23 17:41 ` Pedro Alves
2014-06-23 17:44 ` Pedro Alves
2014-03-07 1:10 ` [PATCH v3 3/5] Fix for even more missed events; eliminate thread-hop code Pedro Alves
2014-03-20 13:59 ` [PUSHED] Re: [PATCH v3 0/5] Fix lost events, and handle multiple step-overs 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).