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

["K.Prasad" <prasad@linux.vnet.ibm.com>]

On Wed, Dec 07, 2011 at 05:01:57PM -0200, Thiago Jung Bauermann wrote:
> 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.
>

Okay.
 
> > > > +		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.
> 

Okay, I will modify the code accordingly.

> > > > @@ -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.
>

Fine, here too. We'll return a -ENOENT here.

Thanks for your comments.

-- K.Prasad