[prev in list] [next in list] [prev in thread] [next in thread]
List: gdb-patches
Subject: Re: [PATCH] gdb/mi: check thread exists when creating thread-specific b/p
From: Andrew Burgess via Gdb-patches <gdb-patches () sourceware ! org>
Date: 2023-04-28 23:18:08
Message-ID: 875y9fa95b.fsf () redhat ! com
[Download RAW message or body]
Andrew Burgess <aburgess@redhat.com> writes:
> I noticed the following behaviour:
>
> $ gdb -q -i=mi /tmp/hello.x
> =thread-group-added,id="i1"
> =cmd-param-changed,param="print pretty",value="on"
> ~"Reading symbols from /tmp/hello.x...\n"
> (gdb)
> -break-insert -p 99 main
> ^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x00000000004 \
> 01198",func="main",file="/tmp/hello.c",fullname="/tmp/hello.c",line="18",thread-groups=["i1"],thread="99",times="0",original-location="main"}
> (gdb)
> info breakpoints
> &"info breakpoints\n"
> ~"Num Type Disp Enb Address What\n"
> ~"1 breakpoint keep y 0x0000000000401198 in main at /tmp/hello.c:18\n"
> &"../../src/gdb/thread.c:1434: internal-error: print_thread_id: Assertion `thr != \
> nullptr' failed.\nA problem internal to GDB has been detected,\nfurther debugging \
> may prove unreliable." &"\n"
> &"----- Backtrace -----\n"
> &"Backtrace unavailable\n"
> &"---------------------\n"
> &"\nThis is a bug, please report it."
> &" For instructions, see:\n"
> &"<https://www.gnu.org/software/gdb/bugs/>.\n\n"
> Aborted (core dumped)
>
> What we see here is that when using the MI a user can create
> thread-specific breakpoints for non-existent threads. Then if we try
> to use the CLI 'info breakpoints' command GDB throws an assertion.
> The assert is a result of the print_thread_id call when trying to
> build the 'stop only in thread xx.yy' line; print_thread_id requires a
> valid thread_info pointer, which we can't have for a non-existent
> thread.
>
> In contrast, when using the CLI we see this behaviour:
>
> $ gdb -q /tmp/hello.x
> Reading symbols from /tmp/hello.x...
> (gdb) break main thread 99
> Unknown thread 99.
> (gdb)
>
> The CLI doesn't allow a breakpoint to be created for a non-existent
> thread. So the 'info breakpoints' command is always fine.
>
> Interestingly, the MI -break-info command doesn't crash, this is
> because the MI uses global thread-ids, and so never calls
> print_thread_id. However, GDB does support using CLI and MI in
> parallel, so we need to solve this problem.
>
> One option would be to change the CLI behaviour to allow printing
> breakpoints for non-existent threads. This would preserve the current
> MI behaviour.
>
> The other option is to pull the MI into line with the CLI and prevent
> breakpoints being created for non-existent threads. This is good for
> consistency, but is a breaking change for the MI.
>
> In the end I figured that it was probably better to retain the
> consistent CLI behaviour, and just made the MI reject requests to
> place a breakpoint on a non-existent thread. The only test we had
> that depended on the old behaviour was
> gdb.mi/mi-thread-specific-bp.exp, which was added by me in commit:
>
> commit 2fd9a436c8d24eb0af85ccb3a2fbdf9a9c679a6c
> Date: Fri Feb 17 10:48:06 2023 +0000
>
> gdb: don't duplicate 'thread' field in MI breakpoint output
>
> I certainly didn't intend for this test to rely on this feature of the
> MI, so I propose to update this test to only create breakpoints for
> threads that exist.
>
> Actually, I've added a new test that checks the MI rejects creating a
> breakpoint for a non-existent thread, and I've also extended the test
> to run with the separate MI/CLI UIs, and then tested 'info
> breakpoints' to ensure this command doesn't crash.
>
> I've extended the documentation of the `-p` flag to explain the
> constraints better.
>
> I have also added a NEWS entry just in case someone runs into this
> issue, at least then they'll know this change in behaviour was
> intentional.
>
> One thing that I did wonder about while writing this patch, is whether
> we should treat requests like this, on both the MI and CLI, as another
> form of pending breakpoint, something like:
>
> (gdb) break foo thread 9
> Thread 9 does not exist.
> Make breakpoint pending on future thread creation? (y or [n]) y
> Breakpoint 1 (foo thread 9) pending.
> (gdb) info breakpoints
> Num Type Disp Enb Address What
> 1 breakpoint keep y <PENDING> foo thread 9
>
> Don't know if folk think that would be a useful idea or not? Either
> way, I think that would be a separate patch from this one.
> ---
I've gone ahead and pushed this change. This was blocking another
series I'd like to post to the list.
Let me know if there are any problems.
Thanks,
Andrew
> gdb/NEWS | 6 ++
> gdb/doc/gdb.texinfo | 5 +-
> gdb/mi/mi-cmd-break.c | 2 +
> .../gdb.mi/mi-thread-specific-bp.exp | 68 +++++++++++++++----
> 4 files changed, 67 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index c32ff92c98a..e4a78dca2df 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -96,6 +96,12 @@ show always-read-ctf
> without a thread restriction. The same is also true for the 'task'
> field of an Ada task-specific breakpoint.
>
> +** It is no longer possible to create a thread-specific breakpoint for
> + a thread that doesn't exist using '-break-insert -p ID'. Creating
> + breakpoints for non-existent threads is not allowed when using the
> + CLI, that the MI allowed it was a long standing bug, which has now
> + been fixed.
> +
> *** Changes in GDB 13
>
> * MI version 1 is deprecated, and will be removed in GDB 14.
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index bfda7edc4f7..4a81580b061 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -32161,7 +32161,10 @@
> Initialize the @var{ignore-count}.
> @item -p @var{thread-id}
> Restrict the breakpoint to the thread with the specified global
> -@var{thread-id}.
> +@var{thread-id}. @var{thread-id} must be a valid thread-id at the
> +time the breakpoint is requested. Breakpoints created with a
> +@var{thread-id} will automatically be deleted when the corresponding
> +thread exits.
> @item --qualified
> This option makes @value{GDBN} interpret a function name specified as
> a complete fully-qualified name.
> diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
> index 75957b75bad..e5432d58990 100644
> --- a/gdb/mi/mi-cmd-break.c
> +++ b/gdb/mi/mi-cmd-break.c
> @@ -243,6 +243,8 @@ mi_cmd_break_insert_1 (int dprintf, const char *command, char \
> **argv, int argc) break;
> case THREAD_OPT:
> thread = atol (oarg);
> + if (!valid_global_thread_id (thread))
> + error (_("Unknown thread %d."), thread);
> break;
> case PENDING_OPT:
> pending = 1;
> diff --git a/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp \
> b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp index 4586fa44bbc..9c04d6f8c2a \
> 100644
> --- a/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
> +++ b/gdb/testsuite/gdb.mi/mi-thread-specific-bp.exp
> @@ -29,21 +29,63 @@ if [build_executable ${testfile}.exp ${binfile} ${srcfile}] {
> return -1
> }
>
> -if {[mi_clean_restart $binfile]} {
> - return -1
> -}
> -
> -mi_create_breakpoint "-p 1 bar" "thread-specific b/p on bar" \
> - -thread "1"
> -
> proc make_loc {num} {
> return [mi_make_breakpoint_loc -thread "1" -number "$::decimal\\.$num"]
> }
>
> -set loc1 [make_loc 1]
> -set loc2 [make_loc 2]
> -set loc3 [make_loc 3]
> +foreach_mi_ui_mode mode {
> +
> + if {$mode == "separate"} {
> + set start_ops "separate-mi-tty"
> + } else {
> + set start_ops ""
> + }
> +
> + if {[mi_clean_restart $binfile $start_ops]} {
> + return -1
> + }
> +
> + # Ensure we get an error when placing a b/p for thread 1 at a point
> + # where thread 1 doesn't exist.
> + mi_gdb_test "-break-insert -p 1 bar" \
> + "\\^error,msg=\"Unknown thread 1\\.\""
>
> -mi_create_breakpoint_multi "-p 1 foo" "thread-specific b/p on foo" \
> - -thread "1" \
> - -locations "\\\[$loc1,$loc2,$loc3\\\]"
> + # If we have a separate CLI UI then run the 'info breakpoints'
> + # command. There was a time when the previous breakpoint request
> + # would succeed, and then 'info breakpoint' on the CLI would
> + # trigger an assertion.
> + if {$mode eq "separate"} {
> + with_spawn_id $gdb_main_spawn_id {
> + gdb_test "info breakpoints" "No breakpoints or watchpoints\\." \
> + "check CLI 'info breakpoints' when there are no breakpoints"
> + }
> + }
> +
> + if {[mi_runto_main] == -1} {
> + return -1
> + }
> +
> + # Ensure we get an error when placing a b/p for a thread that doesn't
> + # exist (when other threads do exist).
> + mi_gdb_test "-break-insert -p 999 bar" \
> + "\\^error,msg=\"Unknown thread 999\\.\""
> +
> + mi_create_breakpoint "-p 1 bar" "thread-specific b/p on bar" \
> + -thread "1"
> +
> + set loc1 [make_loc 1]
> + set loc2 [make_loc 2]
> + set loc3 [make_loc 3]
> +
> + mi_create_breakpoint_multi "-p 1 foo" "thread-specific b/p on foo" \
> + -thread "1" \
> + -locations "\\\[$loc1,$loc2,$loc3\\\]"
> +
> + # Check that 'info breakpoints' on the CLI succeeds.
> + if {$mode eq "separate"} {
> + with_spawn_id $gdb_main_spawn_id {
> + gdb_test "info breakpoints" ".*" \
> + "check CLI 'info breakpoints' when there are some breakpoints"
> + }
> + }
> +}
>
> base-commit: 6a208145d24c47912c8beb4f1f4b9abeb8d51134
> --
> 2.25.4
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic