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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
