Re: [RFA] thread specific breakpoints and single stepping

[Pedro Alves <pedro@codesourcery.com>]

A Tuesday 08 July 2008 03:49:16, Ems SUZUKI wrote:

>
> After the program reached to line 9, I set a breakpoint for only
> thread 1 at line 10.  And I invoked step for thread 2.  I expected
> thread 2 would stop at line 10, but it hopped over there and stopped
> at line 11.  (I've attached the program I used to this mail.)
>
> The cause is in the below:
>
> infrun.c:2117
>
>       if (regular_breakpoint_inserted_here_p (stop_pc))
>         {
>           ecs->random_signal = 0;
>           if (!breakpoint_thread_match (stop_pc, ecs->ptid))
>             thread_hop_needed = 1;
>         }
>
> thread_hop_needed would be set as 1, in the case that the stopped
> thread does not match the condition of the breakpoint.  The thread
> which is currently single-stepping would also go into here.  But the
> stepping thread should not hop over the breakpoints unconditionally,
> but check if it has reached the location where stepping would end.  
>
> I think I've add a check for it with the attached diff.  Does it make
> sense?


>   Index: gdb/infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.284
> diff -u -r1.284 infrun.c
> --- gdb/infrun.c        28 Jun 2008 09:42:15 -0000      1.284
> +++ gdb/infrun.c        8 Jul 2008 02:34:39 -0000
> @@ -2118,7 +2118,11 @@
>         {
>           ecs->random_signal = 0;
>           if (!breakpoint_thread_match (stop_pc, ecs->ptid))
> -           thread_hop_needed = 1;
> +           /* If the thread is currently single-stepping, whether it will
> +              step over the breakpoint or not should be determined later.
> */ +           if (!ptid_equal (ecs->ptid, inferior_ptid)
> +               || !currently_stepping (ecs))
> +             thread_hop_needed = 1;
>         }
>        else if (singlestep_breakpoints_inserted_p)
>         {

I don't think that's correct.  You can't be sure inferior_ptid
is the user stepping thread at this point.  GDB may have context-switched
already due to another event having happened in another thread after
the user started the step, and before reaching here.  E.g., it could
have been that another thread had already thread-hoped this breakpoint,
see a bit below:

      if (thread_hop_needed)
	{
...
	  else
	    {			/* Single step */
	      if (!ptid_equal (inferior_ptid, ecs->ptid))
		context_switch (ecs);
	      ecs->waiton_ptid = ecs->ptid;
	      ecs->wp = &(ecs->ws);
	      ecs->stepping_over_breakpoint = 1;

	      ecs->infwait_state = infwait_thread_hop_state;
	      keep_going (ecs);
	      registers_changed ();
	      return;
	    }
	}

Also, currently_stepping will return true for cases other than
single-stepping due to user request.

I think that what you want is:

      if (regular_breakpoint_inserted_here_p (stop_pc))
	{
	  ecs->random_signal = 0;
	  if (!breakpoint_thread_match (stop_pc, ecs->ptid))
	    {
	      if (!ptid_equal (inferior_ptid, ecs->ptid))
		context_switch (ecs);

	      /* If the thread is currently single-stepping, whether
		 it will step over the this breakpoint or not should be
		 determined later.  */
	      if (!(step_range_end && step_resume_breakpoint == NULL))
		thread_hop_needed = 1;
	    }
	}

bpstat_stop_status will then find that this PC is not to
be stopped (wrong thread), and the single-stepping will continue,
if still within range (just like you suggested).

Does it work for you?  Could you convert your testcase into a
test for GDB's testsuite?  Also could you provide a ChangeLog
entry along with the patch?

Thanks!

-- 
Pedro Alves