[Patch] Use resume instead of target_resume when stepping over watchpoint.

[Patch] Use resume instead of target_resume when stepping over watchpoint.

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

In handle_inferior_event() when stepping over a watch point currently we
issue target_resume().  This only works on architectures that have
hardware single step support.  For gdbarch_software_single_step_p()
systems (like MIPS), we need to insert a single step breakpoint instead.

The fix is to call resume() as it does the right thing already.  I also
added an assert that inferior_ptid == ecs->ptid to be sure that resume()
was stepping the proper thread.

This is essentially the change requested by Daniel in:
http://sourceware.org/ml/gdb-patches/2008-04/msg00443.html

This change is a prerequisite for my forthcoming MIPS hardware watch patch.

Tested on x86_64-pc-linux-gnu as well as mipsel-linux (in conjunction
with the MIPS hardware watch patch).

OK to commit?

2008-09-09  David Daney  <ddaney@avtrex.com>

	* infrun.c (handle_inferior_event): Call resume instead of
	target_resume when stepping over watchpoint.



[-- Attachment #2: infrun.patch --]
[-- Type: text/plain, Size: 715 bytes --]

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.316
diff -u -p -r1.316 infrun.c
--- infrun.c	8 Sep 2008 22:10:20 -0000	1.316
+++ infrun.c	9 Sep 2008 23:37:09 -0000
@@ -2472,7 +2472,8 @@ targets should add new threads to the th
       if (!HAVE_STEPPABLE_WATCHPOINT)
 	remove_breakpoints ();
       registers_changed ();
-      target_resume (ecs->ptid, 1, TARGET_SIGNAL_0);	/* Single step */
+      gdb_assert (ptid_equal (inferior_ptid, ecs->ptid));
+      resume (1, TARGET_SIGNAL_0);	/* Single step */
       waiton_ptid = ecs->ptid;
       if (HAVE_STEPPABLE_WATCHPOINT)
 	infwait_state = infwait_step_watch_state;

Re: [Patch] Use resume instead of target_resume when stepping over watchpoint.

[Joel Brobecker]

Pedro, Others,

What do you think of this patch? Personally, I have pretty much
convinced myself that it shouldn't do any harm, but I really
wished that "resume" would take a ptid as an argument. Except
that this is not trivial to do, and I think that the current
"resume" would need to be split a bit, to remove the code that
determines what to resume.

Anyway, I don't see anything wrong with this patch, but I'd love
for someone to take a look as well. This is a pretty delicate
part of the debugger.  Do we really need the gdb_assert thought?

On Tue, Sep 09, 2008 at 05:31:33PM -0700, David Daney wrote:
> In handle_inferior_event() when stepping over a watch point currently we
> issue target_resume().  This only works on architectures that have
> hardware single step support.  For gdbarch_software_single_step_p()
> systems (like MIPS), we need to insert a single step breakpoint instead.
> 
> The fix is to call resume() as it does the right thing already.  I also
> added an assert that inferior_ptid == ecs->ptid to be sure that resume()
> was stepping the proper thread.
> 
> This is essentially the change requested by Daniel in:
> http://sourceware.org/ml/gdb-patches/2008-04/msg00443.html
> 
> This change is a prerequisite for my forthcoming MIPS hardware watch patch.
> 
> Tested on x86_64-pc-linux-gnu as well as mipsel-linux (in conjunction
> with the MIPS hardware watch patch).
> 
> OK to commit?
> 
> 2008-09-09  David Daney  <ddaney@avtrex.com>
> 
> 	* infrun.c (handle_inferior_event): Call resume instead of
> 	target_resume when stepping over watchpoint.
> 
> 

> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.316
> diff -u -p -r1.316 infrun.c
> --- infrun.c	8 Sep 2008 22:10:20 -0000	1.316
> +++ infrun.c	9 Sep 2008 23:37:09 -0000
> @@ -2472,7 +2472,8 @@ targets should add new threads to the th
>        if (!HAVE_STEPPABLE_WATCHPOINT)
>  	remove_breakpoints ();
>        registers_changed ();
> -      target_resume (ecs->ptid, 1, TARGET_SIGNAL_0);	/* Single step */
> +      gdb_assert (ptid_equal (inferior_ptid, ecs->ptid));
> +      resume (1, TARGET_SIGNAL_0);	/* Single step */
>        waiton_ptid = ecs->ptid;
>        if (HAVE_STEPPABLE_WATCHPOINT)
>  	infwait_state = infwait_step_watch_state;


-- 
Joel

Re: [Patch] Use resume instead of target_resume when stepping over watchpoint.

[Pedro Alves]

On Thursday 30 October 2008 03:08:05, Joel Brobecker wrote:
> Pedro, Others,
> 
> What do you think of this patch? Personally, I have pretty much
> convinced myself that it shouldn't do any harm, but I really
> wished that "resume" would take a ptid as an argument. Except
> that this is not trivial to do, and I think that the current
> "resume" would need to be split a bit, to remove the code that
> determines what to resume.

Won't this slightly change the behaviour on hardware single-step
archs?  Before, we'd always tell the target to resume a single-thread
(keeping the others stopped, on target that support scheduler locking);
with this change, I think you'll tell the target to resume all
threads.

Urglhs, infrun could use a facelift.  The natural function
to call would be keep_going instead of resume/prepare_to_wait,
but keep_going doesn't know a think about watchpoints...
Would setting ecs->event_thread->trap_expected = 1 in addition
to switching to resume so we trigger this:

resume:
     if ((step || singlestep_breakpoints_inserted_p)
	  && tp->trap_expected)
	{
...
	  resume_ptid = inferior_ptid;
        }

be too ugly?  Hmmm, maybe not OK, it can have other side
effects, like tripping on this...

 if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP
      && ecs->event_thread->trap_expected
      && gdbarch_single_step_through_delay_p (current_gdbarch)
      && currently_stepping (ecs->event_thread))
    {

Can you confirm what I think I'm seeing?

> 
> Anyway, I don't see anything wrong with this patch, but I'd love
> for someone to take a look as well. This is a pretty delicate
> part of the debugger.  Do we really need the gdb_assert thought?
> 
> On Tue, Sep 09, 2008 at 05:31:33PM -0700, David Daney wrote:
> > In handle_inferior_event() when stepping over a watch point currently we
> > issue target_resume().  This only works on architectures that have
> > hardware single step support.  For gdbarch_software_single_step_p()
> > systems (like MIPS), we need to insert a single step breakpoint instead.
> > 
> > The fix is to call resume() as it does the right thing already.  I also
> > added an assert that inferior_ptid == ecs->ptid to be sure that resume()
> > was stepping the proper thread.
> > 
> > This is essentially the change requested by Daniel in:
> > http://sourceware.org/ml/gdb-patches/2008-04/msg00443.html
> > 
> > This change is a prerequisite for my forthcoming MIPS hardware watch patch.
> > 
> > Tested on x86_64-pc-linux-gnu as well as mipsel-linux (in conjunction
> > with the MIPS hardware watch patch).
> > 
> > OK to commit?
> > 
> > 2008-09-09  David Daney  <ddaney@avtrex.com>
> > 
> > 	* infrun.c (handle_inferior_event): Call resume instead of
> > 	target_resume when stepping over watchpoint.
> > 
> > 
> 
> > Index: infrun.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/infrun.c,v
> > retrieving revision 1.316
> > diff -u -p -r1.316 infrun.c
> > --- infrun.c	8 Sep 2008 22:10:20 -0000	1.316
> > +++ infrun.c	9 Sep 2008 23:37:09 -0000
> > @@ -2472,7 +2472,8 @@ targets should add new threads to the th
> >        if (!HAVE_STEPPABLE_WATCHPOINT)
> >  	remove_breakpoints ();
> >        registers_changed ();
> > -      target_resume (ecs->ptid, 1, TARGET_SIGNAL_0);	/* Single step */
> > +      gdb_assert (ptid_equal (inferior_ptid, ecs->ptid));
> > +      resume (1, TARGET_SIGNAL_0);	/* Single step */
> >        waiton_ptid = ecs->ptid;
> >        if (HAVE_STEPPABLE_WATCHPOINT)
> >  	infwait_state = infwait_step_watch_state;
> 
> 



-- 
Pedro Alves

Re: [Patch] Use resume instead of target_resume when stepping over watchpoint.

[Joel Brobecker]

[Dave Daney removed from the list, since his email address is no longer
valid. Is it worth pursuing this thread?]

> Won't this slightly change the behaviour on hardware single-step
> archs?  Before, we'd always tell the target to resume a single-thread
> (keeping the others stopped, on target that support scheduler locking);
> with this change, I think you'll tell the target to resume all
> threads.

Hmmm, yes, you're probably right. I we were to set trap_expected,
then we would only step that one thread, but would that be cheating?

> be too ugly?  Hmmm, maybe not OK, it can have other side
> effects, like tripping on this...
> 
>  if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP
>       && ecs->event_thread->trap_expected
>       && gdbarch_single_step_through_delay_p (current_gdbarch)
>       && currently_stepping (ecs->event_thread))

