From patchwork Fri May 24 08:54:50 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Alan Hayward X-Patchwork-Id: 32842 Received: (qmail 25149 invoked by alias); 24 May 2019 08:54:56 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 25141 invoked by uid 89); 24 May 2019 08:54:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, MIME_BASE64_BLANKS, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS, UNSUBSCRIBE_BODY autolearn=ham version=3.3.1 spammy=claiming, pumps, serial, wonders X-HELO: EUR04-DB3-obe.outbound.protection.outlook.com Received: from mail-eopbgr60052.outbound.protection.outlook.com (HELO EUR04-DB3-obe.outbound.protection.outlook.com) (40.107.6.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 May 2019 08:54:54 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=XNaOhr5Z9ClJHS3O9N1e1X2VJiWzU9+Ft9O3Yb6pgmQ=; b=HmaoloyF8A1HyxsuYWaAIFV0prqhxx9jSDojR7+GAWuGpNVyzFNCvE3loe31LJ1sZYb77AiDuHbTmooURzqDsJ9ZMct/leYOJaL0y3RME4eDK6NazWbsvb2zXid7bksYgMXwyhbdf8nUoigfr8bf7O5w7QfoZgz0sBHYWzRm+5o= Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com (10.172.227.22) by DB6PR0802MB2488.eurprd08.prod.outlook.com (10.172.251.147) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1922.17; Fri, 24 May 2019 08:54:50 +0000 Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com ([fe80::8c26:bb4b:6c93:9d40]) by DB6PR0802MB2133.eurprd08.prod.outlook.com ([fe80::8c26:bb4b:6c93:9d40%2]) with mapi id 15.20.1922.017; Fri, 24 May 2019 08:54:50 +0000 From: Alan Hayward To: Pedro Alves CC: Andrew Burgess , "gdb-patches@sourceware.org" , nd Subject: Re: [PATCH] Supress SIGTTOU when handling errors Date: Fri, 24 May 2019 08:54:50 +0000 Message-ID: <3935437B-CD4F-4474-B84A-05859CE822DF@arm.com> References: <20190516155150.71826-1-alan.hayward@arm.com> <20190516180640.GS2568@embecosm.com> In-Reply-To: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alan.Hayward@arm.com; x-ms-exchange-purlcount: 2 x-ms-oob-tlc-oobclassifiers: OLM:10000; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 Content-ID: <7E1B459A735C634BA17AEA410F656F0C@eurprd08.prod.outlook.com> MIME-Version: 1.0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: Alan.Hayward@arm.com X-IsSubscribed: yes > On 23 May 2019, at 21:32, Pedro Alves wrote: > > On 5/16/19 7:06 PM, Andrew Burgess wrote: > >> As I understand it the problem you're seeing could be resolved by >> making sure that the terminal is correctly claimed by GDB at the point >> tcdrain is called. This would require a call to either >> 'target_terminal::ours ()' or 'target_terminal::ours_for_output ()'. >> >> I've made several attempts to fix this in the past (non posted >> though), one problem I've previously run into that I've not yet >> understood is that calling ::ours_for_output doesn't seem to be enough >> to prevent the SIGTTOU (I assume from the tcdrain) and only calling >> ::ours is enough. >> >> What I've been trying to figure out is what the intended difference >> between ::ours_for_output and ::ours actually is, > > The point of ours_for_output is that while the inferior is still > running, leave it in the foreground, such that gdb doesn't interfere > with the terminal mode, Ctrl-C reaches the inferior directly, or such > that any keyboard/stdin input typed by the user goes to the inferior > directly, not to gdb. The latter is particularly important for a > follow up series I was working on but never got a chance to > submit, here: > > https://github.com/palves/gdb/commits/palves/tty-always-separate-session > > With that branch, gdb always puts the inferior in a separate session, > and then pumps stdin/stdout between the inferior's tty and gdb's tty > at the right times. That branch solves problems like not being able > to Ctrl-C while the inferior is blocked in sigwait with SIGINT blocked > (gdb/14559, gdb/9425). That's the fix I mentioned in the commit log > of e671cd59d74c. Anyway, with that branch, if we switch to ours() while > the inferior is running in the foreground, then we miss forwarding the > input to the inferior. See: > > https://github.com/palves/gdb/blob/d3392b83086b0a541aa31fcff301281da7a73a0e/gdb/inflow.c#L762 > > Also, if we miss calling ours_for_output(), then we print output to the > terminal in raw mode. What I'm saying is that that branch/fix exposes > more latent bugs around incorrect ours()/ours_for_output()/inferior() > handling, and the branch includes fixes in that direction, e.g.: the > "target_terminal::ours_for_output(): received SIGINT" patch. > > This is not unlike what old comment in remote.c suggests we could > do, but for local debugging: > > static void > remote_terminal_inferior (struct target_ops *self) > { > /* NOTE: At this point we could also register our selves as the > recipient of all input. Any characters typed could then be > passed on down to the target. */ > } > > >> if we can't always >> write output after calling ::ours_for_output. Part of me wonders if >> we should just switch to using ::ours in all cases.... > > I'm thinking that Alan's original patch to disable SIGTTOU is correct. > When we're in ours_for_output mode, we should be able to write to the > terminal, and we can. But, there are a couple functions that raise > a SIGTTOU if you write to the controlling terminal while in the > background, and your terminal has the TOSTOP attribute set, and tcdrain > is one of them. Looking back at my original patch again, I’m wondering if it’s better to move the ignore higher up the call stack in print_flush, so that it’s set across both flushes: ...or if it really should be left just around the tcdrain. Not quite sure what the behaviour is on non-Linux targets. > > That isn't to say that your patch _isn't_ also correct. We have many > latent bugs around this area. Let me take a better look at that one too. > > I think that even if we get something like your patch in, then > Alan's is still correct because we can have places doing > try/catch to swallow an error but still print it with exception_print, > all while the inferior is running. Of course such spots should > call ours_for_output(), but that will run into the tcdrain issue. > Minor issue is that once my patch is in, it’ll hide the missing ours_for_output bugs (?) > >> >> Anyway, I think most of the problems you're seeing should be fixed by >> claiming the terminal either permanently (calling ::ours or >> ::ours_for_output) or temporarily using >> target_terminal::scoped_restore_terminal_state. >> >> Like I said, I'm not an expert on this code, so maybe I've >> misunderstood the problem you're solving. >> > > Thanks, > Pedro Alves diff --git a/gdb/exceptions.c b/gdb/exceptions.c index ebdc71d98d..cba6d78b0b 100644 --- a/gdb/exceptions.c +++ b/gdb/exceptions.c @@ -32,40 +32,44 @@ static void print_flush (void) { struct ui *ui = current_ui; struct serial *gdb_stdout_serial; if (deprecated_error_begin_hook) deprecated_error_begin_hook (); gdb::optional term_state; /* While normally there's always something pushed on the target stack, the NULL check is needed here because we can get here very early during startup, before the target stack is first initialized. */ if (current_top_target () != NULL && target_supports_terminal_ours ()) { term_state.emplace (); target_terminal::ours_for_output (); } + /* Ignore SIGTTOU which may occur during the drain due to the terminal being + in the background. */ + scoped_ignore_sigttou ignore_sigttou; + /* We want all output to appear now, before we print the error. We have 3 levels of buffering we have to flush (it's possible that some of these should be changed to flush the lower-level ones too): */ /* 1. The _filtered buffer. */ if (filtered_printing_initialized ()) wrap_here (""); /* 2. The stdio buffer. */ gdb_flush (gdb_stdout); gdb_flush (gdb_stderr); /* 3. The system-level buffer. */ gdb_stdout_serial = serial_fdopen (fileno (ui->outstream)); if (gdb_stdout_serial) { serial_drain_output (gdb_stdout_serial); serial_un_fdopen (gdb_stdout_serial); }