Re: patch for invalid hw breakpoints

Re: patch for invalid hw breakpoints

[Eli Zaretskii]

[Patches should be sent to gdb-patches,; I redirected the thread.]

> Date: Thu, 01 Jun 2006 14:20:40 +0100
> From: Nathan Sidwell <nathan@codesourcery.com>
> 
> I have a remote target w/o hw watchpoint support.  I set a watchpoint only to 
> find that I then get an undeletable watchpoint as we try and remove it.
> 
> The bug is that when insert_bp_location fails to insert one of the watched 
> addresses for a watch expression it then goes and tries to remove all the 
> addresses of that watched expression, rather than just the previous fragments of 
> the expression.

And why is that a problem?  I'm sorry, but I couldn't figure out from
your description why removing a watch for an expression for which
insertion didn't happen causes trouble.  IIRC, it is supposed to
silently fail, and its only effect should be to cause GDB to be happy
that the watchpoint was completely removed.  What am I missing?

Re: patch for invalid hw breakpoints

[Daniel Jacobowitz]

On Thu, Jun 01, 2006 at 08:17:48PM +0300, Eli Zaretskii wrote:
> [Patches should be sent to gdb-patches,; I redirected the thread.]
> 
> > Date: Thu, 01 Jun 2006 14:20:40 +0100
> > From: Nathan Sidwell <nathan@codesourcery.com>
> > 
> > I have a remote target w/o hw watchpoint support.  I set a watchpoint only to 
> > find that I then get an undeletable watchpoint as we try and remove it.
> > 
> > The bug is that when insert_bp_location fails to insert one of the watched 
> > addresses for a watch expression it then goes and tries to remove all the 
> > addresses of that watched expression, rather than just the previous fragments of 
> > the expression.
> 
> And why is that a problem?  I'm sorry, but I couldn't figure out from
> your description why removing a watch for an expression for which
> insertion didn't happen causes trouble.  IIRC, it is supposed to
> silently fail, and its only effect should be to cause GDB to be happy
> that the watchpoint was completely removed.  What am I missing?

It caused trouble for a remote stub.  If I remember Nathan's example
right, we sent out packets like this:

-> Z02,1111110
<- E01
-> z02,1111110

We're asking the stub to remove something that isn't inserted.  The
protocol documentation isn't clear on whether stubs have to support
this... but it didn't say that they did, and at least one didn't.

Nathan, have I got that right?

-- 
Daniel Jacobowitz
CodeSourcery

Re: patch for invalid hw breakpoints

[Nathan Sidwell]

Daniel Jacobowitz wrote:
at the watchpoint was completely removed.  What am I missing?
> 
> 
> It caused trouble for a remote stub.  If I remember Nathan's example
> right, we sent out packets like this:
> 
> -> Z02,1111110
> <- E01
> -> z02,1111110
> 
> We're asking the stub to remove something that isn't inserted.  The
> protocol documentation isn't clear on whether stubs have to support
> this... but it didn't say that they did, and at least one didn't.

The remote stub can deal with this, but the functionality detection code in 
remote.c cannot.  In particular this bit of code in remote_remove_watchpoint 
triggers because the just attempted remote_insert_watchpoint call has now set 
the support field to PACKET_DISABLE.

   if (remote_protocol_packets[PACKET_Z0 + packet].support == PACKET_DISABLE)
     error (_("Can't clear hardware watchpoints without the '%s' (%s) packet."),
	   remote_protocol_packets[PACKET_Z0 + packet].name,
	   remote_protocol_packets[PACKET_Z0 + packet].title);

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: patch for invalid hw breakpoints

[Daniel Jacobowitz]

On Thu, Jun 01, 2006 at 06:45:32PM +0100, Nathan Sidwell wrote:
> The remote stub can deal with this, but the functionality detection code in 
> remote.c cannot.  In particular this bit of code in 
> remote_remove_watchpoint triggers because the just attempted 
> remote_insert_watchpoint call has now set the support field to 
> PACKET_DISABLE.

Oh.  So, more like this:

