Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

[Thiago Jung Bauermann <bauerman@br.ibm.com>]

On Thu, 2011-12-01 at 15:50 +0530, K.Prasad wrote:
> On Mon, Nov 28, 2011 at 02:11:11PM +1100, David Gibson wrote:
> > [snip]
> > On Wed, Oct 12, 2011 at 11:09:48PM +0530, K.Prasad wrote:
> > > diff --git a/Documentation/powerpc/ptrace.txt b/Documentation/powerpc/ptrace.txt
> > > index f4a5499..f2a7a39 100644
> > > --- a/Documentation/powerpc/ptrace.txt
> > > +++ b/Documentation/powerpc/ptrace.txt
> > > @@ -127,6 +127,22 @@ Some examples of using the structure to:
> > >    p.addr2           = (uint64_t) end_range;
> > >    p.condition_value = 0;
> > >  
> > > +- set a watchpoint in server processors (BookS)
> > > +
> > > +  p.version         = 1;
> > > +  p.trigger_type    = PPC_BREAKPOINT_TRIGGER_RW;
> > > +  p.addr_mode       = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
> > > +  or
> > > +  p.addr_mode       = PPC_BREAKPOINT_MODE_EXACT;
> > > +
> > > +  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
> > > +  p.addr            = (uint64_t) begin_range;
> > 
> > You should probably document the alignment constraint on the address
> > here, too.
> > 
> 
> Alignment constraints will be learnt by the user-space during runtime.
> We provide that as part of 'struct ppc_debug_info' in
> 'data_bp_alignment' field.
> 
> While the alignment is always 8-bytes for BookS, I think userspace
> should be left to learn it through PTRACE_PPC_GETHWDEBUGINFO.

Right. In particular, BookE doesn't have alignment constraints.

> > > +		attr.bp_len = len;
> > > +		ret =  modify_user_hw_breakpoint(bp, &attr);
> > > +		if (ret) {
> > > +			ptrace_put_breakpoints(child);
> > > +			return ret;
> > > +		}
> > 
> > If a bp already exists, you're modifying it.  I thought the semantics
> > of the new interface meant that you shoul return ENOSPC in this case,
> > and a DEL would be necessary before adding another breakpoint.
> > 
> 
> I'm not too sure what would be the desired behaviour for this interface,
> either way is fine with me. I'd like to hear from the GDB folks (copied
> in this email) to know what would please them.

ENOSPC should be returned. The interface doesn't have provisions for
modifying breakpoints. The client should delete/create instead of trying
to modify.

Since PTRACE_PPC_GETHWDEBUGINFO returns the number of available
breakpoint registers, the client shouldn't (and GDB doesn't) try to set
more breakpoints than possible.
 
> > > @@ -1426,10 +1488,24 @@ static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
> > >  #else
> > >  	if (data != 1)
> > >  		return -EINVAL;
> > > +
> > > +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> > > +	if (ptrace_get_breakpoints(child) < 0)
> > > +		return -ESRCH;
> > > +
> > > +	bp = thread->ptrace_bps[0];
> > > +	if (bp) {
> > > +		unregister_hw_breakpoint(bp);
> > > +		thread->ptrace_bps[0] = NULL;
> > > +	}
> > > +	ptrace_put_breakpoints(child);
> > > +	return 0;
> > 
> > Shouldn't DEL return an error if there is no existing bp.
> >
> 
> Same comment as above. We'd like to know what behaviour would help the
> GDB use this interface better as there's no right or wrong way here.

GDB expects DEL to return ENOENT is there's no existing bp.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center