From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Snyder To: gdb-patches@sources.redhat.com Subject: Re: [PATCH/RFC] Improve the "thread_step_needed" logic Date: Mon, 25 Jun 2001 17:26:00 -0000 Message-id: <3B37D6BC.510B1A8C@cygnus.com> References: <200106152335.f5FNZ9S02028@mvstp600e.cygnus.com> X-SW-Source: 2001-06/msg00406.html 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 > > * 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 ----