Re: patch for invalid hw breakpoints

[Eli Zaretskii <eliz at gnu dot org>]

> 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.