Re: [PATCH 8/8] gdb: fix mi breakpoint-deleted notifications for thread-specific b/p

[Pedro Alves <pedro@palves.net>]

Hi!

On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:

> My proposed solution is this; in remove_threaded_breakpoints, instead
> of setting the disposition to disp_del_at_next_stop and setting the
> number to zero, we now just call delete_breakpoint directly.
> 
> The notification will now be sent out immediately; as soon as the
> thread exits.
> 
> As the number has not changed when delete_breakpoint is called, the
> notification will have the correct number.
> 
> And as the breakpoint is immediately removed from the breakpoint list,
> we no longer need to worry about 'maint info breakpoints' trying to
> print the thread-id for an exited thread.
> 
> My only concern is that calling delete_breakpoint directly seems so
> obvious that I wonder why the original patch (that added
> remove_threaded_breakpoints) didn't take this approach.  This code was
> added in commit 49fa26b0411d, but the commit message offers no clues
> to why this approach was taken, and the original email thread offers
> no insights either[2].  There are no test regressions after making
> this change, so I'm hopeful that this is going to be fine.
> 
> [2] https://sourceware.org/pipermail/gdb-patches/2013-September/106493.html

I can't be absolutely sure there weren't other reasons, but I think that was because back
then we couldn't access inferior memory if the current thread was running or had exited, when
native debugging on GNU/Linux.  One of the original versions of the step-over-thread-exit
series had that problem as well, which was what lead to changing gdb to always access
memory via /proc/pid/mem instead of ptrace, eliminating that class of problems.

> 
> The Complication
> ----------------
> 
> Of course, it couldn't be that simple.

It never is.  :-)

> +	    # The output has arrived!  Check how we did.  There are other bugs
> +	    # that come into play here which change which output we'll see.
> +	    if {$mode eq "separate" && !$is_remote} {
> +		gdb_assert { $saw_mi_thread_exited && $saw_mi_bp_deleted \
> +			     && !$saw_cli_thread_exited \
> +			     && !$saw_cli_bp_deleted } \
> +		    $gdb_test_name
> +	    } else {
> +		if { $saw_mi_thread_exited && $saw_mi_bp_deleted \
> +			     && $saw_cli_thread_exited \
> +			     && $saw_cli_bp_deleted } {
> +		    kpass "gdb/30129" $gdb_test_name
> +		} elseif { $saw_mi_thread_exited && $saw_mi_bp_deleted \
> +			     && !$saw_cli_thread_exited \
> +			     && $saw_cli_bp_deleted } {
> +		    kfail "gdb/30129" $gdb_test_name
> +		} else {
> +		    fail "$gdb_test_name (XXX)"

I guess you meant to remove the XX or replace it with something else?


Otherwise LGTM.

Approved-By: Pedro Alves <pedro@palves.net>