From patchwork Mon May 20 08:44:27 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Alan Hayward X-Patchwork-Id: 32763 Received: (qmail 101985 invoked by alias); 20 May 2019 08:44:34 -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 101832 invoked by uid 89); 20 May 2019 08:44:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=-24.1 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 autolearn=ham version=3.3.1 spammy=don, don=e2, 46, 7?= X-HELO: EUR04-VI1-obe.outbound.protection.outlook.com Received: from mail-eopbgr80055.outbound.protection.outlook.com (HELO EUR04-VI1-obe.outbound.protection.outlook.com) (40.107.8.55) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 20 May 2019 08:44:31 +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=5Aia2NikeFh3Lpb1zWojCLUzaF0lsUws9gmJeT0txqQ=; b=Xu2vfLicGtVOnafgy2QK4RIm5y8/r2mzkIwmlyCPkoWv5kJqszFI34MjRyMuyqgb145PtFhwT3L/WSLtN8Gxqf/V1sJiLoUzkX8Z7MglmG16GFgSj6O4DY8PV7FefeIv9FnrqCK2IxiOpi2dJCvbZ1FioR/z1NN8qpJuno5/QpU= Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com (10.172.227.22) by DB6PR0802MB2184.eurprd08.prod.outlook.com (10.172.226.146) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1900.17; Mon, 20 May 2019 08:44:28 +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.1900.020; Mon, 20 May 2019 08:44:28 +0000 From: Alan Hayward To: Andrew Burgess CC: Andreas Schwab , "gdb-patches@sourceware.org" , nd , Pedro Alves Subject: Re: [PATCH] Supress SIGTTOU when handling errors Date: Mon, 20 May 2019 08:44:27 +0000 Message-ID: <2DDEE8DB-726F-466B-AB69-593351102ECB@arm.com> References: <20190516155150.71826-1-alan.hayward@arm.com> <87y333ev6j.fsf@igel.home> <20190519220622.GB2568@embecosm.com> In-Reply-To: <20190519220622.GB2568@embecosm.com> authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alan.Hayward@arm.com; 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: <49BB6557E892B641BFE2212293C30DB0@eurprd08.prod.outlook.com> MIME-Version: 1.0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-IsSubscribed: yes > On 19 May 2019, at 23:06, Andrew Burgess wrote: > > * Andreas Schwab [2019-05-18 15:41:56 +0200]: > >> On Mai 16 2019, Alan Hayward wrote: >> >>> [I've seen this on and off over many months on AArch64 and Arm, and am >>> assuming it isn't the intended behaviour. Not sure if this should be at >>> tcdrain or it should be done at a higher level - eg in the terminal >>> handling code] >>> >>> Calls to error () can cause SIGTTOU to send gdb to the background. >>> >>> For example, on an Arm build: >>> (gdb) b main >>> Breakpoint 1 at 0x10774: file /build/gdb/testsuite/../../../src/binutils-gdb/gdb/testsuite/gdb.base/watchpoint.c, line 174. >>> (gdb) r >>> Starting program: /build/gdb/testsuite/outputs/gdb.base/watchpoint/watchpoint >>> >>> [1]+ Stopped ../gdb ./outputs/gdb.base/watchpoint/watchpoint >> >> e671cd59d74cec9f53e110ce887128d1eeadb7f2 is the first bad commit >> commit e671cd59d74cec9f53e110ce887128d1eeadb7f2 >> Author: Pedro Alves >> Date: Tue Jan 30 14:23:51 2018 +0000 >> >> Per-inferior target_terminal state, fix PR gdb/13211, more >> >> Andreas. > > Andreas, > > Thanks for tracking this down. +1 > > It appears that the change in this patch that seems to be responsible > would correspond to Alan's patch #2 option. > > I wonder if we should just apply something like the below to revert > part of Pedro's patch? This will fix this problems we're seeing (as > Alan already pointed out) as this effectively makes 'ours_for_output > ()' the same as 'ours ()' again. > > My concern would be whether there's going to be some place in GDB that > calls 'ours_for_output ()' and assumes Ctrl-C / Ctrl-Z will be > automatically passed to the inferior. This change means they are now > passed to GDB instead, will GDB always forward these to the inferior > correctly? I’m wary about changing the behaviour of ours_for_output for everyone. With patch #2 / your version, then it’s making ::ours_for_output meaningless because it’s just the same as ::ours. Looking around the code, ::ours_for_output is only(?) used directly before printing to the terminal. It looks like print_flush is the only case where output is flushed before printing. Therefore is this just a edge case? - ours_for_output works fine as long as you don’t want to flush. print_flush uses scoped_restore_terminal_state, so that means the terminal state is restored at the end of the function. I’m wondering then, if this version is better. Only use ::ours around the flush, and then switch to ours_for_output for the printing. > > Thanks, > Andrew > > > > -- > > diff --git a/gdb/inflow.c b/gdb/inflow.c > index 339b55c0bc6..6ed22c14b6b 100644 > --- a/gdb/inflow.c > +++ b/gdb/inflow.c > @@ -506,10 +506,11 @@ child_terminal_ours_1 (target_terminal_state desired_state) > /* Set tty state to our_ttystate. */ > serial_set_tty_state (stdin_serial, our_terminal_info.ttystate); > > - /* If we only want output, then leave the inferior's pgrp in the > - foreground, so that Ctrl-C/Ctrl-Z reach the inferior > - directly. */ > - if (job_control && desired_state == target_terminal_state::is_ours) > + /* If might be tempting to think that we can leave the inferior's > + pgrp in the foreground if we only want ours_for_output, however, > + calls to tcdrain within GDB will result in SIGTTOU unless GDB's > + process group is in the foreground. */ > + if (job_control) > { > #ifdef HAVE_TERMIOS_H > result = tcsetpgrp (0, our_terminal_info.process_group); > @@ -526,7 +527,7 @@ child_terminal_ours_1 (target_terminal_state desired_state) > #endif /* termios */ > } > > - if (!job_control && desired_state == target_terminal_state::is_ours) > + if (!job_control) > { > signal (SIGINT, sigint_ours); > #ifdef SIGQUIT diff --git a/gdb/exceptions.c b/gdb/exceptions.c index ebdc71d98d..d4e3197d21 100644 --- a/gdb/exceptions.c +++ b/gdb/exceptions.c @@ -46,7 +46,9 @@ print_flush (void) if (current_top_target () != NULL && target_supports_terminal_ours ()) { term_state.emplace (); - target_terminal::ours_for_output (); + /* Use ::ours instead of ::ours_for_output to prevent a SIGTTOU during the + flush. */ + target_terminal::ours (); } /* We want all output to appear now, before we print the error. We @@ -70,6 +72,13 @@ print_flush (void) serial_un_fdopen (gdb_stdout_serial); } + /* Now that the output has been flushed, switch to ::ours_for_output. */ + if (current_top_target () != NULL && target_supports_terminal_ours ()) + { + term_state.emplace (); + target_terminal::ours_for_output (); + } + annotate_error_begin (); }