Re: patch for invalid hw breakpoints

[Daniel Jacobowitz <drow at false dot org>]

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