I guess we might, if we hit the watchpoint while stepping. But then,
trap_expected would have already been set, and thus we'd be OK without
setting it anyways.

Regardless, I think this just shows that calling resume like this is
just too ugly. Perhaps a better approach would be to extract out the
part that handles software_single_step out of resume and have both
resume & handle_inferior_event call that new function?

-- 
Joel

Re: [Patch] Use resume instead of target_resume when stepping over watchpoint.

[David Daney]

Joel Brobecker wrote:
> [Dave Daney removed from the list, since his email address is no longer
> valid. Is it worth pursuing this thread?]
> 

It turns out that I am now subscribed under my new e-mail address.  But 
thanks to Tom Tromey for pinging me on this message.

>> Won't this slightly change the behaviour on hardware single-step
>> archs?  Before, we'd always tell the target to resume a single-thread
>> (keeping the others stopped, on target that support scheduler locking);
>> with this change, I think you'll tell the target to resume all
>> threads.
> 
> Hmmm, yes, you're probably right. I we were to set trap_expected,
> then we would only step that one thread, but would that be cheating?
> 
>> be too ugly?  Hmmm, maybe not OK, it can have other side
>> effects, like tripping on this...
>>
>>  if (ecs->event_thread->stop_signal == TARGET_SIGNAL_TRAP
>>       && ecs->event_thread->trap_expected
>>       && gdbarch_single_step_through_delay_p (current_gdbarch)
>>       && currently_stepping (ecs->event_thread))
> 
> I guess we might, if we hit the watchpoint while stepping. But then,
> trap_expected would have already been set, and thus we'd be OK without
> setting it anyways.
> 
> Regardless, I think this just shows that calling resume like this is
> just too ugly. Perhaps a better approach would be to extract out the
> part that handles software_single_step out of resume and have both
> resume & handle_inferior_event call that new function?
> 

I agree, and in the first version of the patch I hacked something 
together that kind of did it that way, but it was not very clean.

Now that the MIPS hardware watch register support is merged into the 
Linux kernel, my plans are to pursue this patch set further. 
Unfortunately I am waiting for action out of the FSF copyright clerk so 
that I can update my employer disclaimer.

The patch is not forgotten.  Previously I was working on it to make my 
own life a litter easier, but now I have an actual mandate to work on it.

David Daney