Re: [RFC] Reinsert the single stepping breakpoint after being hopped over

[Jie Zhang <jzhang918@gmail.com>]

Daniel,

Thanks for your thoughts. I tried your patch. It also works. However,
I think the concept of reinserting the hopped stepping breakpoint is
more intuitive.

Thanks,
Jie


On 8/2/05, Daniel Jacobowitz <drow@false.org> wrote:
> On Tue, Aug 02, 2005 at 11:11:45PM +0800, Jie Zhang wrote:
> > Hi,
> >
> > I'm working on a new port of GDB for Blackfin. It's usually used for
> > remote debugging using gdbserver and software single stepping. It has
> > many FAILs for schedlock.exp. I traced these FAILs and, I think, found
> > a bug in GDB.
> >
> > Assume there are two threads in the program and we are stepping on
> > one. (I use GDB for the whole of GDB and gdbserver.)
> >
> > 1. GDB inserts a breakpoint for stepping and resumes both threads.
> > 2. Both threads hit this breakpoint.
> > 3. The first stopped thread GDB looks for (for some reason) is not the
> > thread we are stepping. GDB make it hop over the stepping breakpoint.
> > 4. GDB resume both threads by
> >
> >     resume (1, TARGET_SIGNAL_0);
> >
> > So the thread we are stepping will do a new step and the previous stop
> > of step will never be noticed by GDB. (It's caught by gdbserver and
> > saved as a pending status. However, when it stops again for the new
> > step, the previous stepping breakpoint has been removed, so
> > check_removed_breakpoint () will clear status_pending_p and the
> > previous stop is lost.) GDB will never get a chance to check if the
> > stepping is out of the range. The step command will not return.
> 
> Yes... I fixed this once already, but I failed to consider the case
> where both threads have hit the singlestep breakpoint, rather than
> another thread hitting the breakpoint before the original thread has
> had a chance to step.
> 
> I've fixed the version you encountered before, but it appears I never
> submitted the patch.  I'm not sure if it still applies, but could you
> give this a try?
> 
> > To fix it, GDB should put the previous stepping breakpoint back, not
> > do a new stepping when resuming both threads. The following patch is
> > trying to fix this. It adds a new field "singlestep_breakpoint_addr"
> > in "struct thread_info" to remember the last single stepping
> > breakpoint address. Passing 2 as the second argument to
> > SOFTWARE_SINGLE_STEP () to tell the target software single stepping
> > function that we just reinsert the previous single stepping
> > breakpoint.
> 
> Interesting approach.  I don't like the implementation - I'd rather not
> extend context_switch - but the concept may be better than mine.  Let
> me think about this for a little.
> 
> --
> Daniel Jacobowitz
> CodeSourcery, LLC
> 
> Index: infrun.c
> ===================================================================
> RCS file: /big/fsf/rsync/src-cvs/src/gdb/infrun.c,v
> retrieving revision 1.137
> diff -u -p -r1.137 infrun.c
> --- infrun.c    16 Feb 2004 20:49:51 -0000      1.137
> +++ infrun.c    6 Mar 2004 04:23:06 -0000
> @@ -479,6 +479,9 @@ static int singlestep_breakpoints_insert
>  /* The thread we inserted single-step breakpoints for.  */
>  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;
> @@ -570,6 +573,7 @@ resume (int step, enum target_signal sig
>           `wait_for_inferior' */
>        singlestep_breakpoints_inserted_p = 1;
>        singlestep_ptid = inferior_ptid;
> +      singlestep_pc = read_pc ();
>      }
> 
>    /* Handle any optimized stores to the inferior NOW...  */
> @@ -1201,6 +1205,7 @@ context_switch (struct execution_control
>                          &ecs->current_line, &ecs->current_symtab, &step_sp);
>      }
>    inferior_ptid = ecs->ptid;
> +  flush_cached_frames ();
>  }
> 
>  /* Wrapper for PC_IN_SIGTRAMP that takes care of the need to find the
> @@ -1803,7 +1808,10 @@ handle_inferior_event (struct execution_
>         }
>        else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
>         {
> +         gdb_assert (ptid_equal (inferior_ptid, singlestep_ptid));
> +
>           ecs->random_signal = 0;
> +
>           /* 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
> @@ -1811,9 +1819,32 @@ handle_inferior_event (struct execution_
>           if (!ptid_equal (singlestep_ptid, ecs->ptid)
>               && in_thread_list (singlestep_ptid))
>             {
> -             thread_hop_needed = 1;
> -             stepping_past_singlestep_breakpoint = 1;
> -             saved_singlestep_ptid = 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 runs a risk of losing signal information
> +                for singlestep_ptid, but prevents us continuously
> +                moving the single-step breakpoint.  This situation
> +                means that the thread has trapped or been signalled,
> +                but the event has not been reported to GDB yet.
> +                Really we should arrange to report all events, or to
> +                re-poll the remote looking for this particular
> +                thread (i.e. temporarily enable schedlock).  */
> +             if (read_pc_pid (singlestep_ptid) != singlestep_pc)
> +               {
> +                 /* 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.  */
> +                 ecs->ptid = singlestep_ptid;
> +                 stop_pc = read_pc_pid (ecs->ptid);
> +               }
> +             else
> +               {
> +                 thread_hop_needed = 1;
> +                 stepping_past_singlestep_breakpoint = 1;
> +                 saved_singlestep_ptid = singlestep_ptid;
> +               }
>             }
>         }
> 
> @@ -1942,8 +1973,6 @@ handle_inferior_event (struct execution_
> 
>        if (context_hook)
>         context_hook (pid_to_thread_id (ecs->ptid));
> -
> -      flush_cached_frames ();
>      }
> 
>    if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
>