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

[Pedro Alves <palves@redhat.com>]

On 02/24/2012 11:48 PM, Maciej W. Rozycki wrote:

> Hi Pedro,
> 
>  Back to this change, was distracted by something else.


So was I.  :-)

>>>  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.
> 
>  Yes, this is about watchpoints, not breakpoints. ;)


I fully understand that.  What I was saying is that if that is a concern,
then breakpoints with bit #31 set are quite likely to suffer from exactly
the same issue, with the remote possibly reporting the trapped address as
truncated to 32 bits or as a properly sign-extended 64-bit value.  And I'd
imagine the code that handles the low level traps/exceptions to handle
both watchpoint and breakpoint addresses the same, and the stub code that
marshals those breakpoint addresses to rsp to be just as susceptible to
the issue as code that marshals watchpoint addresses.  Now, I'm not
saying we should run to fix that...

I'm still curious why isn't bkpt_breakpoint_hit broken like I described
above, when a breakpoint is subject to adjustment, such as
that done by mips_breakpoint_from_pc.  I probably need to stare more
at the code.  ;-)

>> /* Implementation of target method FOO.  */
> 
>  Umm, there aren't that many comments of this kind there actually...


We started enforcing that rule not too long ago.

Anyway, thanks for the fixes, and congratulations on your new role.  :-)

-- 
Pedro Alves