Re: [PATCH v3] [PR python/29603] Disable out-of-scope watchpoints

[Simon Marchi <simark@simark.ca>]

On 10/20/22 14:11, Johnson Sun via Gdb-patches wrote:
> Currently, when a local software watchpoint goes out of scope, GDB sets the
> watchpoint's disposition to `delete at next stop' and then normal stops
> (i.e., stop and wait for the next GDB command). When GDB normal stops, it
> automatically deletes the breakpoints with their disposition set to
> `delete at next stop'.
> 
> Suppose a Python script decides not to normal stop when a local software
> watchpoint goes out of scope, the watchpoint will not be automatically
> deleted even when its disposition is set to `delete at next stop'.
> 
> Since GDB single-steps the program and tests the watched expression after each
> instruction, not deleting the watchpoint causes the watchpoint to be hit many
> more times than it should, as reported in PR python/29603.
> 
> This was happening because the watchpoint is not deleted or disabled when
> going out of scope.
> 
> This commit fixes this issue by disabling the watchpoint when going out of
> scope. It also adds a test to ensure this feature isn't regressed in the
> future.
> 
> Two other solutions seem to solve this issue, but are in fact inappropriate:
> 1. Automatically delete breakpoints on all kinds of stops
>    (in `fetch_inferior_event'). This solution is very slow since the deletion
>    requires O(N) time for N breakpoints.
> 2. Disable the watchpoint after the watchpoint's disposition is set to
>    `delete at next stop' (in `watchpoint_del_at_next_stop'). This solution
>    modifies a non-extension-related code path, and isn't preferred since this
>    issue cannot occur without extension languages. (gdb scripts always normal
>    stop before continuing execution)

I have a bit of trouble reviewing this, because I'm not too familiar
with the lifecycle of watchpoints (I know the principle, but not the
specifically where things happen in GDB).  So it's difficult to tell
whether this is right or if there's a better way to handle it.

However, the supplied test does not pass for me:

    source /home/simark/build/binutils-gdb/gdb/testsuite/outputs/gdb.python/py-watchpoint/py-watchpoint.py
    Watchpoint 2: i
    Python script imported
    (gdb) PASS: gdb.python/py-watchpoint.exp: import python scripts
    python print(len(gdb.breakpoints()))
    2
    (gdb) PASS: gdb.python/py-watchpoint.exp: check modified BP count
    gdb_expect_list pattern: /Watchpoint Hit: ./
    continue
    Continuing.
    Watchpoint Hit: 1
    gdb_expect_list pattern: /[
    ]+Watchpoint . deleted because the program has left the block in/
    FAIL: gdb.python/py-watchpoint.exp: run until program stops (pattern 2) (timeout)
    gdb_expect_list pattern: /[
    ]+which its expression is valid./
    gdb_expect_list pattern: /Watchpoint Hit: ./
    gdb_expect_list pattern: /[
    ]+012\[Inferior 1 \(process .*\) exited normally\]/
    gdb_expect_list pattern: //
    python print(len(gdb.breakpoints()))
    Watchpoint Hit: 2
    Watchpoint Hit: 3
    Watchpoint Hit: 4

    Watchpoint 2 deleted because the program has left the block in
    which its expression is valid.
    Watchpoint Hit: 5
    012[Inferior 1 (process 2381681) exited normally]
    (gdb) FAIL: gdb.python/py-watchpoint.exp: check BP count (the program exited)

> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29603
> ---
>  gdb/breakpoint.c                           |  2 +
>  gdb/testsuite/gdb.python/py-watchpoint.c   | 27 ++++++++++++
>  gdb/testsuite/gdb.python/py-watchpoint.exp | 48 ++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-watchpoint.py  | 30 ++++++++++++++
>  4 files changed, 107 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.c
>  create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.exp
>  create mode 100644 gdb/testsuite/gdb.python/py-watchpoint.py
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index bff3bac7d1a..15f4ae2131c 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5340,6 +5340,8 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>    /* Evaluate extension language breakpoints that have a "stop" method
>       implemented.  */
>    bs->stop = breakpoint_ext_lang_cond_says_stop (b);
> +  if (b->disposition == disp_del_at_next_stop)
> +    disable_breakpoint(b);

Is there a reason to do it here in particular, and not, let's say as
soon as we change the disposition to disp_del_at_next_stop?

Other question: shouldn't marking the watchpoint as
disp_del_at_next_stop make it so it won't be inserted next time we
resume?  After all should_be_inserted returns false for breakpoint
locations that are disp_del_at_next_stop.  Perhaps it's because since we
don't do a "normal" stop, breakpoint locations stay inserted, and
there's nothing that pulls this location out of the target?  Therefore,
maybe a solution, to keep things coherent, would be to pull the
breakpoint locations out when marking the breakpoint as
disp_del_at_next_stop?  This way, we would respect the invariant that a
breakpoint disp_del_at_next_stop can't be inserted (I don't know if this
is really what is happening though, it's just speculation).

And, in a broader scope, why wait until the next normal stop to delete
the watchpoint?  A "normal" stop being a stop that we present to the
user (the normal_stop function).  We currently call
breakpoint_auto_delete in normal_stop, which is why we don't reach it
when your Breakpoint.stop method returns False.  At the end of, let's
say, handle_signal_stop, could we go over the bpstat chain and delete
any breakpoint we've just hit that is marked for deletion?

Simon