Re: [RFC] GDB patches for hw watchpoints - revised

[Wu Zhou <woodzltc@cn.ibm.com>]

On Tue, 13 Dec 2005, Daniel Jacobowitz wrote:

> On Tue, Dec 13, 2005 at 02:10:03PM +0800, Wu Zhou wrote:
> > 2. The second one need Anton's patch, which changed three lines in 
> > arch/ppc64/mm/fault.c:
> 
> > With this patch, I can use PTRACE_GETSIGINFO to get the stopped data 
> > address (it is in siginfo.si_addr).  But one problem is that 
> > to_stopped_by_watchpoint will call PTRACE_GETSIGINFO first to determine if 
> > the stop is caused by watchpoint.  And another problem is that gdb need to 
> > single step the process to execute current instruction when a watchpoint 
> > is hit.  This will again drop into bpstat_stop_status, which will call 
> > stopped_by_watchpoint and thus call PTRACE_GETSIGINFO again.  
> > 
> > I take a look at IA64's code, it set the dd bit of IA64_PSR_REGNUM, which
> > will disable the watchpoint for the next instruction.  But it seems that 
> > ppc don't have such a way.  Do we have any workaround for this?  
> 
> Could you cache the stopped data address when we are stopped by a
> watchpoint, and then return the cached one if we aren't stopped by a
> watchpoint any more but were the previous instruction?

That is mostly the same as what I am doing with this method.  But what do 
you means by "were the previous instruction"?  Do you means to add another 
check on the instruction address when restoring the stopped_data_address?

The related code is like this:

int
ppc_linux_stopped_by_watchpoint (void)
{
  int tid;
  struct siginfo siginfo;
  ptid_t ptid = inferior_ptid;
  CORE_ADDR *addr_p;

  tid = TIDGET(ptid);
  if (tid == 0)
    tid = PIDGET (ptid);

  errno = 0;
  ptrace (PTRACE_GETSIGINFO, tid, (PTRACE_TYPE_ARG3) 0, &siginfo);

  if (errno != 0 || siginfo.si_signo != SIGTRAP ||
      (siginfo.si_code & 0xffff) != 0x0004)
    return 0;

  last_stopped_data_address = (CORE_ADDR) siginfo.si_addr;
  return 1;
}

int
ppc_linux_stopped_data_address (struct target_ops *target, CORE_ADDR 
*addr_p)
{
  if (last_stopped_data_address)
    {
      *addr_p = last_stopped_data_address;
      return 1;
    }
  return 0;
}

last_stopped_data_address is a static CORE_ADDR in ppc-linux-nat.c, which 
is initialized to 0.

> I realize this is gross; the entire "step, then check the stopped data
> address" is a bad idea, in my opinion.

I am confused by this at the very first time too.  But it seems to 
desirable provided that quite a lot of h/w watchpoint is non-steppable.  
Maybe we can adopt another patch for data watchpoint, to say, "check 
data address, then single step"?

> 
> > 3. The third one is a little tricky.  Now that ppc has at most 1 DABR. So 
> > I can set the stopped_data_address to the data address when we set the 
> > watchpoint (in ppc_linux_insert_watchpoint).  Everytime 
> > target_stopped_data_address is called, the breakpoint is either read or 
> > access, so it is already clear that it is stopped by watchpoint.  Then 
> > this trick seems to make sense, right?
> 
> Weren't there other PPCs with more than one though?

Sorry, I don't know much about different PPC processors.  My knowledge 
about DABR is from the publically available PowerPC programming environment
manual. That book said that PowerPC has one optional DABR register. And 
the kernel is also coding like that.

Anton, do you have any comment on this question?

> 
> > I had tested the above three methods.  The first one works ok when the 
> > data breakpoint is aligned by 8 bytes.  The third one works ok for both 
> > aligned and non-aligned data breakpoint.  For the second one, I don't know 
> > how to work around the extra PTRACE_GETSIGINFO call caused by the single 
> > step yet.  But if I reserver the stopped_data_address when we first 
> > hit watchpoint, and store it back when I call ppc_linux_stopped_data_address. 
> > I can make rwatch and awatch to work as expected.
> 
> ... right, I guess I should have read your whole message first!  That's
> my preferred option if we can find a clean way to do it.  I thought
> there was a target with an example of this, but I can't find it.  I'm
> not sure I understand why ia64 needs to disable the watchpoint, though.

I had a search in the current gdb source.  Don't see it either. As for the 
IA64 mechanism, I don't understand that either. :-) But it seems that it 
can handle the problem above. (If no, how can these code got into the 
source? ) 

Regards
- Wu Zhou