Re: patch for invalid hw breakpoints

Daniel Jacobowitz
2006-06-02T13:52:45Z

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