This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [PATCH/RFC] Improve the "thread_step_needed" logic
- To: gdb-patches at sources dot redhat dot com
- Subject: Re: [PATCH/RFC] Improve the "thread_step_needed" logic
- From: Michael Snyder <msnyder at cygnus dot com>
- Date: Mon, 25 Jun 2001 17:26:36 -0700
- Organization: Red Hat
- References: <200106152335.f5FNZ9S02028@mvstp600e.cygnus.com>
Committed.
Michael Snyder wrote:
>
> 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 ----