This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
[PATCH/RFC] Improve the "thread_step_needed" logic
- To: gdb-patches at sources dot redhat dot com
- Subject: [PATCH/RFC] Improve the "thread_step_needed" logic
- From: Michael Snyder <msnyder at mvstp600e dot cygnus dot com>
- Date: Fri, 15 Jun 2001 16:35:09 -0700
This is a fairly significant change, that both simplifies and
improves the logic for deciding when a single thread should be
stepped (to get past a breakpoint), rather than stepping all threads.
In a nutshell, this replaces the state variable "thread_step_needed"
with the following logic in resume():
resume_ptid = RESUME_ALL; /* Default */
if ((step || singlestep_breakpoints_inserted_p) &&
!breakpoints_inserted && breakpoint_here_p (read_pc ()))
{
/* Stepping past a breakpoint without inserting breakpoints.
Make sure only the current thread gets to step, so that
other threads don't sneak past breakpoints while they are
not inserted. */
resume_ptid = inferior_ptid;
}
I have run the thread testsuites on Linux, and done rather extensive
hand-testing. The behavior is clearly improved by this change --
it eliminates instances where threads are allowed to run away because
they are resumed while breakpoints are not inserted.
I'll wait a week or two for people to yell before committing this.
2001-06-15 Michael Snyder <msnyder@redhat.com>
* infrun.c: Eliminate the "thread_step_needed" state variable,
and replace it with a relatively simple test in resume.
(resume): Replace thread_step_needed logic with a test for
stepping, breakpoint_here_p and breakpoints_inserted.
Move CANNOT_STEP_BREAKPOINT logic to after thread_step logic.
(proceed): Discard thread_step_needed logic.
(wait_for_inferior, fetch_inferior_event, handle_inferior_event):
Discard thread_step_needed logic.
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.37
diff -c -3 -p -r1.37 infrun.c
*** infrun.c 2001/06/15 22:44:20 1.37
--- infrun.c 2001/06/15 23:23:48
*************** static ptid_t previous_inferior_ptid;
*** 110,140 ****
static int may_follow_exec = MAY_FOLLOW_EXEC;
- /* resume and wait_for_inferior use this to ensure that when
- stepping over a hit breakpoint in a threaded application
- only the thread that hit the breakpoint is stepped and the
- other threads don't continue. This prevents having another
- thread run past the breakpoint while it is temporarily
- removed.
-
- This is not thread-specific, so it isn't saved as part of
- the infrun state.
-
- Versions of gdb which don't use the "step == this thread steps
- and others continue" model but instead use the "step == this
- thread steps and others wait" shouldn't do this. */
-
- static int thread_step_needed = 0;
-
- /* This is true if thread_step_needed should actually be used. At
- present this is only true for HP-UX native. */
-
- #ifndef USE_THREAD_STEP_NEEDED
- #define USE_THREAD_STEP_NEEDED (0)
- #endif
-
- static int use_thread_step_needed = USE_THREAD_STEP_NEEDED;
-
/* GET_LONGJMP_TARGET returns the PC at which longjmp() will resume the
program. It needs to examine the jmp_buf argument and extract the PC
from it. The return value is non-zero on success, zero otherwise. */
--- 110,115 ----
*************** resume (int step, enum target_signal sig
*** 821,834 ****
struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
QUIT;
! #ifdef CANNOT_STEP_BREAKPOINT
! /* Most targets can step a breakpoint instruction, thus executing it
! normally. But if this one cannot, just continue and we will hit
! it anyway. */
! if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
! step = 0;
! #endif
/* Some targets (e.g. Solaris x86) have a kernel bug when stepping
over an instruction that causes a page fault without triggering
a hardware watchpoint. The kernel properly notices that it shouldn't
--- 796,804 ----
struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
QUIT;
! /* FIXME: calling breakpoint_here_p (read_pc ()) three times! */
+
/* Some targets (e.g. Solaris x86) have a kernel bug when stepping
over an instruction that causes a page fault without triggering
a hardware watchpoint. The kernel properly notices that it shouldn't
*************** resume (int step, enum target_signal sig
*** 909,950 ****
if (should_resume)
{
ptid_t resume_ptid;
! if (use_thread_step_needed && thread_step_needed)
{
! /* We stopped on a BPT instruction;
! don't continue other threads and
! just step this thread. */
! thread_step_needed = 0;
! if (!breakpoint_here_p (read_pc ()))
! {
! /* Breakpoint deleted: ok to do regular resume
! where all the threads either step or continue. */
! resume_ptid = RESUME_ALL;
! }
! else
! {
! if (!step)
! {
! warning ("Internal error, changing continue to step.");
! remove_breakpoints ();
! breakpoints_inserted = 0;
! trap_expected = 1;
! step = 1;
! }
! resume_ptid = inferior_ptid;
! }
}
! else
{
! /* Vanilla resume. */
! if ((scheduler_mode == schedlock_on) ||
! (scheduler_mode == schedlock_step && step != 0))
resume_ptid = inferior_ptid;
- else
- resume_ptid = RESUME_ALL;
}
target_resume (resume_ptid, step, sig);
}
--- 879,913 ----
if (should_resume)
{
ptid_t resume_ptid;
+
+ resume_ptid = RESUME_ALL; /* Default */
! if ((step || singlestep_breakpoints_inserted_p) &&
! !breakpoints_inserted && breakpoint_here_p (read_pc ()))
{
! /* Stepping past a breakpoint without inserting breakpoints.
! Make sure only the current thread gets to step, so that
! other threads don't sneak past breakpoints while they are
! not inserted. */
! resume_ptid = inferior_ptid;
}
!
! if ((scheduler_mode == schedlock_on) ||
! (scheduler_mode == schedlock_step &&
! (step || singlestep_breakpoints_inserted_p)))
{
! /* User-settable 'scheduler' mode requires solo thread resume. */
resume_ptid = inferior_ptid;
}
+
+ #ifdef CANNOT_STEP_BREAKPOINT
+ /* Most targets can step a breakpoint instruction, thus executing it
+ normally. But if this one cannot, just continue and we will hit
+ it anyway. */
+ if (step && breakpoints_inserted && breakpoint_here_p (read_pc ()))
+ step = 0;
+ #endif
target_resume (resume_ptid, step, sig);
}
*************** proceed (CORE_ADDR addr, enum target_sig
*** 1019,1034 ****
else
{
write_pc (addr);
-
- /* New address; we don't need to single-step a thread
- over a breakpoint we just hit, 'cause we aren't
- continuing from there.
-
- It's not worth worrying about the case where a user
- asks for a "jump" at the current PC--if they get the
- hiccup of re-hiting a hit breakpoint, what else do
- they expect? */
- thread_step_needed = 0;
}
#ifdef PREPARE_TO_PROCEED
--- 982,987 ----
*************** proceed (CORE_ADDR addr, enum target_sig
*** 1046,1052 ****
if (PREPARE_TO_PROCEED (1) && breakpoint_here_p (read_pc ()))
{
oneproc = 1;
- thread_step_needed = 1;
}
#endif /* PREPARE_TO_PROCEED */
--- 999,1004 ----
*************** wait_for_inferior (void)
*** 1287,1294 ****
/* Fill in with reasonable starting values. */
init_execution_control_state (ecs);
- thread_step_needed = 0;
-
/* We'll update this if & when we switch to a new thread. */
previous_inferior_ptid = inferior_ptid;
--- 1239,1244 ----
*************** fetch_inferior_event (void *client_data)
*** 1347,1354 ****
/* Fill in with reasonable starting values. */
init_execution_control_state (async_ecs);
- thread_step_needed = 0;
-
/* We'll update this if & when we switch to a new thread. */
previous_inferior_ptid = inferior_ptid;
--- 1297,1302 ----
*************** handle_inferior_event (struct execution_
*** 1498,1508 ****
/* Fall thru to the normal_state case. */
case infwait_normal_state:
- /* Since we've done a wait, we have a new event. Don't
- carry over any expectations about needing to step over a
- breakpoint. */
- thread_step_needed = 0;
-
/* See comments where a TARGET_WAITKIND_SYSCALL_RETURN event
is serviced in this loop, below. */
if (ecs->enable_hw_watchpoints_after_wait)
--- 1446,1451 ----
*************** handle_inferior_event (struct execution_
*** 1934,1963 ****
context_switch (ecs);
ecs->waiton_ptid = ecs->ptid;
ecs->wp = &(ecs->ws);
- thread_step_needed = 1;
ecs->another_trap = 1;
- /* keep_stepping will call resume, and the
- combination of "thread_step_needed" and
- "ecs->another_trap" will cause resume to
- solo-step the thread. The subsequent trap
- event will be handled like any other singlestep
- event. */
-
ecs->infwait_state = infwait_thread_hop_state;
keep_going (ecs);
registers_changed ();
return;
}
}
- else
- {
- /* This breakpoint matches--either it is the right
- thread or it's a generic breakpoint for all threads.
- Remember that we'll need to step just _this_ thread
- on any following user continuation! */
- thread_step_needed = 1;
- }
}
}
else
--- 1877,1890 ----
*************** handle_inferior_event (struct execution_
*** 2413,2419 ****
case BPSTAT_WHAT_SINGLE:
if (breakpoints_inserted)
{
- thread_step_needed = 1;
remove_breakpoints ();
}
breakpoints_inserted = 0;
--- 2340,2345 ----