From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 125809 invoked by alias); 18 May 2019 09:10:10 -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 125798 invoked by uid 89); 18 May 2019 09:10:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.1 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=HX-Spam-Relays-External:209.85.128.66, H*RU:209.85.128.66, grab, misunderstood X-HELO: mail-wm1-f66.google.com Received: from mail-wm1-f66.google.com (HELO mail-wm1-f66.google.com) (209.85.128.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 18 May 2019 09:10:08 +0000 Received: by mail-wm1-f66.google.com with SMTP id f204so8959767wme.0 for ; Sat, 18 May 2019 02:10:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=Nc+k0bU6ulDjvmCEjLOOvGDJ/pmt6A7R5gmCPYXND94=; b=YZf+de1sAUU1H+4/yGHlHu3cijih/BHq5uUolLX7ohJufLQAwF3QYUvuIWMHYSuAx7 LihAUP7oRLt7RkG3rxGVtOnOOzffUIwPunKvdgSUJ9sNRlM5KkW0eLfhaeVrNL0egsjp W74UJiNvHbG0YjZtX3r/0O16PbDGzVJXYSzeNVeDSoXyur3QTg119rgrQpJKzqZpqrCT RZVjhSncD5dk3Xm37U+TlvUYkt+N2ChX6gX8gsb7QhzFpbssdJ8ICEpy9p76BqVE2qH1 /qNXnP7DC56Q6yH2JV/wQFMpUVqyn+kLtHPdJZ0vy4U/70CDnk37EqBatfn8TAuekxU0 nHrw== Return-Path: Received: from localhost (host86-180-62-212.range86-180.btcentralplus.com. [86.180.62.212]) by smtp.gmail.com with ESMTPSA id 7sm12871911wro.85.2019.05.18.02.10.05 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 18 May 2019 02:10:05 -0700 (PDT) Date: Sat, 18 May 2019 09:10:00 -0000 From: Andrew Burgess To: Alan Hayward Cc: "gdb-patches@sourceware.org" , nd Subject: Re: [PATCH] Supress SIGTTOU when handling errors Message-ID: <20190518091004.GA2568@embecosm.com> References: <20190516155150.71826-1-alan.hayward@arm.com> <20190516180640.GS2568@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: X-Fortune: Experiments must be reproducible User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg00440.txt.bz2 * Alan Hayward [2019-05-17 12:46:56 +0000]: >=20 >=20 > > On 16 May 2019, at 19:06, Andrew Burgess = wrote: > >=20 > > * Alan Hayward [2019-05-16 15:51:53 +0000]: > >=20 > >> [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] > >>=20 > >> Calls to error () can cause SIGTTOU to send gdb to the background. > >>=20 > >> For example, on an Arm build: > >> (gdb) b main > >> Breakpoint 1 at 0x10774: file /build/gdb/testsuite/../../../src/binut= ils-gdb/gdb/testsuite/gdb.base/watchpoint.c, line 174. > >> (gdb) r > >> Starting program: /build/gdb/testsuite/outputs/gdb.base/watchpoint/wa= tchpoint > >>=20 > >> [1]+ Stopped ../gdb ./outputs/gdb.base/watchpoint/wa= tchpoint > >> localhost$ fg > >> ../gdb ./outputs/gdb.base/watchpoint/watchpoint > >> Cannot parse expression `.L1199 4@r4'. > >> warning: Probes-based dynamic linker interface failed. > >> Reverting to original interface. > >>=20 > >> The SIGTTOU is raised whilst inside a syscall during the call to tcdra= in. > >> Fix is to use scoped_ignore_sigttou to ensure SIGTTOU is blocked. > >>=20 > >> In addition fix include comments - job_control is not included via > >> terminal.h > >=20 > > Maybe someone else knows this better, but this feels like the wrong > > solution to me. > >=20 > > 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 ()=E2= =80=99. >=20 > That makes sense. Wasn=E2=80=99t aware about that code before.=20 >=20 > >=20 > > 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. >=20 >=20 > Playing about with the patch you posted, I also found that ::ours prevents > the signal, but ::ours_for_output doesn=E2=80=99t. Looks like it=E2=80= =99s due to the > following tcsetpgrp not happening: >=20 > inflow.c:child_terminal_ours_1 () >=20 > if (job_control && desired_state =3D=3D target_terminal_state::is_ours) > { > #ifdef HAVE_TERMIOS_H > result =3D tcsetpgrp (0, our_terminal_info.process_group); >=20 > > What I've been trying to figure out is what the intended difference > > between ::ours_for_output and ::ours actually is, 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=E2=80=A6. >=20 > Agreed, I=E2=80=99m not sure of the intended differences here. >=20 > >=20 > > 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. >=20 > The problem with that is finding all possible error cases and ensuring > they all all fixed up. This has bothered me too. The requirement really is that we can't have a call to error when the terminal is not ours _unless_ there's a catch handler in place that will reacquire the terminal. I've been trying to figure out a good way we could test this condition, but so far I've not come up with anything good. I think you're correct - in the short term we should assume that GDB doesn't hold the above requirement, and ensure we grab the terminal at the point the error is caught / printed. >=20 > For example, my error was coming from the try/catch/exception_print > in solid-svr4.c:solib_event_probe_action () >=20 > >=20 > > Like I said, I'm not an expert on this code, so maybe I've > > misunderstood the problem you're solving. > >=20 >=20 > We=E2=80=99re definitely looking at the same issue. >=20 > Working back up the call trace from where my change was, all the error > prints first end up in exceptions.c::print_flush () which already has > a call to ::ours_for_output. >=20 > I=E2=80=99ve posted two patches below. Both of them fix all my issues, in= cluding > your GDB_FAKE_ERROR case. >=20 > If ::ours_for_output and ::ours are working as intended, then the first > patch is probably the correct fix. >=20 > But, if ::ours_for_output and ::ours are not working as intended, then > possibly more investigation is required to know why patch 2 works. >=20 > Alan. >=20 >=20 >=20 > PATCH 1: >=20 > gdb/ChangeLog: >=20 > 2019-05-17 Alan Hayward >=20 > * exceptions.c (print_flush): Use target_terminal::ours. >=20 > diff --git a/gdb/exceptions.c b/gdb/exceptions.c > index ebdc71d98d..47bfb95153 100644 > --- a/gdb/exceptions.c > +++ b/gdb/exceptions.c > @@ -46,7 +46,7 @@ print_flush (void) > if (current_top_target () !=3D NULL && target_supports_terminal_ours (= )) > { > term_state.emplace (); > - target_terminal::ours_for_output (); > + target_terminal::ours (); > } >=20 > /* We want all output to appear now, before we print the error. We >=20 I think that this is my preferred patch, though I think using 'ours' instead of the more obvious 'ours_for_output' is worth a comment. >=20 >=20 > PATCH 2: >=20 >=20 > gdb/ChangeLog: >=20 > 2019-05-17 Alan Hayward >=20 > * inflow.c (child_terminal_ours_1): Call tcsetpgrp for > is_ours_for_output. >=20 > diff --git a/gdb/inflow.c b/gdb/inflow.c > index 339b55c0bc..b376e24e86 100644 > --- a/gdb/inflow.c > +++ b/gdb/inflow.c > @@ -509,7 +509,9 @@ child_terminal_ours_1 (target_terminal_state desired_= state) > /* 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 =3D=3D target_terminal_state::is_= ours) > + if (job_control > + && (desired_state =3D=3D target_terminal_state::is_ours > + || desired_state =3D=3D target_terminal_state::is_ours_for_= output)) > { > #ifdef HAVE_TERMIOS_H >=20 >=20 The only reason I don't like this is that I don't really understand the wider impact it might have. I guess there are places in GDB where we call 'ours_for_output' and assume that Ctrl-C / Ctrl-Z will be delivered to the inferior. If these suddenly start arriving in GDB then it's not clear if we'll have the correct infrastructure in place to pass these signals on to the inferior. You should probably wait for a while to see if any terminal experts want to comment, but if nobody else comments within a week I think you should go ahead with patch #1. Thanks, Andrew