Re: [PATCH] remote: Fix hw watchpoint address matching

[Pedro Alves <pedro at codesourcery dot com>]

On Wednesday 07 December 2011 23:18:21, Maciej W. Rozycki wrote:
> On Wed, 7 Dec 2011, Pedro Alves wrote:
> 
> > > +static int
> > > +remote_watchpoint_addr_within_range (struct target_ops *target, CORE_ADDR addr,
> > > +				     CORE_ADDR start, int length)
> > > +{
> > > +  CORE_ADDR diff = remote_address_masked (addr - start);
> > > +
> > > +  return diff >= 0 && diff < length;
> > > +}
> > 
> > CORE_ADDR is unsigned.  `>= 0' is always true.
> 
>  Umm...
> 
> > Wouldn't it be much more readable to:
> > 
> > {
> >   CORE_ADDR start = remote_address_masked (start);
> > 
> >   return start <= addr && addr < start + length;
> > }
> > 
> > ?
> > 
> > (assuming addr is already masked, since that was the address
> > the target reported.)
> 
>  This makes me nervous.  I think we should be liberal on what we accept.  
> In particular ILP32 ABIs on 64-bit targets may be affected.  An example is 
> the MIPS n64 ABI where the width of general registers is 64 bits and 
> addresses are sign-extended 32 bits.  When bit #31 is set in the address, 
> the remote stub may possibly report the value as truncated to 32 bits or 
> as a properly sign-extended 64-bit value.  Not that I observed this 
> anywhere, but I think we should accept both.

If such thing were possible, then wouldn't breakpoints break?
We store the (masked) address of where we ended up putting
the breakpoint in bp_tgt->placed_address (remote_insert_breakpoint),
and if the target reported an address not exactly bp_tgt->placed_address,
we wouldn't be able to match it up, resulting in spurious SIGTRAPs.
Hmm, actually, it looks like breakpoint.c:bkpt_breakpoint_hit is broken
in that it should be using bl->target_info.placed_address instead
of bl->address ?  How is this not breaking on cases that need
breakpoint adjustment?  I'm probably missing something.

>  Here's an updated version; I have annotated the function now too per 
> Joel's suggestion elsewhere even though these are rather scarce throughout 
> remote.c.  

Actually, for implementations of defined interfaces, such as the
target vector or gdbarch callbacks, we prefer to leave the explanation
of the interface to where the interface is defined, and, write something
like 

/* Implementation of target method FOO.  */

This prevents comment bit rot whenever the main comment in the
interface declaration changes, but implementations' comments
are forgotten.

I see that target_watchpoint_addr_within_range is unfortunately
undocumented in target.h.  Fortunately, you've already written
the necessary comment.  :-)  Could you place it there instead
please?  Okay with that change.  Thanks.

-- 
Pedro Alves