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

[Simon Marchi <simark@simark.ca>]

On 1/23/23 05:15, Johnson Sun via Gdb-patches wrote:
> Hi,
> 
> Thanks for reviewing this!

Sorry for the delay.

> 
>> the supplied test does not pass for me
> 
> The current test doesn't seem to produce consistent results across different
> machines, I'll try to fix it in the next version.
> 
>> 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?
> 
> I have implemented this in the v1 patch, I called `disable_breakpoint' as
> soon as we change the disposition to `disp_del_at_next_stop'
> (in `watchpoint_del_at_next_stop'). However,
> LancelotSIX <lsix@lancelotsix.com> mentioned that the fix is in a
> non-extension-related code path, and suggested disabling it in
> `bpstat_check_breakpoint_conditions' instead (the v3 patch).
> See: https://sourceware.org/pipermail/gdb-patches/2022-September/192120.html

Ah, sorry for missing that.  I now see that you also mentionned it in
your commit message.

> Both the v1 and v3 patch fixes the issue. I personally prefer the v1 patch.
> (i.e., modifying `watchpoint_del_at_next_stop')
> 
>> 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).
> 
> For software watchpoints, they cannot be pulled out of the target, since
> they may not be inserted into the target in the first place:
> 
>      /* Locations of type bp_loc_other and
>     bp_loc_software_watchpoint are only maintained at GDB side,
>     so there is no need to remove them.  */
> 
>     -- gdb/breakpoint.c:3840
> 
> Their expressions are re-evaluated by single-stepping through the program:
> 
>     Software watchpoints are very slow, since GDB needs to single-step
>     the program being debugged and test the value of the watched
>     expression(s) after each instruction.
> 
>     -- https://sourceware.org/gdb/wiki/Internals%20Watchpoints
> 
> Therefore, we must either disable or delete the software watchpoint.
> We cannot pull it out of the target since it's only maintained on the
> GDB side.

Ah ok, I had not understood that we are talking about software
watchpoints.

> 
>> 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?
> 
> I believe this choice is due to performance issues.
> 
> In an early attempt, I tried to call `breakpoint_auto_delete' on all kinds
> of stops. However, this implementation is very slow when we have a large
> number of breakpoints, since we need to go over the entire bpstat chain on
> all stops. I recall that this implementation fails on certain testcases with
> a large number of breakpoints with many breakpoint stops.

There's a difference between going over all breakpoints, and going over
the bpstat chain.  Unecessarily going over all breakpoints is not a good
idea, indeed.  But the bpstat chain only contains elements for
breakpoints that were hit right now.  In a pathological case, you could
have a lot of elements in that chain, but I don't think it's the norm.
The typical case is to have just one element, when hitting a single
breakpoint.

breakpoint_auto_delete does both.  I was thinking that just looking at
the bpstat chain, let's say at the end of handle_inferior_event, could
be done for cheap.

But in any case, that's a larger change, I think we should focus on your
fix, and if somebody feels like going it, this can be another change
later.

> Furthermore, the reason for using the `disp_del_at_next_stop' state remains
> unclear, as mentioned in the comments:
> 
>     /* NOTE drow/2003-09-08: This state only exists for removing
>        watchpoints.  It's not clear that it's necessary...  */
> 
>     -- gdb/breakpoint.c:2914
> 
> By tracing the git history, the `disp_del_at_next_stop' state is introduced
> in commit c906108c21474dfb4ed285bcc0ac6fe02cd400cc, which doesn't provide
> any proper explanation of this state.
> 
> I think the best way to deal with this is to avoid going over the entire
> bpstat chain when deleting breakpoints. Potentially by keeping track of
> a chain of breakpoints that should be deleted, and changing the bpstat chain
> to a doubly linked list for the ease of deletion. Such changes deserve a
> dedicated patch, though.

Maybe the bpstat chain could use intrusive_list:

https://gitlab.com/gnutools/binutils-gdb/-/blob/master/gdbsupport/intrusive_list.h

> To sum up, I prefer modifying `watchpoint_del_at_next_stop' (i.e., the v1 patch).
> If you also think it's appropriate, I'll fix the failing test and prepare the
> v4 patch accordingly.

I think I prefer the v1 too, it puts the action of disabling the
watchpoint closer to where we decide to disable it, so it's easier to
follow.  I don't think it's a problem that the fix touches a
non-extension code path.  The change in v1 sounds logical to me: the
intent of watchpoint_del_at_next_stop is to get rid of the watchpoint,
make it so the user can't hit it anymore.  It's just that for some
reason (unknown to me at this point), we can't delete it right away.  If
disabling it is needed to ensure the watchpoint is not seen by the user
in some circumstances, then so be it.

I guess another solution would be to make watchpoint_check check
b->disposition, and skip it if it's disp_del_at_next_stop.  I don't
think it's any better though.

Simon