From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21610 invoked by alias); 26 Jun 2008 13:33:16 -0000 Received: (qmail 21601 invoked by uid 22791); 26 Jun 2008 13:33:15 -0000 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 26 Jun 2008 13:32:54 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id 37D7698366; Thu, 26 Jun 2008 13:32:52 +0000 (GMT) Received: from caradoc.them.org (22.svnf5.xdsl.nauticom.net [209.195.183.55]) by nan.false.org (Postfix) with ESMTP id 1EA559835A; Thu, 26 Jun 2008 13:32:52 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.69) (envelope-from ) id 1KBral-0006YI-7A; Thu, 26 Jun 2008 09:32:51 -0400 Date: Thu, 26 Jun 2008 13:41:00 -0000 From: Daniel Jacobowitz To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [non-stop] 10/10 split user/internal threads Message-ID: <20080626133251.GA22726@caradoc.them.org> Mail-Followup-To: Pedro Alves , gdb-patches@sourceware.org References: <200806152209.45049.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <200806152209.45049.pedro@codesourcery.com> User-Agent: Mutt/1.5.17 (2008-05-11) X-IsSubscribed: yes 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 X-SW-Source: 2008-06/txt/msg00467.txt.bz2 On Sun, Jun 15, 2008 at 10:09:44PM +0100, Pedro Alves wrote: > I've added three points where GDB makes the internal (inferior_ptid) > thread switch to the user selected thread (user_selected_ptid): > >  1 - execute_command, before executing a command >  2 - execute_command, after executing a command, >  3 - fetch_inferior_event, after handling the event. > > With these in place, the user never notices when GDB switches > between threads internally while handing events. The _internal name is a warning sign to me: it means we have two functions that conceptually do the same thing, but one of them is only "safe for internal use". We call both of them from internal to GDB, so let's try to find a better name. How about execute_command_in_current_context? Is -interpreter-exec ever going to get a --thread argument? We have to be careful because it currently restores the globally selected thread via cli_interpreter_exec, and even if it didn't it may call a user defined command. There are 22 calls to execute_command in GDB (including two from Insight). This patch changes seven of them to execute_command_internal: thread.c, mi-main.c, mi-cmd-env.c. I checked all the others, just in case - they mostly look correct. The only cases I'm worried about are breakpoint commands and conditions. For instance bpstat_do_actions to execute_control_command to execute_command will mess with inferior_ptid. Should it? What about bpstat_check_breakpoint_conditions to breakpoint_cond_eval to evaluate_expression? Also, hook- and hookpost- commands will still be run after execute_command_internal - but every call to execute_command_internal manually sets the appropriate thread, so this looks fine. > +enum thread_state > +{ > + THREAD_STOPPED, > + THREAD_RUNNING, > + THREAD_EXITED, > +}; > + > +static enum thread_state main_thread_state = 0; THREAD_STOPPED instead of 0? > - /* Backup current thread and selected frame. */ > - if (!is_running (inferior_ptid)) > - saved_frame_id = get_frame_id (get_selected_frame (NULL)); > - else > - saved_frame_id = null_frame_id; > - > - old_chain = make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id); Is this sort of simplification really a good idea? Functions which leave inferior_ptid with an essentially unspecified value, while it's still global state used by most of GDB. > +/* Only safe to use this cleanup on a stopped thread, and > + inferior_ptid isn't ran before running the cleanup. */ > + > struct current_thread_cleanup Not sure what you mean by this comment. > + /* In non-stop mode, we don't want GDB to switch threads on the > + users back, to avoid races where the user is typing a command to user's -- Daniel Jacobowitz CodeSourcery