| Message ID | 20180802165357.176928-1-jon.turney@dronecode.org.uk |
|---|---|
| State | New, archived |
| Headers |
Received: (qmail 81006 invoked by alias); 2 Aug 2018 16:54:17 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 80989 invoked by uid 89); 2 Aug 2018 16:54:16 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=reproduction, observed, regressed, H*Ad:D*org.uk X-HELO: rgout03.bt.lon5.cpcloud.co.uk Received: from rgout0304.bt.lon5.cpcloud.co.uk (HELO rgout03.bt.lon5.cpcloud.co.uk) (65.20.0.210) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Aug 2018 16:54:14 +0000 X-OWM-Source-IP: 86.151.121.200 (GB) X-OWM-Env-Sender: jonturney@btinternet.com X-VadeSecure-score: verdict=clean score=0/300, class=clean X-SNCR-VADESECURE: CLEAN Received: from localhost.localdomain (86.151.121.200) by rgout03.bt.lon5.cpcloud.co.uk (9.0.019.26-1) (authenticated as jonturney@btinternet.com) id 5B4CE16C01BA77D6; Thu, 2 Aug 2018 17:54:12 +0100 From: Jon Turney <jon.turney@dronecode.org.uk> To: gdb-patches@sourceware.org Cc: Jon Turney <jon.turney@dronecode.org.uk> Subject: [PATCH] Remove a spurious target_terminal::ours() from windows_nat_target::wait() Date: Thu, 2 Aug 2018 17:53:57 +0100 Message-Id: <20180802165357.176928-1-jon.turney@dronecode.org.uk> |
Commit Message
Jon Turney
Aug. 2, 2018, 4:53 p.m. UTC
This causes the inferior to stop with SIGTTIN if it tries to read from the terminal after it has been continued. See https://cygwin.com/ml/cygwin/2016-09/msg00285.html for reproduction. Since MinGW doesn't have a tcsetpgrp(), I don't think this problem would be observed there, but Cygwin does so target_terminal::ours() will call it. Calling target_terminal::ours() here seems to be is no longer appropriate after the "Merge async and sync code paths" changes (as the inferior is now in a separate process group even in sync mode(?), which is always used on Windows targets) This call was added in commit c44537cf (and see https://sourceware.org/ml/gdb-patches/2007-02/msg00167.html for what it fixed, which is not regressed by this change) When windows_nat_target::wait() is entered, the inferior is running (either it's been just been started or attached to, or windows_continue() was called), so grabbing the controlling terminal away from it here seems to be wrong, since infrun.c takes care of calling target_terminal::ours() when the inferior stops. gdb/ChangeLog: 2018-08-02 Jon Turney <jon.turney@dronecode.org.uk> * windows-nat.c (windows_nat_target::wait): Remove a spurious target_terminal::ours(). --- gdb/windows-nat.c | 2 -- 1 file changed, 2 deletions(-)
Comments
On 2018-08-02 12:53, Jon Turney wrote: > This causes the inferior to stop with SIGTTIN if it tries to read from > the > terminal after it has been continued. > > See https://cygwin.com/ml/cygwin/2016-09/msg00285.html for > reproduction. > > Since MinGW doesn't have a tcsetpgrp(), I don't think this problem > would be > observed there, but Cygwin does so target_terminal::ours() will call > it. > > Calling target_terminal::ours() here seems to be is no longer > appropriate > after the "Merge async and sync code paths" changes (as the inferior is > now > in a separate process group even in sync mode(?), which is always used > on > Windows targets) > > This call was added in commit c44537cf (and see > https://sourceware.org/ml/gdb-patches/2007-02/msg00167.html for what it > fixed, which is not regressed by this change) > > When windows_nat_target::wait() is entered, the inferior is running > (either > it's been just been started or attached to, or windows_continue() was > called), so grabbing the controlling terminal away from it here seems > to be > wrong, since infrun.c takes care of calling target_terminal::ours() > when the > inferior stops. > > gdb/ChangeLog: > > 2018-08-02 Jon Turney <jon.turney@dronecode.org.uk> > > * windows-nat.c (windows_nat_target::wait): Remove a spurious > target_terminal::ours(). This seems good to me, but I'd rather check with Pedro, since he knows this stuff much better. Simon
On 08/07/2018 03:45 AM, Simon Marchi wrote: > On 2018-08-02 12:53, Jon Turney wrote: >> This causes the inferior to stop with SIGTTIN if it tries to read from the >> terminal after it has been continued. >> >> See https://cygwin.com/ml/cygwin/2016-09/msg00285.html for reproduction. >> >> Since MinGW doesn't have a tcsetpgrp(), I don't think this problem would be >> observed there, but Cygwin does so target_terminal::ours() will call it. >> >> Calling target_terminal::ours() here seems to be is no longer appropriate >> after the "Merge async and sync code paths" changes (as the inferior is now >> in a separate process group even in sync mode(?), which is always used on >> Windows targets) I don't really understand what the async/sync code paths changes here, but regardless ... >> >> This call was added in commit c44537cf (and see >> https://sourceware.org/ml/gdb-patches/2007-02/msg00167.html for what it >> fixed, which is not regressed by this change) >> >> When windows_nat_target::wait() is entered, the inferior is running (either >> it's been just been started or attached to, or windows_continue() was >> called), so grabbing the controlling terminal away from it here seems to be >> wrong, since infrun.c takes care of calling target_terminal::ours() when the >> inferior stops. >> >> gdb/ChangeLog: >> >> 2018-08-02 Jon Turney <jon.turney@dronecode.org.uk> >> >> * windows-nat.c (windows_nat_target::wait): Remove a spurious >> target_terminal::ours(). > > This seems good to me, but I'd rather check with Pedro, since he knows this stuff much better. ... this LGTM. Thanks, Pedro Alves
On 09/08/2018 14:23, Pedro Alves wrote: > On 08/07/2018 03:45 AM, Simon Marchi wrote: >> On 2018-08-02 12:53, Jon Turney wrote: >>> This causes the inferior to stop with SIGTTIN if it tries to read from the >>> terminal after it has been continued. >>> >>> See https://cygwin.com/ml/cygwin/2016-09/msg00285.html for reproduction. >>> >>> Since MinGW doesn't have a tcsetpgrp(), I don't think this problem would be >>> observed there, but Cygwin does so target_terminal::ours() will call it. >>> >>> Calling target_terminal::ours() here seems to be is no longer appropriate >>> after the "Merge async and sync code paths" changes (as the inferior is now >>> in a separate process group even in sync mode(?), which is always used on >>> Windows targets) > > I don't really understand what the async/sync code paths changes > here, but regardless ... Me neither. Ideally I would have noticed this regression closer to the time those changes were made, but... >>> This call was added in commit c44537cf (and see >>> https://sourceware.org/ml/gdb-patches/2007-02/msg00167.html for what it >>> fixed, which is not regressed by this change) >>> >>> When windows_nat_target::wait() is entered, the inferior is running (either >>> it's been just been started or attached to, or windows_continue() was >>> called), so grabbing the controlling terminal away from it here seems to be >>> wrong, since infrun.c takes care of calling target_terminal::ours() when the >>> inferior stops. >>> >>> gdb/ChangeLog: >>> >>> 2018-08-02 Jon Turney <jon.turney@dronecode.org.uk> >>> >>> * windows-nat.c (windows_nat_target::wait): Remove a spurious >>> target_terminal::ours(). >> >> This seems good to me, but I'd rather check with Pedro, since he knows this stuff much better. > > ... this LGTM. Thanks. 0c0a40e0ab..a44294f5ed master -> master
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c index aea502638e..40cc224805 100644 --- a/gdb/windows-nat.c +++ b/gdb/windows-nat.c @@ -1703,8 +1703,6 @@ windows_nat_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus, { int pid = -1; - target_terminal::ours (); - /* We loop when we get a non-standard exception rather than return with a SPURIOUS because resume can try and step or modify things, which needs a current_thread->h. But some of these exceptions mark