From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 104921 invoked by alias); 2 Aug 2016 17:45:37 -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 104883 invoked by uid 89); 2 Aug 2016 17:45:32 -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=kinda, Reducing, click 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; Tue, 02 Aug 2016 17:45:30 +0000 Received: from EUSAAHC004.ericsson.se (Unknown_Domain [147.117.188.84]) by (Symantec Mail Security) with SMTP id 35.F4.02568.09CD0A75; Tue, 2 Aug 2016 19:46:56 +0200 (CEST) Received: from [142.133.110.144] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.86) with Microsoft SMTP Server id 14.3.301.0; Tue, 2 Aug 2016 13:38:48 -0400 Subject: Re: [PATCH 1/2] mi: Restore original thread/frame when specifying --thread or --thread-group To: Pedro Alves , References: <20160801211401.18155-1-simon.marchi@ericsson.com> <20160801211401.18155-2-simon.marchi@ericsson.com> From: Simon Marchi Message-ID: <155e8052-19b5-a446-c9bd-f973ded3eaa3@ericsson.com> Date: Tue, 02 Aug 2016 17:45:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-08/txt/msg00037.txt.bz2 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