-> Z2,11110000
<- [empty: I don't support that.]
tries to send: -> z2,11110000
[internal error, I already know I don't support that!]

That could be changed in remote.c, but not removing what we didn't
insert does seem cleaner.

-- 
Daniel Jacobowitz
CodeSourcery

Re: patch for invalid hw breakpoints

[Eli Zaretskii]

> Date: Thu, 1 Jun 2006 14:03:21 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org
> 
> -> Z2,11110000
> <- [empty: I don't support that.]
> tries to send: -> z2,11110000
> [internal error, I already know I don't support that!]
> 
> That could be changed in remote.c, but not removing what we didn't
> insert does seem cleaner.

Is it really clean for remote.c to throw an internal error?  I always
thought remote.c implemented an API similar to ptrace, so it should
behave like ptrace, i.e. return an error code, not throw exceptions.

More to the point, I don't like the solution: IMHO it assumes too much
about the procedure we use to insert high-level watchpoints.  The
assumption that we never insert target-side watchpoints past some
point in the value chain is not guaranteed to hold forever.

If solving this in remote.c seems unclean for some reason (I don't
think so, but that's me), how about adding to the watchpoint data
structure a flag for each low-level watchpoint we insert, and storing
there whether it was actually inserted?  The code that removes
watchpoints could then consult that flag and refrain from removing a
non-inserted watch.  Does this make sense?

Re: patch for invalid hw breakpoints

[Daniel Jacobowitz]

On Thu, Jun 01, 2006 at 11:53:38PM +0300, Eli Zaretskii wrote:
> > Date: Thu, 1 Jun 2006 14:03:21 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org
> > 
> > -> Z2,11110000
> > <- [empty: I don't support that.]
> > tries to send: -> z2,11110000
> > [internal error, I already know I don't support that!]
> > 
> > That could be changed in remote.c, but not removing what we didn't
> > insert does seem cleaner.
> 
> Is it really clean for remote.c to throw an internal error?  I always
> thought remote.c implemented an API similar to ptrace, so it should
> behave like ptrace, i.e. return an error code, not throw exceptions.

Normally it would; this is an internal error, i.e. a failed consistency
check.  Well, it calls error(), but I think it really qualifies as an
internal error.

Anyway, do you think it's sensible for a "target_remove_watchpoint"
method to be called on something that is not an inserted watchpoint?
I'd think no.  It could mess up reference counts, for instance.

> More to the point, I don't like the solution: IMHO it assumes too much
> about the procedure we use to insert high-level watchpoints.  The
> assumption that we never insert target-side watchpoints past some
> point in the value chain is not guaranteed to hold forever.
> 
> If solving this in remote.c seems unclean for some reason (I don't
> think so, but that's me), how about adding to the watchpoint data
> structure a flag for each low-level watchpoint we insert, and storing
> there whether it was actually inserted?  The code that removes
> watchpoints could then consult that flag and refrain from removing a
> non-inserted watch.  Does this make sense?

That seems fine to me.  If you recall, long ago I was planning to split
this up to have a "low level breakpoint" (struct bp_location) for each
value in the chain, and bp_location already has the inserted flag.  But
I never had time to finish it.  This would be a less intrusive way of
accomplishing about the same thing.

-- 
Daniel Jacobowitz
CodeSourcery

Re: patch for invalid hw breakpoints

[Eli Zaretskii]

> Date: Thu, 1 Jun 2006 17:11:59 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Nathan Sidwell <nathan@codesourcery.com>, gdb-patches@sourceware.org
> 
> > Is it really clean for remote.c to throw an internal error?  I always
> > thought remote.c implemented an API similar to ptrace, so it should
> > behave like ptrace, i.e. return an error code, not throw exceptions.
> 
> Normally it would; this is an internal error, i.e. a failed consistency
> check.  Well, it calls error(), but I think it really qualifies as an
> internal error.

Why cannot it return an error code instead?

> Anyway, do you think it's sensible for a "target_remove_watchpoint"
> method to be called on something that is not an inserted watchpoint?
> I'd think no.  It could mess up reference counts, for instance.

The problem is, the high-level GDB interface to target-side watchpoint
support currently assumes that the target will cope with such
problems.  breakpoint.c doesn't know nor care about such
``unimportant'' details as how many addresses the target needs to
watch for a given data object on the value chain.  The requirement
from the target is to maintain whatever data it needs to track the
watchpoint related resources at all times, and silently cope with
seemingly unreasonable requests issued by the relatively blissful
high-level code in breakpoint.c.  For that, no target should ever
throw exceptions when GDB tries to do something unreasonable with
watchpoints.  For example, in the specific example of reference
counts, the target end should increment and decrement the counts even
if it doesn't actually insert/remove the watchpoint at the specified
address.

If we are about to change this basic arrangement, i.e. if we want to
keep some of the information on the application level, I fear we will
eventually need to redesign the whole interface.

Meanwhile, I really don't like the fact that remote.c throws an
internal error in situations that don't require that.  To me, internal
error means a situation akin to SIGSEGV: something is dead wrong, but
the code has no means of figuring out what's that.  I don't think we
are talking about such a situation.  On the contrary, the code knows
_exactly_ what went wrong: it was called to provide an unsupported
functionality.  The evident assumption that it will _never_ be called
more than once with unsupported requests is simply wrong: nobody said
that the application side is smart enough to always avoid that.  And
even if such an assumption was valid, it is not up to remote.c to
decide that an internal error is in order, because the impossible
situation happened in higher-level GDB code, not in remote.c.  I think
it is not a modular design to have code that throws an exception below
the level where an impossible situation happened.

> > If solving this in remote.c seems unclean for some reason (I don't
> > think so, but that's me), how about adding to the watchpoint data
> > structure a flag for each low-level watchpoint we insert, and storing
> > there whether it was actually inserted?  The code that removes
> > watchpoints could then consult that flag and refrain from removing a
> > non-inserted watch.  Does this make sense?
> 
> That seems fine to me.  If you recall, long ago I was planning to split
> this up to have a "low level breakpoint" (struct bp_location) for each
> value in the chain, and bp_location already has the inserted flag.  But
> I never had time to finish it.  This would be a less intrusive way of
> accomplishing about the same thing.

Indeed.  But I think remote.c shouldn't throw an internal error in
such cases.

Re: patch for invalid hw breakpoints

[Daniel Jacobowitz]

On Fri, Jun 02, 2006 at 11:42:59AM +0300, Eli Zaretskii wrote:
> > Anyway, do you think it's sensible for a "target_remove_watchpoint"
> > method to be called on something that is not an inserted watchpoint?
> > I'd think no.  It could mess up reference counts, for instance.
> 
> The problem is, the high-level GDB interface to target-side watchpoint
> support currently assumes that the target will cope with such
> problems.  breakpoint.c doesn't know nor care about such
> ``unimportant'' details as how many addresses the target needs to
> watch for a given data object on the value chain.  The requirement
> from the target is to maintain whatever data it needs to track the
> watchpoint related resources at all times, and silently cope with
> seemingly unreasonable requests issued by the relatively blissful
> high-level code in breakpoint.c.  For that, no target should ever
> throw exceptions when GDB tries to do something unreasonable with
> watchpoints.  For example, in the specific example of reference
> counts, the target end should increment and decrement the counts even
> if it doesn't actually insert/remove the watchpoint at the specified
> address.
> 
> If we are about to change this basic arrangement, i.e. if we want to
> keep some of the information on the application level, I fear we will
> eventually need to redesign the whole interface.

Is this contract that you're asserting documented anywhere?  I haven't
seen it written down, and it's very different from my interpretation.
It's likely that many targets get it wrong.  The documentation just
says "insert or remove a hardware watchpoint", and I wouldn't read that
as a requirement to support removing bogus things which we could not
insert.  We don't attempt to remove a disabled watchpoint; why should
we attempt to remove one we failed to insert?

Anyway, this is not a productive argument.  Let's change remote.c
instead.  If someone ever finishes separating into a location per
watched value, then we can arrange to only remove what we inserted
at that later time.

Nathan, want to remove the errors from both insert and remove methods?

> Meanwhile, I really don't like the fact that remote.c throws an
> internal error in situations that don't require that.  To me, internal
> error means a situation akin to SIGSEGV: something is dead wrong, but
> the code has no means of figuring out what's that.

If I were writing this code it would be a gdb_assert, which generates
an internal error.  Those are designed for two situations: hopelessly
confused, and violated assumptions.  This is the latter, not the
former.  It's thrown here because here is where we can detect that it
happened.

-- 
Daniel Jacobowitz
CodeSourcery

Re: patch for invalid hw breakpoints

[Eli Zaretskii]

> Date: Fri, 2 Jun 2006 09:52:38 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org, Nathan Sidwell <nathan@codesourcery.com>
> 
> Is this contract that you're asserting documented anywhere?

Not that I know of.

> Anyway, this is not a productive argument.  Let's change remote.c
> instead.  If someone ever finishes separating into a location per
> watched value, then we can arrange to only remove what we inserted
> at that later time.

That'd be fine with me.

> > Meanwhile, I really don't like the fact that remote.c throws an
> > internal error in situations that don't require that.  To me, internal
> > error means a situation akin to SIGSEGV: something is dead wrong, but
> > the code has no means of figuring out what's that.
> 
> If I were writing this code it would be a gdb_assert, which generates
> an internal error.  Those are designed for two situations: hopelessly
> confused, and violated assumptions.  This is the latter, not the
> former.  It's thrown here because here is where we can detect that it
> happened.

Assertions for violated assumptions reveal bugs.  In this case, the
bug is that the assumption was invalid.

Re: patch for invalid hw breakpoints

[Daniel Jacobowitz]

On Fri, Jun 02, 2006 at 11:56:53PM +0300, Eli Zaretskii wrote:
> > Date: Fri, 2 Jun 2006 09:52:38 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > Cc: gdb-patches@sourceware.org, Nathan Sidwell <nathan@codesourcery.com>
> > 
> > Is this contract that you're asserting documented anywhere?
> 
> Not that I know of.

Well then, that explains why I wasn't familiar with it!

I'll take your word for it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: patch for invalid hw breakpoints

[Nathan Sidwell]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

Eli Zaretskii wrote:
>>Date: Thu, 1 Jun 2006 17:11:59 -0400
>>From: Daniel Jacobowitz <drow@false.org>
>>Cc: Nathan Sidwell <nathan@codesourcery.com>, gdb-patches@sourceware.org
>>

> 
> Indeed.  But I think remote.c shouldn't throw an internal error in
> such cases.

This patch ok?

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk


[-- Attachment #2: remote-1.patch --]
[-- Type: text/x-patch, Size: 3301 bytes --]

2006-06-05  Nathan Sidwell  <nathan@codesourcery.com>

	* gdb/remote.c (remote_insert_watchpoint): Return -1, rather than
	fatal error if packet is disabled.
	(remote_remove_watchpoint, remote_insert_hw_breakpoint,
	remote_remove_hw_breakpoint): Likewise.

Index: gdb/remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.202.2.4
diff -c -3 -p -r1.202.2.4 remote.c
*** gdb/remote.c	24 May 2006 08:00:02 -0000	1.202.2.4
--- gdb/remote.c	5 Jun 2006 14:26:11 -0000
*************** remote_insert_watchpoint (CORE_ADDR addr
*** 4670,4678 ****
    enum Z_packet_type packet = watchpoint_to_Z_packet (type);
  
    if (remote_protocol_packets[PACKET_Z0 + packet].support == PACKET_DISABLE)
!     error (_("Can't set hardware watchpoints without the '%s' (%s) packet."),
! 	   remote_protocol_packets[PACKET_Z0 + packet].name,
! 	   remote_protocol_packets[PACKET_Z0 + packet].title);
  
    sprintf (buf, "Z%x,", packet);
    p = strchr (buf, '\0');
--- 4670,4676 ----
    enum Z_packet_type packet = watchpoint_to_Z_packet (type);
  
    if (remote_protocol_packets[PACKET_Z0 + packet].support == PACKET_DISABLE)
!     return -1;
  
    sprintf (buf, "Z%x,", packet);
    p = strchr (buf, '\0');
*************** remote_remove_watchpoint (CORE_ADDR addr
*** 4705,4714 ****
    enum Z_packet_type packet = watchpoint_to_Z_packet (type);
  
    if (remote_protocol_packets[PACKET_Z0 + packet].support == PACKET_DISABLE)
!     error (_("Can't clear hardware watchpoints without the '%s' (%s) packet."),
! 	   remote_protocol_packets[PACKET_Z0 + packet].name,
! 	   remote_protocol_packets[PACKET_Z0 + packet].title);
! 
    sprintf (buf, "z%x,", packet);
    p = strchr (buf, '\0');
    addr = remote_address_masked (addr);
--- 4703,4710 ----
    enum Z_packet_type packet = watchpoint_to_Z_packet (type);
  
    if (remote_protocol_packets[PACKET_Z0 + packet].support == PACKET_DISABLE)
!     return -1;
!   
    sprintf (buf, "z%x,", packet);
    p = strchr (buf, '\0');
    addr = remote_address_masked (addr);
*************** remote_insert_hw_breakpoint (CORE_ADDR a
*** 4796,4805 ****
    BREAKPOINT_FROM_PC (&addr, &len);
  
    if (remote_protocol_packets[PACKET_Z1].support == PACKET_DISABLE)
!     error (_("Can't set hardware breakpoint without the '%s' (%s) packet."),
! 	   remote_protocol_packets[PACKET_Z1].name,
! 	   remote_protocol_packets[PACKET_Z1].title);
! 
    *(p++) = 'Z';
    *(p++) = '1';
    *(p++) = ',';
--- 4792,4799 ----
    BREAKPOINT_FROM_PC (&addr, &len);
  
    if (remote_protocol_packets[PACKET_Z1].support == PACKET_DISABLE)
!     return -1;
!   
    *(p++) = 'Z';
    *(p++) = '1';
    *(p++) = ',';
*************** remote_remove_hw_breakpoint (CORE_ADDR a
*** 4838,4846 ****
    BREAKPOINT_FROM_PC (&addr, &len);
  
    if (remote_protocol_packets[PACKET_Z1].support == PACKET_DISABLE)
!     error (_("Can't clear hardware breakpoint without the '%s' (%s) packet."),
! 	   remote_protocol_packets[PACKET_Z1].name,
! 	   remote_protocol_packets[PACKET_Z1].title);
  
    *(p++) = 'z';
    *(p++) = '1';
--- 4832,4838 ----
    BREAKPOINT_FROM_PC (&addr, &len);
  
    if (remote_protocol_packets[PACKET_Z1].support == PACKET_DISABLE)
!     return -1;
  
    *(p++) = 'z';
    *(p++) = '1';

Re: patch for invalid hw breakpoints

[Eli Zaretskii]

> Date: Mon, 05 Jun 2006 15:29:42 +0100
> From: Nathan Sidwell <nathan@codesourcery.com>
> CC:  gdb-patches@sourceware.org, Daniel Jacobowitz <dan@codesourcery.com>
> 
> Eli Zaretskii wrote:
> >>Date: Thu, 1 Jun 2006 17:11:59 -0400
> >>From: Daniel Jacobowitz <drow@false.org>
> >>Cc: Nathan Sidwell <nathan@codesourcery.com>, gdb-patches@sourceware.org
> >>
> 
> > 
> > Indeed.  But I think remote.c shouldn't throw an internal error in
> > such cases.
> 
> This patch ok?

Yes, thanks.

Re: patch for invalid hw breakpoints

[Nathan Sidwell]

Daniel Jacobowitz wrote:
> On Thu, Jun 01, 2006 at 06:45:32PM +0100, Nathan Sidwell wrote:
> 
>>The remote stub can deal with this, but the functionality detection code in 
>>remote.c cannot.  In particular this bit of code in 
>>remote_remove_watchpoint triggers because the just attempted 
>>remote_insert_watchpoint call has now set the support field to 
>>PACKET_DISABLE.
> 
> 
> Oh.  So, more like this:
> 
> -> Z2,11110000
> <- [empty: I don't support that.]
> tries to send: -> z2,11110000
> [internal error, I already know I don't support that!]
> 
> That could be changed in remote.c, but not removing what we didn't
> insert does seem cleaner.

That was the other alternative, of tracking Z & z packet functionality 
separately, but as you say not removing what we didn't insert seems cleaner.

I suspect that there's actually another failure mode which my patch fixes and it 
would be hard for remote.c to fix, and that is when the watched expression 
inserts more than one hardware watchpoint.  The current code will attempt to 
insert multiple hw watchpoints, and remote.c will signal an internal error on 
inserting the second one

-> Z2,11110000
<- [empty: I don't support that.]
tries to send: -> Z2,abcdef
[internal error, I already know I don't support that!]

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: patch for invalid hw breakpoints

[Eli Zaretskii]

> Date: Fri, 02 Jun 2006 08:25:59 +0100
> From: Nathan Sidwell <nathan@codesourcery.com>
> CC: Eli Zaretskii <eliz@gnu.org>,  gdb-patches@sourceware.org
> 
> I suspect that there's actually another failure mode which my patch fixes and it 
> would be hard for remote.c to fix, and that is when the watched expression 
> inserts more than one hardware watchpoint.  The current code will attempt to 
> insert multiple hw watchpoints, and remote.c will signal an internal error on 
> inserting the second one
> 
> -> Z2,11110000
> <- [empty: I don't support that.]
> tries to send: -> Z2,abcdef
> [internal error, I already know I don't support that!]

One more reason not to throw an internal error in remote.c, if you ask
me.  Low-level interfaces have no business doing such things, IMHO.

Re: patch for invalid hw breakpoints

[Nathan Sidwell]

Nathan Sidwell wrote:

> The remote stub can deal with this, but the functionality detection code 
> in remote.c cannot.  In particular this bit of code in 
> remote_remove_watchpoint triggers because the just attempted 
> remote_insert_watchpoint call has now set the support field to 
> PACKET_DISABLE.

one thing that may not be clear from what I said is that this particular stub 
does not support Z2 packets, so returns an empty response as required by the gdb 
protocol.  If the stub did support Z2 packets, it would have to deal with 
duplicates.

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk