[RFC] Wrong hw_watchpoint_used_count? (multiple location watchpoints)

[RFC] Wrong hw_watchpoint_used_count? (multiple location watchpoints)

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1420 bytes --]

This is mostly from code inspection, while investigating something else,
but I think that the current implementation of hw_watchpoint_used_count
is inaccurate. It counts the number of breakpoints which match the given
type.  But in fact, when I looked at the way the watchpoint is created,
watchpoints may have multiple bp_locations. So, a better approximation
would be to count the number of bp_locations, no?

Even so, this does not appear to be entirely accurate either. For
instance, if the user tries to watch an entity that does not fit
in one watchpoint, I think we can still end up with a situation
where we have one bp_location needing 2 h/w watchpoints.
In the end, it looks like the only way to really clean things up
would be to store for each watchpoint the associated number of
needed hardware watchpoints...

I thought about making the following change.  It's not clear that it is
a step in the right direction or not.  Pragmatically, I think it gives
more accurate results.  In terms of implementation, if we eventually
decide to store the mem_cnt in the breakpoint, then we'll have to
go back to iterating over all breakpoints (not bp_locations)...

When in doubt, do nothing?

        * breakpoint.c (hw_watchpoint_used_count): Compute the number
        of hardware watchpoints by iterating over all bp_locations
        instead of all breakpoints.

Tested on x86_64-linux. No regression...

-- 
Joel

