From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 82247 invoked by alias); 23 May 2019 20:33:44 -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 82223 invoked by uid 89); 23 May 2019 20:33:44 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=Claim, owned, foreground, ours 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; Thu, 23 May 2019 20:33:42 +0000 Received: by mail-wm1-f66.google.com with SMTP id 198so7155268wme.3 for ; Thu, 23 May 2019 13:33:42 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:4eeb:42ff:feef:f164? ([2001:8a0:f913:f700:4eeb:42ff:feef:f164]) by smtp.gmail.com with ESMTPSA id s11sm453159wrb.71.2019.05.23.13.33.39 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Thu, 23 May 2019 13:33:39 -0700 (PDT) Subject: Re: [PATCH] Supress SIGTTOU when handling errors To: Andrew Burgess , Alan Hayward References: <20190516155150.71826-1-alan.hayward@arm.com> <20190516180640.GS2568@embecosm.com> <20190516183004.GT2568@embecosm.com> Cc: "gdb-patches@sourceware.org" , nd From: Pedro Alves Message-ID: Date: Thu, 23 May 2019 20:33:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190516183004.GT2568@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-05/txt/msg00549.txt.bz2 On 5/16/19 7:30 PM, Andrew Burgess wrote: > Below is one of my attempts at looking into this issue. This isn't a > patch for merging, it's just some random hacking at this point, but it > shows what I'm thinking... > > With this patch applied set the environment variable 'GDB_FAKE_ERROR' > before starting GDB, set a breakpoint and run and you'll hit the > SIGTTOU issue. > > $ GDB_FAKE_ERROR=y ./gdb ./gdb > (gdb) b main > Breakpoint 1 at 0x410236: main. (24 locations) > (gdb) r > Starting program: .... > Warning: > Cannot insert breakpoint 1: fake error > ... > > Thanks, > Andrew > > --- > > commit 395c148903ba4a96a55c9ade4b809b9df2ccbcb8 > Author: Andrew Burgess > Date: Tue Sep 4 15:44:48 2018 +0100 > > Remove change of terminal ownership before throw > > Calling target_terminal::ours_for_output before throwing an error > seems wrong, surely the site at which the terminal is passed to the > inferior should take care of reclaiming the terminal using a RAII > object within that scope. That's not true/correct, because run control / event handle is asynchronous. You put the inferior in the foreground, resume the inferior, and then go back to the event loop. The stack frame that put the terminal in the foreground is long gone, it isn't there to restore the terminal state anymore. Instead, we (should) switch the terminal back to gdb when the target stops and/or when we give back the prompt to the user. > > If the error is caught before we reclaim the terminal _then_ we can > call target_terminal::ours_for_output, but I don't think we should be > calling this at the throw site. "I don't think we should be calling this at the throw site" Agreed with that part. > > Issues: > > 1. Why do I need to use target_terminal::ours instead of > target_terminal::ours_for_output in event-loop.c??? Because target_terminal::ours_for_output does not put gdb in the foreground -- the tcsetpgrp call in child_terminal_ours_1. > > 2. The change in event-loop.c shouldn't be there anyway, I should be > just asserting that the terminal is owned by GDB at this point, the > terminal should be returned to GDB using RAII at some other point in > the stack (presumably the same frame level as we give up the > terminal). Fully disagreed. :-) > > 3. Should be able to assert in the every output routing that GDB > owns the terminal - but this is broken so badly by our debug. Right, I tried that once too, in the context of that branch I pointed at above -- we don't switch to ours_for_output before log output. I ended up just shrugging and thinking that at least debug output is not meant for the users, so if debug output comes out a bit messed up it's not _that_ bad. > Maybe we can get GDB to automatically reclaim the terminal before > writing out each debug. I assume in some cases debug output would > not work due to not owning the terminal??? I think output should always work. If you miss an ours_for_output call, all that happens is that you end up printing in whatever terminal mode/settings the inferior configured for the terminal, instead of in gdb's mode/settings. But then again, if the inferior prints at the same time as gdb, or queries/saves the terminal settings, it'll see gdb's settings... That's another thing that branch fixes, because with that branch, gdb is in control of flushing the output from the inferior's terminal to gdb's terminal at the right times, and the inferior's terminal settings are never tweaked by gdb. > > gdb/ChangeLog: > > * breakpoint.c (update_inserted_breakpoint_locations): Remove call > to target_terminal::ours_for_output before throwing an error. > (insert_breakpoint_locations): Likewise. > (bkpt_insert_location): ***REMOVE*** Add code to raise a fake > error. > * event-loop.c: Add 'target.h' include. > (start_event_loop): Claim terminal before printing the exception. > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 5b0a9fde61f..3a5396f7725 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,13 @@ > +2019-05-12 Andrew Burgess > + > + * breakpoint.c (update_inserted_breakpoint_locations): Remove call > + to target_terminal::ours_for_output before throwing an error. > + (insert_breakpoint_locations): Likewise. > + (bkpt_insert_location): ***REMOVE*** Add code to raise a fake > + error. > + * event-loop.c: Add 'target.h' include. > + (start_event_loop): Claim terminal before printing the exception. > + > 2019-05-08 Tom Tromey > > * gdbtypes.c (objfile_type_data): Change type. > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 35da97bd041..053d75dcfd4 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -2913,10 +2913,7 @@ update_inserted_breakpoint_locations (void) > } > > if (error_flag) > - { > - target_terminal::ours_for_output (); > - error_stream (tmp_error_stream); > - } > + error_stream (tmp_error_stream); This bit is OK. > } > > /* Used when starting or continuing the program. */ > @@ -3013,7 +3010,6 @@ insert_breakpoint_locations (void) > tmp_error_stream.printf ("Could not insert hardware breakpoints:\n\ > You may have requested too many hardware breakpoints/watchpoints.\n"); > } > - target_terminal::ours_for_output (); > error_stream (tmp_error_stream); This bit is OK. > } > } > @@ -12343,6 +12339,9 @@ bkpt_insert_location (struct bp_location *bl) > { > CORE_ADDR addr = bl->target_info.reqstd_address; > > + if (getenv ("GDB_FAKE_ERROR") != NULL) > + error ("fake error"); > + > bl->target_info.kind = breakpoint_kind (bl, &addr); > bl->target_info.placed_address = addr; > > diff --git a/gdb/event-loop.c b/gdb/event-loop.c > index caeb5f38d9b..611f63b5942 100644 > --- a/gdb/event-loop.c > +++ b/gdb/event-loop.c > @@ -21,6 +21,7 @@ > #include "event-loop.h" > #include "event-top.h" > #include "ser-event.h" > +#include "target.h" > > #ifdef HAVE_POLL > #if defined (HAVE_POLL_H) > @@ -371,6 +372,13 @@ start_event_loop (void) > } > catch (const gdb_exception &ex) > { > + /* Ideally the terminal should have been restored to GDB as part > + of unwinding the stack to get back to here, but things are > + not ideal. Further, based on the name alone we should be able > + to call target_terminal::ours_for_output () here, but that's > + not enough, we need to call full target_terminal::ours (). */ This comment is not OK. > + target_terminal::ours (); See comment just below exception_print. It reads: /* If any exception escaped to here, we better enable stdin. Otherwise, any command that calls async_disable_stdin, and then throws, will leave stdin inoperable. */ Also see async_enable_stdin: /* Re-enable stdin after the end of an execution command in synchronous mode, or after an error from the target, and we aborted the exec operation. */ void async_enable_stdin (void) { struct ui *ui = current_ui; if (ui->prompt_state == PROMPT_BLOCKED) { target_terminal::ours (); ui_register_input_event_handler (ui); ui->prompt_state = PROMPT_NEEDED; } } So it seems to be that we should call async_enable_stdin _before_ calling exception_print, instead of after. Does that work? If not, why not? I would have tried it myself to confirm, but I couldn't reproduce the SIGTTOU issue with your patch, for some reason. > + > exception_print (gdb_stderr, ex); > > /* If any exception escaped to here, we better enable > Thanks, Pedro Alves