From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 97039 invoked by alias); 2 Aug 2016 14:49:47 -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 97024 invoked by uid 89); 2 Aug 2016 14:49:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=nowadays, feet, brain, spread X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 02 Aug 2016 14:49:44 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 88E344E03C; Tue, 2 Aug 2016 14:49:43 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u72EnfrZ006372; Tue, 2 Aug 2016 10:49:42 -0400 Subject: Re: [PATCH 1/2] mi: Restore original thread/frame when specifying --thread or --thread-group To: Simon Marchi , gdb-patches@sourceware.org References: <20160801211401.18155-1-simon.marchi@ericsson.com> <20160801211401.18155-2-simon.marchi@ericsson.com> From: Pedro Alves Message-ID: Date: Tue, 02 Aug 2016 14:49:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160801211401.18155-2-simon.marchi@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-08/txt/msg00031.txt.bz2 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