From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@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 To: CC: Simon Marchi Subject: [PATCH 1/2] mi: Restore original thread/frame when specifying --thread or --thread-group Date: Mon, 01 Aug 2016 21:15:00 -0000 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 X-SW-Source: 2016-08/txt/msg00021.txt.bz2 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(-) 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); } -- 2.9.2