From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23559 invoked by alias); 7 Jul 2014 16:39:24 -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 23536 invoked by uid 89); 7 Jul 2014 16:39:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qa0-f73.google.com Received: from mail-qa0-f73.google.com (HELO mail-qa0-f73.google.com) (209.85.216.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 07 Jul 2014 16:39:20 +0000 Received: by mail-qa0-f73.google.com with SMTP id m5so636346qaj.2 for ; Mon, 07 Jul 2014 09:39:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:mime-version:content-type :content-transfer-encoding:message-id:date:to:cc:subject:in-reply-to :references; bh=uiGslCfmq7i66YBJ8OfktBuPuT/VveCydwDk8oQgDws=; b=RjmlVzar8IHjeItISLXkdQEAxxCwFTckp68rqgQOmO+Mc6QMmubg/P1xkSzORRAXlZ uIplXqrUDMLCJX/HpAEJFQXDzOYF9Ub3JjY5n30w82BXG/dAV1KPgYaojW5MtXppnnH8 5h9TeLfAQpqdEmOganCZ1hbu9lWWpmQ2r98PEN5wrC0ngrQ4RPQIklUH5TO/KLOAVNh6 065Q5oFlXzkQYmPPSlSFVf4c35sUurTF4bX00hDFCfM9AyEPHjmOC1EZOOaWEjn6r9fT df9K6FzcX+F2YrFdtZkjAcj7Z/P8OKWAk2FwyQwmslZmKazo1BGKxDrfn7Ru6UHNhgvg agWQ== X-Gm-Message-State: ALoCoQkL0wmX/sP5o7GW9RqxY20mB7LhWcHc9e59eQaxDHvaYcQk3VPOfGrSxJR3LxFoCVTgdDKr X-Received: by 10.58.152.143 with SMTP id uy15mr15051968veb.27.1404751158114; Mon, 07 Jul 2014 09:39:18 -0700 (PDT) Received: from corp2gmr1-1.hot.corp.google.com (corp2gmr1-1.hot.corp.google.com [172.24.189.92]) by gmr-mx.google.com with ESMTPS id j43si2338000yhh.5.2014.07.07.09.39.18 for (version=TLSv1.1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 07 Jul 2014 09:39:18 -0700 (PDT) Received: from ruffy.mtv.corp.google.com (ruffy.mtv.corp.google.com [172.17.128.44]) by corp2gmr1-1.hot.corp.google.com (Postfix) with ESMTP id 4371731C3BF; Mon, 7 Jul 2014 09:39:17 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <21434.52532.737427.778289@ruffy.mtv.corp.google.com> Date: Mon, 07 Jul 2014 16:39:00 -0000 To: Pedro Alves Cc: Mark Wielaard , Jan Kratochvil , gdb-patches@sourceware.org Subject: Re: [PATCH] Fix "attach" command vs user input race [Re: Regression for attach from stdin [Re: [pushed] Re: [PATCH v6 0/2] enable target-async by default]] In-Reply-To: <53B6B0B8.2050702@redhat.com> References: <1400878753-24688-1-git-send-email-palves@redhat.com> <538739A2.2050105@redhat.com> <20140701162830.GA25877@host2.jankratochvil.net> <1404291574.3766.35.camel@bordewijk.wildebeest.org> <53B3CDCC.9050502@redhat.com> <53B57911.10304@redhat.com> <53B6B0B8.2050702@redhat.com> X-IsSubscribed: yes X-SW-Source: 2014-07/txt/msg00126.txt.bz2 Pedro Alves writes: > gdb/ > 2014-07-04 Pedro Alves > > * infcmd.c (attach_command_post_wait): Don't call > target_terminal_inferior here. > (attach_command): Call it here instead. > [...] > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index c4bb401..76ab12b 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -2381,9 +2381,6 @@ attach_command_post_wait (char *args, int from_tty, int async_exec) > > post_create_inferior (¤t_target, from_tty); > > - /* Install inferior's terminal modes. */ > - target_terminal_inferior (); > - > if (async_exec) > { > /* The user requested an `attach&', so be sure to leave threads > @@ -2495,9 +2492,11 @@ attach_command (char *args, int from_tty) > shouldn't refer to attach_target again. */ > attach_target = NULL; > > - /* Set up the "saved terminal modes" of the inferior > - based on what modes we are starting it with. */ > + /* Set up the "saved terminal modes" of the inferior based on what > + modes we are starting it with, and remove stdin from the event > + loop. */ > target_terminal_init (); > + target_terminal_inferior (); > > /* Set up execution context to know that we should return from > wait_for_inferior as soon as the target reports a stop. */ Our nomenclature here is problematic. I always do a double take when I see attach and target_terminal_foo mentioned in the same sentence. Bleah. For the case at hand, and at least until we have something more readable, can I ask for a slight change here? target_terminal_inferior can do a lot more than just "remove stdin from the event loop", thus as a reader I'm left being unsure if there aren't still more bugs here. Plus, while I can see how to map the comment to the code in the patch, "Set up the "saved terminal modes" of the inferior based on what modes we are starting it with" --> target_terminal_init (); and "remove stdin from the event loop" --> target_terminal_inferior (); If I look at the result after the patch, combining the two steps into one sentence doesn't help clarify things given that target_terminal_inferior can do more than just "remove stdin from the event loop". E.g., this is a marginal improvement: /* Set up the "saved terminal modes" of the inferior based on what modes we are starting it with. */ target_terminal_init (); /* And remove stdin from the event loop. */ target_terminal_inferior (); But I'm hoping for more text in the second comment to explain things better.