Re: [SCM] master: Implement hasWatchpointTriggered in abstract, and sub-classes.

Re: [SCM] master: Implement hasWatchpointTriggered in abstract, and sub-classes.

[Phil Muldoon]

pmuldoon@sourceware.org wrote:

This implements a function that checks the debug status register on 
whether a hardware debug data breakpoint (watchpoint) has been 
triggered. Normally this function is called after a SIGTRAP is issued 
from ptrace. On x8664 and IA32 the first four bits represent the four 
hardware breakpoint registers.  If bit  0 = 1 then the debug hardware 
data breakpoint (D0) has triggered, bit 1 for D1 and so on. I believe 
the software is responsible for clearing the bit if set, and I have not 
done this here. I'm torn whether to reset the bit if:

debugStatus & (1L << index)) != 0;

where debug status is the debug status register,  and index is the 
watchpoint to check. Should the bit be reset in hasWatchpointTriggered? 
Or in a separate function function called as a separate API?

Regards

Phil
> The branch, master has been updated
>        via  a0fc4305e0afa24959ac5cd073f65c6bb50eb8b0 (commit)
>       from  b927d6c13dcc27d155895895fcb958b4c32fb595 (commit)
>
> Those revisions listed above that are new to this repository have
> not appeared on any other notification email.
>
> - Log -----------------------------------------------------------------
> commit a0fc4305e0afa24959ac5cd073f65c6bb50eb8b0
> Author: Phil Muldoon <pmuldoon@redhat.com>
> Date:   Tue Apr 1 21:48:19 2008 +0100
>
>     Implement hasWatchpointTriggered in abstract, and sub-classes.
>     
>     2008-04-01  Phil Muldoon  <pmuldoon@redhat.com>
>     
>     	* Watchpoint.java (hasWatchpointTriggered): Define.
>     	* IA32Watchpoint.java (hasWatchpointTriggered): Implement.
>     	* X8664Watchpoint.java (hasWatchpointTriggered): Ditto.
>
> -----------------------------------------------------------------------
>
> Summary of changes:
>  frysk-core/frysk/isa/watchpoints/ChangeLog         |    6 ++++++
>  .../frysk/isa/watchpoints/IA32Watchpoint.java      |   13 ++++++++++++-
>  frysk-core/frysk/isa/watchpoints/Watchpoint.java   |   10 ++++++++++
>  .../frysk/isa/watchpoints/X8664Watchpoint.java     |   12 ++++++++++++
>  4 files changed, 40 insertions(+), 1 deletions(-)
>
> First 500 lines of diff:
> diff --git a/frysk-core/frysk/isa/watchpoints/ChangeLog b/frysk-core/frysk/isa/watchpoints/ChangeLog
> index 0e4f56f..f53652b 100644
> --- a/frysk-core/frysk/isa/watchpoints/ChangeLog
> +++ b/frysk-core/frysk/isa/watchpoints/ChangeLog
> @@ -1,3 +1,9 @@
> +2008-04-01  Phil Muldoon  <pmuldoon@redhat.com>
> +
> +	* Watchpoint.java (hasWatchpointTriggered): Define.
> +	* IA32Watchpoint.java (hasWatchpointTriggered): Implement.
> +	* X8664Watchpoint.java (hasWatchpointTriggered): Ditto.
> +
>  2008-03-28  Phil Muldoon <pmuldoon@redhat.com>
>  
>  	* Watchpoint.java: New. Initial Implementation.
> diff --git a/frysk-core/frysk/isa/watchpoints/IA32Watchpoint.java b/frysk-core/frysk/isa/watchpoints/IA32Watchpoint.java
> index 3627d03..5f97636 100644
> --- a/frysk-core/frysk/isa/watchpoints/IA32Watchpoint.java
> +++ b/frysk-core/frysk/isa/watchpoints/IA32Watchpoint.java
> @@ -197,7 +197,18 @@ class IA32Watchpoint extends Watchpoint {
>  	    task.setRegister(IA32Registers.DEBUG_CONTROL, debugControl);
>      }
>  
> -    
> +    /**
> +     * Reads the Debug Status Register and checks if 
> +     * the breakpoint specified has fired.
> +     *
> +     * @param task - task to read the debug control
> +     * register from.
> +     */
> +    public boolean hasWatchpointTriggered(Task task, int index) {
> +	long debugStatus = task.getRegister(IA32Registers.DEBUG_STATUS);	
> +	return (debugStatus & (1L << index)) != 0;
> +    }
> +
>  
>      /**
>       * Reads the Debug control register.
> diff --git a/frysk-core/frysk/isa/watchpoints/Watchpoint.java b/frysk-core/frysk/isa/watchpoints/Watchpoint.java
> index fc8c234..a0ef202 100644
> --- a/frysk-core/frysk/isa/watchpoints/Watchpoint.java
> +++ b/frysk-core/frysk/isa/watchpoints/Watchpoint.java
> @@ -94,6 +94,16 @@ public abstract class Watchpoint  {
>       */
>      public abstract long readControlRegister(Task task);
>  
> +    
> +    /**
> +     * Reads the Debug Status Register and checks if 
> +     * the breakpoint specified has fired.
> +     *
> +     * @param task - task to read the debug control
> +     * register from.
> +     */
> +    public abstract boolean hasWatchpointTriggered(Task task, int index);
> +
>      /**
>       * Returns number of watchpoints for this architecture
>       *
> diff --git a/frysk-core/frysk/isa/watchpoints/X8664Watchpoint.java b/frysk-core/frysk/isa/watchpoints/X8664Watchpoint.java
> index c581f92..1df98a5 100644
> --- a/frysk-core/frysk/isa/watchpoints/X8664Watchpoint.java
> +++ b/frysk-core/frysk/isa/watchpoints/X8664Watchpoint.java
> @@ -205,5 +205,17 @@ class X8664Watchpoint extends Watchpoint {
>  	return task.getRegister(X8664Registers.DEBUG_CONTROL);
>      }
>  
> +    /**
> +     * Reads the Debug Status Register and checks if 
> +     * the breakpoint specified has fired.
> +     *
> +     * @param task - task to read the debug control
> +     * register from.
> +     */
> +    public boolean hasWatchpointTriggered(Task task, int index) {
> +	long debugStatus = task.getRegister(X8664Registers.DEBUG_STATUS);
> +	return (debugStatus & (1L << index)) != 0;
> +    }
> +
>  
>  }
>
>
> hooks/post-receive
> --
> frysk system monitor/debugger
>   

Re: [SCM] master: Implement hasWatchpointTriggered in abstract, and sub-classes.

[Mark Wielaard]

Hi Phil,

On Tue, 2008-04-01 at 21:58 +0100, Phil Muldoon wrote:
> pmuldoon@sourceware.org wrote:
> 
> This implements a function that checks the debug status register on 
> whether a hardware debug data breakpoint (watchpoint) has been 
> triggered. Normally this function is called after a SIGTRAP is issued 
> from ptrace. On x8664 and IA32 the first four bits represent the four 
> hardware breakpoint registers.  If bit  0 = 1 then the debug hardware 
> data breakpoint (D0) has triggered, bit 1 for D1 and so on. I believe 
> the software is responsible for clearing the bit if set, and I have not 
> done this here. I'm torn whether to reset the bit if:
> 
> debugStatus & (1L << index)) != 0;
>
> where debug status is the debug status register,  and index is the 
> watchpoint to check.

I think this is a little too architecture specific and a little error
prone in usage. Code using this now has to iterate over all the possible
watchpoint indexes for a particular architecture. Maybe this is small
and trivial on all architectures, but it would feel more natural imho if
there was just a method to query which watchpoint index was triggered if
any. Something like:

    /**
     * Reads the Debug Status Register and checks whether 
     * any watchpoint has fired.
     *
     * @param task - task to read the debug control
     * register from.
     * @returns the index of the watchpoint triggered,
     * or -1 if no watchpoint has triggered.
     */
    public abstract int watchpointTriggered(Task task);

With as possible architecture implementation something like:

    public int watchpointTriggered(Task task)
    {
      Register register = IA32Registers.DEBUG_STATUS;
      long debugStatus = task.getRegister(register);
      for (int index = 0; i < noOfWatchpoints; index++)
        if ((debugStatus & (1L << index)) != 0)
          return index;

      return -1;
    }

(Not tested or compiled)

You might actually want to make this a little bit more abstract and
maybe just return the address, range and mode (read/write) that
triggered instead of the index. That would need a little container class
that holds these values. The benefit is that it would abstract away any
architecture specific implementation details (like the index and whether
or not this is done with specific cpu support or not).

>  Should the bit be reset in hasWatchpointTriggered? 
> Or in a separate function function called as a separate API?

A separate method is cleaner. The problem I had with that (in the
stepping flag) is that it then becomes somewhat harder to know when to
clear the status flags. Since the state machine is guaranteed to check
the status before any state transitions it is somewhat easier to make it
a check-and-clear-status method:
http://sourceware.org/ml/frysk/2007-q1/msg00024.html
If you have a separate method you can either call it directly after
checking the status, or in the Running.sendContinue() method. I
introduced this to have one point in the ptrace state machine where a
Task is set running again:
http://sourceware.org/ml/frysk/2007-q2/msg00329.html
We also already discussed basing the stepping flag on this new
infrastructure: http://sourceware.org/ml/frysk/2007-q3/msg00324.html
(And I agreed it would be somewhat cleaner than what is done now.)

