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

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

Hi Eli,

On Tue, 2011-05-03 at 02:05 -0400, Eli Zaretskii wrote:
> 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.

This was hard for me to evaluate as I didn't find anywhere what
other_type_used was ever meant to be. Not even in the patch submission
that introduced it. Thanks for the insight.

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

GDB doesn't have a command to create a hardware watchpoint or a software
watchpoint. It will decide which will be created based on what it thinks
can be done. Do you think this should be changed and there should be an
hwatch command which would fail if an hw watchpoint is not possible? Or
just that a watchpoint should never be converted between sw and hw after
they are created?

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

In the particular case of this patch, I think this reasoning is
misplaced. It may not have been clear in my previous message, but what I
was saying was that I tried to attack the problem in a minimally
intrusive way (i.e., a kludge :-) ) but gave up and left the code
basically untouched.

I said that fixing the problem was out of the scope of this patch series
meaning "I was developing the feature and I noticed this problem. I
tried to solve it while I was at it but things got involved". So I don't
think it's fair to say "You tried to address that problem and even
mentioned in your patch submission, now you have to fix it!"

This is GDB's behaviour without this patch:

(gdb) awatch a
Hardware access (read/write) watchpoint 6: a
(gdb) rwatch c
Hardware read watchpoint 7: c
(gdb) watch d
Hardware watchpoint 8: d
(gdb) i b
Num     Type            Disp Enb Address    What
6       acc watchpoint  keep y              a
7       read watchpoint keep y              c
8       hw watchpoint   keep y              d
(gdb) c
Continuing.
Unexpected error setting breakpoint or watchpoint: No space left on device.
(gdb)

and this is the behaviour with the patch:

(gdb) awatch a
Hardware access (read/write) watchpoint 2: a
(gdb) rwatch c
Hardware read watchpoint 3: c
(gdb) watch d
Hardware watchpoint 4: d
(gdb) i b
Num     Type            Disp Enb Address    What
2       acc watchpoint  keep y              a
3       read watchpoint keep y              c
4       hw watchpoint   keep y              d
(gdb) c
Continuing.
Warning:
Could not insert hardware watchpoint 3.
Could not insert hardware watchpoint 4.
Could not insert hardware breakpoints:
You may have requested too many hardware breakpoints/watchpoints.

(gdb)

So the problem is still there. It's not better, but not worse either.
The output is slightly different because of the better error handling
this patch introduces. Thus, IMHO overhauling resources handling is not
a prerequisite or even directly related to this patch (or the next one
in the series). The acceptance or rejection of this patch is orthogonal
to that.

Having said that, I agree that it's a shame that GDB can't keep track of
debug hardware availability, something that one would expect to be among
the basic tasks of a debugger. So if we can agree on how GDB should deal
with the problem, I'm willing to work on it on a best effort basis
(meaning that I wouldn't have much time to dedicate to this, but would
eventually make progress).

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

No need to apologize. I understand your frustration, especially since I
read the discussion you and Cagney had in 2002 about this very topic,
and nothing actually changed since then...
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center