Message ID | 20160801211401.18155-2-simon.marchi@ericsson.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 13521 invoked by alias); 1 Aug 2016 21:15:08 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 13459 invoked by uid 89); 1 Aug 2016 21:15:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 spammy= X-HELO: usplmg20.ericsson.net Received: from usplmg20.ericsson.net (HELO usplmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 01 Aug 2016 21:14:55 +0000 Received: from EUSAAHC005.ericsson.se (Unknown_Domain [147.117.188.87]) by (Symantec Mail Security) with SMTP id 49.1B.02568.A1CBF975; Mon, 1 Aug 2016 23:16:10 +0200 (CEST) Received: from elxcz23q12-y4.dyn.mo.ca.am.ericsson.se (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.87) with Microsoft SMTP Server (TLS) id 14.3.301.0; Mon, 1 Aug 2016 17:14:05 -0400 From: Simon Marchi <simon.marchi@ericsson.com> To: <gdb-patches@sourceware.org> CC: Simon Marchi <simon.marchi@ericsson.com> Subject: [PATCH 1/2] mi: Restore original thread/frame when specifying --thread or --thread-group Date: Mon, 1 Aug 2016 17:14:00 -0400 Message-ID: <20160801211401.18155-2-simon.marchi@ericsson.com> In-Reply-To: <20160801211401.18155-1-simon.marchi@ericsson.com> References: <20160801211401.18155-1-simon.marchi@ericsson.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
Commit Message
Simon Marchi
Aug. 1, 2016, 9:14 p.m. UTC
I am sending a first version of this patch, even though I am not completely satisfied with it. Hopefully I can get some input from the community. We are in the process of integrating Pedro's great separate ui work [0] in Eclipse CDT. With this, the hope is that GDB users will feel right at home with a console that behaves just like plain GDB in a terminal, and appreciate the graphical support of CDT. Earlier [1], we found that if the current thread is X, passing "--thread Y" to an MI command would leave Y as the current thread. This is not a problem for CDT itself, since it always uses --thread for MI commands. However, it also affects the user selected thread. So the internal front-end logic can unexpectedly change the currently selected thread from the point of view of the user using the console. So, to avoid changing the current selection under the user's feet while they are typing a command, --thread and --thread-group should leave the inferior/thread/frame selection in their original state. This naive patch simply adds a restore_current_thread cleanup whenever --thread or --thread-group is used (using --frame implies using --thread). As a side effect, the code that checks whether a =thread-selected event should be emitted was simplified. It works okay for most commands, but I am worried about these corner cases: 1. If the command is actually meant to change the current thread and --thread/--thread-group is specified, the command will have no effect. Some front-ends might add --thread X for everything, so I think this is a plausible scenario: -thread-select --thread 3 4 -interpreter-exec --thread 3 console "thread 4" Eclipse CDT is not affected by this, but I am mentionning it anyway. 2. When a breakpoint is hit in _all-stop_, GDB's behavior is to switch to the thread that hit the breakpoint (which makes sense). With a _synchronous_ target, I have the feeling that it could inhibit this behavior, even though I couldn't reproduce it with "maint set target-async off". My idea is that the events could go like this: 1. The front-end issues "-exec-continue --thread-group i1". Or with --thread 1, it doesn't matter because it's all-stop. 2. We create a restore_current_thread cleanup with the current thread (let's say #1) 3. We enter the implementation of -exec-continue and we block somewhere deep in there because the target is sync 4. A breakpoint is hit by thread #2, so the implementation of -exec-continue eventually returns. It leaves thread #2 as the current thread, because it's the one that hit the breakpoint. 5. The cleanup (wrongfully) restores thread #1 as being the current thread. I can get around #1 by having an ugly global variable "meant_to_change_current_thread" that can be set to mean that the thread should not be restored to its original value, because the command actually wants to change the user selected thread. gdb_thread_select, for example, would do that. It really feels hackish though. Perhaps #2 could be solved the same way, but I don't really know where we would set meant_to_change_current_thread. IOW, where does GDB take the conscious decision to change/leave the user selected thread to the thread that hit the breakpoint? [0] https://sourceware.org/ml/gdb-patches/2016-05/msg00098.html [1] https://www.sourceware.org/ml/gdb/2015-10/msg00073.html gdb/ChangeLog: * mi/mi-main.c (mi_execute_command): Simplify computation of report_change. (mi_cmd_execute): Create restore current thread cleanup if one of --thread or --thread-group is used. --- gdb/mi/mi-main.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
Comments
On 08/01/2016 10:14 PM, Simon Marchi wrote: > I am sending a first version of this patch, even though I am not > completely satisfied with it. Hopefully I can get some input from the > community. > > We are in the process of integrating Pedro's great separate ui work [0] > in Eclipse CDT. With this, the hope is that GDB users will feel right > at home with a console that behaves just like plain GDB in a terminal, > and appreciate the graphical support of CDT. > > Earlier [1], we found that if the current thread is X, passing > "--thread Y" to an MI command would leave Y as the current thread. > This is not a problem for CDT itself, since it always uses --thread for > MI commands. However, it also affects the user selected thread. So the > internal front-end logic can unexpectedly change the currently selected > thread from the point of view of the user using the console. > > So, to avoid changing the current selection under the user's feet while > they are typing a command, --thread and --thread-group should leave the > inferior/thread/frame selection in their original state. > > This naive patch simply adds a restore_current_thread cleanup whenever > --thread or --thread-group is used (using --frame implies using > --thread). As a side effect, the code that checks whether a > =thread-selected event should be emitted was simplified. > > It works okay for most commands, but I am worried about these corner > cases: > > 1. If the command is actually meant to change the current thread and > --thread/--thread-group is specified, the command will have no > effect. Some front-ends might add --thread X for everything, so I > think this is a plausible scenario: > > -thread-select --thread 3 4 > -interpreter-exec --thread 3 console "thread 4" > > Eclipse CDT is not affected by this, but I am mentionning it anyway. > > 2. When a breakpoint is hit in _all-stop_, GDB's behavior is to switch to > the thread that hit the breakpoint (which makes sense). With a > _synchronous_ target, I have the feeling that it could inhibit this > behavior, even though I couldn't reproduce it with "maint set > target-async off". My idea is that the events could go like this: > > 1. The front-end issues "-exec-continue --thread-group i1". Or with > --thread 1, it doesn't matter because it's all-stop. > 2. We create a restore_current_thread cleanup with the current thread > (let's say #1) > 3. We enter the implementation of -exec-continue and we block somewhere > deep in there because the target is sync This doesn't happen, because even sync targets go through the event loop -> fetch_inferior_event nowadays. You may be able to trigger something with infcalls, as those are always blocking and go through different paths. Or maybe with execution commands inside a canned sequence of commands, and/or playing with "interpreter-exec mi" in the CLI. > 4. A breakpoint is hit by thread #2, so the implementation of > -exec-continue eventually returns. It leaves thread #2 as the > current thread, because it's the one that hit the breakpoint. > 5. The cleanup (wrongfully) restores thread #1 as being the current thread. > > I can get around #1 by having an ugly global variable > "meant_to_change_current_thread" that can be set to mean that the thread > should not be restored to its original value, because the command > actually wants to change the user selected thread. gdb_thread_select, > for example, would do that. It really feels hackish though. Yeah. Ugly, though might be the simplest. > > Perhaps #2 could be solved the same way, but I don't really know where > we would set meant_to_change_current_thread. IOW, where does GDB take > the conscious decision to change/leave the user selected thread to the > thread that hit the breakpoint? It's done in normal_stop, here: ... Also skip saying anything in non-stop mode. In that mode, as we don't want GDB to switch threads behind the user's back, to avoid races where the user is typing a command to apply to thread x, but GDB switches to thread y before the user finishes entering the command, fetch_inferior_event installs a cleanup to restore the current thread back to the thread the user had selected right after this event is handled, so we're not really switching, only informing of a stop. */ if (!non_stop && !ptid_equal (previous_inferior_ptid, inferior_ptid) && target_has_execution && last.kind != TARGET_WAITKIND_SIGNALLED && last.kind != TARGET_WAITKIND_EXITED && last.kind != TARGET_WAITKIND_NO_RESUMED) { SWITCH_THRU_ALL_UIS (state) { target_terminal_ours_for_output (); printf_filtered (_("[Switching to %s]\n"), target_pid_to_str (inferior_ptid)); annotate_thread_changed (); } previous_inferior_ptid = inferior_ptid; } An alternative may be to decouple the "user-selected thread" from "inferior_ptid" further. previous_inferior_ptid is already something like that, but not explicitly. So we'd make previous_inferior_ptid be the "user-selected thread", and make the places that want to explicitly change the user-selected thread change that global. That'd be gdb_thread_select, etc. and likely "kill", "detach", "attach", "run" and "target remote" too. The input/command entry points would then be responsible for making inferior_ptid (the internal selected thread) point to the user-selected thread. We'd no longer need to use make_cleanup_restore_current_thread to restore back the internal gdb selected thread, as it's simply no longer necessary. Reducing all the temporary thread switching may be a good thing. E.g., it likely reduces register cache refetching. This would be similar to how remote.c uses a lazy scheme that reduces Hg traffic, where gdb keeps a local cache of the remote's selected thread, and delays updating it on the remote target end until memory, registers etc. are accessed. That is, even if core gdb switches inferior_ptid around, that doesn't trigger immediate Hg packets. If the user has thread 3 selected, and gdb happens to need to read memory/registers off of thread 1, spread over several packets, then we only switch the remote side to thread 1 once, and don't switch it back to thread 3, even if inferior_ptid is switched back. So, assuming a simply implementation for clarity, here's what would happen inside gdb's little brain. Assume the user-selected thread starts out as thread 1: > -thread-select --thread 3 4 . MI starts processing input. The user-selected thread is thread 1, so MI switches inferior_ptid to match. Whatever was inferior_ptid before is irrelevant. . MI processes the "--thread 3" switch, which is handled by MI common code, and this causes inferior_ptid to be set to thread 3 (but not the user-selected thread global). . The "-thread-select" implementation switches both inferior_ptid and the current user-selected thread to thread 4. > -interpreter-exec --thread 3 console "thread 5" Similar. The last point is replaced by: . The "thread 5" implementation switches the user-visible thread to thread 5. > -interpreter-exec --thread 1 console "c" and then thread 3 hits a breakpoint. This is similar too. The last point is replaced by: . normal_stop switches the user-visible thread to thread 3. I think this might also pave the way to optionally make each UI have its own independently selected thread. (That would also solve these problems, I guess, though it bring in a set of its own problems. I think pulling that off would be a too large a change to make at this point. A frontend that would want to keep CLI and MI in sync would do that explicitly, by reacting to =thread-selected etc. events, which would be augmented to indicate the originating UI, and switching MI's selected thread accordingly. But certainly we'd need to make selected frame at least be per-UI as well, and who knows what else.) Thanks, Pedro Alves
On 16-08-02 10:49 AM, Pedro Alves wrote: > On 08/01/2016 10:14 PM, Simon Marchi wrote: >> I can get around #1 by having an ugly global variable >> "meant_to_change_current_thread" that can be set to mean that the thread >> should not be restored to its original value, because the command >> actually wants to change the user selected thread. gdb_thread_select, >> for example, would do that. It really feels hackish though. > > Yeah. Ugly, though might be the simplest. > >> >> Perhaps #2 could be solved the same way, but I don't really know where >> we would set meant_to_change_current_thread. IOW, where does GDB take >> the conscious decision to change/leave the user selected thread to the >> thread that hit the breakpoint? > > It's done in normal_stop, here: > > ... > Also skip saying anything in non-stop mode. In that mode, as we > don't want GDB to switch threads behind the user's back, to avoid > races where the user is typing a command to apply to thread x, > but GDB switches to thread y before the user finishes entering > the command, fetch_inferior_event installs a cleanup to restore > the current thread back to the thread the user had selected right > after this event is handled, so we're not really switching, only > informing of a stop. */ > if (!non_stop > && !ptid_equal (previous_inferior_ptid, inferior_ptid) > && target_has_execution > && last.kind != TARGET_WAITKIND_SIGNALLED > && last.kind != TARGET_WAITKIND_EXITED > && last.kind != TARGET_WAITKIND_NO_RESUMED) > { > SWITCH_THRU_ALL_UIS (state) > { > target_terminal_ours_for_output (); > printf_filtered (_("[Switching to %s]\n"), > target_pid_to_str (inferior_ptid)); > annotate_thread_changed (); > } > previous_inferior_ptid = inferior_ptid; > } > > > An alternative may be to decouple the "user-selected thread" from > "inferior_ptid" further. previous_inferior_ptid is already > something like that, but not explicitly. > > So we'd make previous_inferior_ptid be the "user-selected thread", and make > the places that want to explicitly change the user-selected thread change > that global. That'd be gdb_thread_select, etc. and likely "kill", "detach", > "attach", "run" and "target remote" too. > > The input/command entry points would then be responsible for making > inferior_ptid (the internal selected thread) point to the user-selected > thread. We'd no longer need to use make_cleanup_restore_current_thread > to restore back the internal gdb selected thread, as it's simply > no longer necessary. Reducing all the temporary thread switching > may be a good thing. E.g., it likely reduces register cache refetching. > > This would be similar to how remote.c uses a lazy scheme that reduces > Hg traffic, where gdb keeps a local cache of the remote's selected thread, > and delays updating it on the remote target end until memory, registers > etc. are accessed. That is, even if core gdb switches inferior_ptid around, > that doesn't trigger immediate Hg packets. If the user has thread 3 > selected, and gdb happens to need to read memory/registers off of > thread 1, spread over several packets, then we only switch the remote > side to thread 1 once, and don't switch it back to thread 3, even if > inferior_ptid is switched back. > > So, assuming a simply implementation for clarity, here's what > would happen inside gdb's little brain. > > Assume the user-selected thread starts out as thread 1: > >> -thread-select --thread 3 4 > > . MI starts processing input. The user-selected thread is thread 1, > so MI switches inferior_ptid to match. Whatever was inferior_ptid > before is irrelevant. > > . MI processes the "--thread 3" switch, which is handled by MI common > code, and this causes inferior_ptid to be set to thread 3 > (but not the user-selected thread global). > > . The "-thread-select" implementation switches both > inferior_ptid and the current user-selected thread to > thread 4. > >> -interpreter-exec --thread 3 console "thread 5" > > Similar. The last point is replaced by: > > . The "thread 5" implementation switches the user-visible > thread to thread 5. > >> -interpreter-exec --thread 1 console "c" > > and then thread 3 hits a breakpoint. > > This is similar too. The last point is replaced by: > > . normal_stop switches the user-visible thread to thread 3. Ok, I kinda had the same design idea, but it was blurry. Now that you explain it (I didn't know what previous_inferior_ptid was), it's clear. I'll try to prototype it quickly to see if it's viable. > I think this might also pave the way to optionally make each UI have > its own independently selected thread. (That would also solve these > problems, I guess, though it bring in a set of its own problems. > I think pulling that off would be a too large a change to make at > this point. A frontend that would want to keep CLI and MI in sync > would do that explicitly, by reacting to =thread-selected etc. events, which > would be augmented to indicate the originating UI, and switching MI's > selected thread accordingly. But certainly we'd need to make selected > frame at least be per-UI as well, and who knows what else.) We thought about that too for the future. Not only =thread-selected events need to include the originating UI, but the -thread-select command needs to be able to change the thread for another UI than the current one, so that when you click on a thread in the UI, the front-end (MI) can change the thread for the CLI. Thanks! Simon
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index b1cbd8b..3966b91 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -2163,18 +2163,9 @@ mi_execute_command (const char *cmd, int from_tty) = (struct mi_interp *) top_level_interpreter_data (); int report_change = 0; - if (command->thread == -1) - { - report_change = (!ptid_equal (previous_ptid, null_ptid) - && !ptid_equal (inferior_ptid, previous_ptid) - && !ptid_equal (inferior_ptid, null_ptid)); - } - else if (!ptid_equal (inferior_ptid, null_ptid)) - { - struct thread_info *ti = inferior_thread (); - - report_change = (ti->global_num != command->thread); - } + report_change = (!ptid_equal (previous_ptid, null_ptid) + && !ptid_equal (inferior_ptid, previous_ptid) + && !ptid_equal (inferior_ptid, null_ptid)); if (report_change) { @@ -2224,6 +2215,8 @@ mi_cmd_execute (struct mi_parse *parse) if (!inf) error (_("Invalid thread group for the --thread-group option")); + make_cleanup_restore_current_thread (); + set_current_inferior (inf); /* This behaviour means that if --thread-group option identifies an inferior with multiple threads, then a random one will be @@ -2246,6 +2239,8 @@ mi_cmd_execute (struct mi_parse *parse) if (is_exited (tp->ptid)) error (_("Thread id: %d has terminated"), parse->thread); + make_cleanup_restore_current_thread (); + switch_to_thread (tp->ptid); } @@ -2302,6 +2297,7 @@ mi_cmd_execute (struct mi_parse *parse) make_cleanup_ui_file_delete (stb); error_stream (stb); } + do_cleanups (cleanup); }