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

[Mark Wielaard <mark@klomp.org>]

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