This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: [PATCH/RFC] Improve the "thread_step_needed" logic


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 ----


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]