Cheers,

Mark

Re: [SCM] master: Implement hasWatchpointTriggered in abstract, and sub-classes.

[Phil Muldoon]

Mark Wielaard wrote:
> debugStatus & (1L << index)) != 0;
>
> where debug status is the debug status register,  and index is the 
> watchpoint to check.
>   
>
> I think this is a little too architecture specific and a little error
> prone in usage. Code using this now has to iterate over all the possible
> watchpoint indexes for a particular architecture. Maybe this is small
> and trivial on all architectures, but it would feel more natural imho if
> there was just a method to query which watchpoint index was triggered if
> any. Something like:
>
> public abstract int watchpointTriggered(Task task);
>   

Originally I did have something like this, but it does not answer the 
scenario of what happens when a sigtrap is delivered that denotes 
multiple watchpoints being triggered? There's no real clean way to do 
this per architecture. Could return an array of .. perhaps booleans 
denoting each register as being triggered, or maybe a list of .. same 
again, what? As there already is a general function implemented:

int getWatchpointCount()

the user can iterate over the watchpoints themselves in a simple loop.  
If you can make watchpointTriggered work so it accounts for multiple 
watchpoints hit, I'll be happy ;)

Regards

Phil

>>  Should the bit be reset in hasWatchpointTriggered? 
>> Or in a separate function function called as a separate API?
>>     
>
> A separate method is cleaner. The problem I had with that (in the
> stepping flag) is that it then becomes somewhat harder to know when to
> clear the status flags. Since the state machine is guaranteed to check
> the status before any state transitions it is somewhat easier to make it
> a check-and-clear-status method:
> http://sourceware.org/ml/frysk/2007-q1/msg00024.html
> If you have a separate method you can either call it directly after
> checking the status, or in the Running.sendContinue() method. I
> introduced this to have one point in the ptrace state machine where a
> Task is set running again:
> http://sourceware.org/ml/frysk/2007-q2/msg00329.html
> We also already discussed basing the stepping flag on this new
> infrastructure: http://sourceware.org/ml/frysk/2007-q3/msg00324.html
> (And I agreed it would be somewhat cleaner than what is done now.)
>
>   

Ok I'll look at what goes on here. The second half of the question is, 
how and by what mechanism do you allow the user to continue a task. By 
default the task is stopped and will stay stopped until such a time that 
the user is done inspecting the environment. How will the user restart 
the task?

Thanks!

Regards

Phil
> Cheers,
>
> Mark
>
>   

Re: [SCM] master: Implement hasWatchpointTriggered in abstract, and sub-classes.

[Mark Wielaard]

Hi Phil,

On Wed, 2008-04-02 at 09:53 +0100, Phil Muldoon wrote:
> Mark Wielaard wrote:
> > I think this is a little too architecture specific and a little error
> > prone in usage. Code using this now has to iterate over all the possible
> > watchpoint indexes for a particular architecture. Maybe this is small
> > and trivial on all architectures, but it would feel more natural imho if
> > there was just a method to query which watchpoint index was triggered if
> > any. Something like:
> >
> > public abstract int watchpointTriggered(Task task);
> 
> Originally I did have something like this, but it does not answer the 
> scenario of what happens when a sigtrap is delivered that denotes 
> multiple watchpoints being triggered? There's no real clean way to do 
> this per architecture. Could return an array of .. perhaps booleans 
> denoting each register as being triggered, or maybe a list of .. same 
> again, what?

If so then I would return a Set of the actual watchpoints (just return
the address, range and mode [read/write]) that triggered instead of the
index (or null of course if no watchpoint was triggered). That would
need a little container class that holds these values. The benefit is
that it would abstract away any architecture specific implementation
details (like the index and whether or not this is done with specific
cpu support or not). And that you can easily store them in a Collection
that you can then return from a method like this.

> the user can iterate over the watchpoints themselves in a simple loop.  
> If you can make watchpointTriggered work so it accounts for multiple 
> watchpoints hit, I'll be happy ;)

Could you show some scenarios where higher level constructs get mapped
onto overlapping watchpoints (which cannot be compressed into
non-overlapping ones) or instructions which can trigger multiple
hardware watchpoints at once? It seems like things become simpler if we
only have to worry about one hardware watchpoint triggering at any given
moment. But maybe there are valid scenarios where multiple watchpoints
trigger at the same time (in that case, do you really care which one?)

> The second half of the question is, 
> how and by what mechanism do you allow the user to continue a task. By 
> default the task is stopped and will stay stopped until such a time that 
> the user is done inspecting the environment. How will the user restart 
> the task?

The user doesn't explicitly restart the task, the task just transforms
into a Running (sub) state that triggers a continueRunning() whenever
all blockers are removed (or no blockers were ever introduced in the
first place).

Cheers,

Mark