Re: [RFA 2/3] Demote to sw watchpoint only in update_watchpoint

[Eli Zaretskii <eliz at gnu dot org>]

> From: Thiago Jung Bauermann <bauerman@br.ibm.com>
> Cc: gdb-patches ml <gdb-patches@sourceware.org>
> Date: Tue, 03 May 2011 01:55:46 -0300
> 
> I hardcoded bp_hardware_watchpoint in an attempt to make read and access
> watchpoints also count as hardware watchpoints when determining the
> number of used and available debug resources (otherwise only watchpoints
> of the same type as the one being created are taken into account when
> counting how many debug resources are used).
> 
> I just realised it doesn't work, and to fix it I'd have to slightly
> change the meaning of the other_type_used argument to be the number of
> other types of watchpoints, instead of just one or zero indicated
> whether there is any watchpoint of another type being used.
> 
> I didn't make the change because I don't know if there's any target
> which would break. I couldn't find anywhere what other_type_used
> actually means...
> 
> On the other hand, the current code is already broken in that regard,
> and creating several read and access watchpoints on today's GDB will
> confuse it and allow creating more hardware watchpoints (read, access or
> regular) than possible. Thus, this patch doesn't make the situation any
> worse. So now I just use b->type.

Sorry, but I object.  This kind of changes just adds kludges upon
kludges on what is a fundamentally broken design to begin with.  It
was originally designed and implemented for a single platform (which
we no longer support, btw), and doesn't scale well, or not at all, to
other platforms, not even to x86.

The problem with the meaning of the other_type_used argument is the
telltale sign: it only made sense for that single platform for which
hardware watchpoints were originally implemented in GDB.  It no longer
makes any sense with today's platforms, and actually cannot be used at
all with most of them (at least those I'm familiar with), for a very
simple reason: a single `int' parameter doesn't provide a way to
report enough information about the consumption of related resources.
It doesn't matter if that parameter is a boolean or not; it simply
isn't up to this job.  Much more information is needed about the
related resources to decide whether another watchpoint can or cannot
be set.

The fundamental design problem here is this: there's no way GDB
application level can know whether the target can accommodate an
additional hardware watchpoint of a given type and attributes.  Only
the target itself can tell that accurately, because the information
needed to answer that question is extremely target-dependent.  OTOH,
it is bad UI design to let the user set a hardware watchpoint, then
demote it to a software watchpoint (or even refuse to insert a
watchpoint at all) at "resume" time.  So on the one hand, we _want_
the GDB application level to know if a hardware watchpoint is
possible, to tell the user it cannot, and OTOH there's a problem
knowing this at that level.

We should resolve this conflict in the most direct way: introduce a
method, to be implemented by each target, that will answer these
questions.  It should accept the exact spec of the new watchpoint, and
it should have access to the watchpoints and breakpoints already set,
including their full specs.  With that information in hand, a target
can reliably produce a definitive response, at least in the vast
majority of cases, when the corresponding resources are under GDB
control.

To avoid breaking too many targets, the default implementation should
be what we do now.  This will make the result no worse than the
current code.

Let's solve this problem once and for all.  Any other way of treating
this problem will only sweep it under the carpet for a few more months
or years, until some other advanced platform comes out that needs more
kludgeying.  The result will be (already is) code that is hard to
maintain and that is in real danger of breaking platforms other than
the one which needs the patch.  When will it stop?

> The real fix IMHO would be to make the -tdep code manage the creation
> and deletion of bp_locations, so that it could always make an informed
> decision about what can and cannot be done with the available hardware
> debug resources.

Exactly!

> But that's out of the scope of this patch series.

I say, let's stop postponing this to some other patch series.  I've
been to that movie enough to know that it's a polite form of saying
"never".

Sorry for being so blunt, but I think this problem is well past the
time we should have solved it for good.