[-- Attachment #2: 0001-Wrong-hw_watchpoint_used_count-multiple-location-wat.patch --]
[-- Type: text/x-diff, Size: 1050 bytes --]

From 6b81fca8153717a836ecee2d4e7ce891b3d32a6e Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 29 Dec 2009 19:06:50 +0400
Subject: [PATCH] Wrong hw_watchpoint_used_count? (multiple location watchpoints)

        * breakpoint.c (hw_watchpoint_used_count): Compute the number
        of hardware watchpoints by iterating over all bp_locations
        instead of all breakpoints.

---
 gdb/breakpoint.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0dc8474..91587cb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5820,12 +5820,14 @@ hw_breakpoint_used_count (void)
 static int
 hw_watchpoint_used_count (enum bptype type, int *other_type_used)
 {
-  struct breakpoint *b;
+  struct bp_location *loc, **loc_temp;
   int i = 0;
 
   *other_type_used = 0;
-  ALL_BREAKPOINTS (b)
+  ALL_BP_LOCATIONS (loc, loc_temp)
   {
+    struct breakpoint *b = loc->owner;
+
     if (breakpoint_enabled (b))
       {
 	if (b->type == type)
-- 
1.6.3.3

Re: [RFC] Wrong hw_watchpoint_used_count? (multiple location watchpoints)

[Eli Zaretskii]

> Date: Fri, 8 Jan 2010 11:57:01 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> This is mostly from code inspection, while investigating something else,
> but I think that the current implementation of hw_watchpoint_used_count
> is inaccurate. It counts the number of breakpoints which match the given
> type.  But in fact, when I looked at the way the watchpoint is created,
> watchpoints may have multiple bp_locations. So, a better approximation
> would be to count the number of bp_locations, no?
> 
> Even so, this does not appear to be entirely accurate either. For
> instance, if the user tries to watch an entity that does not fit
> in one watchpoint, I think we can still end up with a situation
> where we have one bp_location needing 2 h/w watchpoints.
> In the end, it looks like the only way to really clean things up
> would be to store for each watchpoint the associated number of
> needed hardware watchpoints...

But this is something only the target knows.  There's no way for
breakpoint.c to know that, unless we introduce an API through which
breakpoint.c can ask the target to provide that number.

Please note, for example, that some watchpoints can actually need
*zero* bp_locations, because some other watchpoint already watches the
same address.  x86 uses reference counting to not waste hardware
resources for the same address.

Re: [RFC] Wrong hw_watchpoint_used_count? (multiple location watchpoints)

[Joel Brobecker]

> But this is something only the target knows.  There's no way for
> breakpoint.c to know that, unless we introduce an API through which
> breakpoint.c can ask the target to provide that number.

That's what I eventually thought as well... I have relatively little
experience in how GDB is designed to handle watchpoints, but it seemed
that the boundary between the what the target knows and how the core
uses it provide watchpoint support is pretty hard to find... Perhaps,
one day, we'll want to clarify this area of our code.

> Please note, for example, that some watchpoints can actually need
> *zero* bp_locations, because some other watchpoint already watches the
> same address.  x86 uses reference counting to not waste hardware
> resources for the same address.

Clever!

Thanks for the feedback.

-- 
Joel

Re: [RFC] Wrong hw_watchpoint_used_count? (multiple location watchpoints)

[Eli Zaretskii]

> Date: Fri, 8 Jan 2010 14:29:55 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > But this is something only the target knows.  There's no way for
> > breakpoint.c to know that, unless we introduce an API through which
> > breakpoint.c can ask the target to provide that number.
> 
> That's what I eventually thought as well... I have relatively little
> experience in how GDB is designed to handle watchpoints, but it seemed
> that the boundary between the what the target knows and how the core
> uses it provide watchpoint support is pretty hard to find...

The current boundary is the places where breakpoint.c calls the
various target_* methods which return information that only the target
knows.

It is my opinion that breakpoint.c currently tries to know too much
about the target-side implementation details of the watchpoints, and
that introduces unpleasant side effects and unwanted dependencies on
the underlying platforms.  One unpleasant side effect is that we only
announce at "continue" time that too many hardware resources were
required; we should have announced that at "watchpoint" time.

In particular, I think that the bookkeeping of the various locations
where watchpoints are inserted should be left to the target.  That
would allow, e.g., targets that don't support range watchpoints to
emulate them with multiple watchpoints.

Re: [RFC] Wrong hw_watchpoint_used_count? (multiple location watchpoints)

[Joel Brobecker]

> The current boundary is the places where breakpoint.c calls the
> various target_* methods which return information that only the target
> knows.

Right - my point is that the design itself is unclear. Your example
of the x86 watchpoint support is one of the examples where this is
particularly weird in terms of user interface, and where the implementation
implements a form of support that is different from what I *thought*
the spirit of watchpoint support in GDB was. I thought that the design
would be that GDB would ask the target if they still have enough
resources to add an extra hardware watchpoint, and automatically
downgrade to a software watchpoint if not. On x86, my guess when
I read the code, is that it won't even allow the user to continue
the debugging until some watchpoints are removed.

> One unpleasant side effect is that we only
> announce at "continue" time that too many hardware resources were
> required; we should have announced that at "watchpoint" time.

Fully agreed.

> In particular, I think that the bookkeeping of the various locations
> where watchpoints are inserted should be left to the target.  That
> would allow, e.g., targets that don't support range watchpoints to
> emulate them with multiple watchpoints.

I agree as well.

-- 
Joel

Re: [RFC] Wrong hw_watchpoint_used_count? (multiple location watchpoints)

[Eli Zaretskii]

> Date: Fri, 8 Jan 2010 16:25:55 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> I thought that the design would be that GDB would ask the target if
> they still have enough resources to add an extra hardware
> watchpoint, and automatically downgrade to a software watchpoint if
> not.

That _is_ the design.  However, GDB does not tell enough to the target
for the target to give an accurate answer.  And what's more, some
questions cannot be answered without actually trying to call ptrace or
its equivalents, and getting its ``opinion''.  Unless we mirror all
the necessary information on the target level that is (which is what
x86 does).

Re: [RFC] Wrong hw_watchpoint_used_count? (multiple location watchpoints)

[Joel Brobecker]

> That _is_ the design.  However, GDB does not tell enough to the target
> for the target to give an accurate answer.  And what's more, some
> questions cannot be answered without actually trying to call ptrace or
> its equivalents, and getting its ``opinion''.  Unless we mirror all
> the necessary information on the target level that is (which is what
> x86 does).

Hmmm, so you're saying that the situation on x86 is because we cannot
know in advance how many watchpoints the CPU provides?

-- 
Joel

Re: [RFC] Wrong hw_watchpoint_used_count? (multiple location watchpoints)

[Eli Zaretskii]

> Date: Fri, 8 Jan 2010 17:08:58 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > That _is_ the design.  However, GDB does not tell enough to the target
> > for the target to give an accurate answer.  And what's more, some
> > questions cannot be answered without actually trying to call ptrace or
> > its equivalents, and getting its ``opinion''.  Unless we mirror all
> > the necessary information on the target level that is (which is what
> > x86 does).
> 
> Hmmm, so you're saying that the situation on x86 is because we cannot
> know in advance how many watchpoints the CPU provides?

On x86 we do know.  On other architectures we might not be able to
know.  But even on x86, breakpoint.c does not pass enough information
to the target for the latter to know if the requested watchpoints will
be successfully inserted.  For that, GDB would need to provide the
address of each watchpoint, its type (read/write), and the length of
the watched area.  It currently does not provide that.