From patchwork Thu May 16 18:30:04 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 32724 Received: (qmail 108426 invoked by alias); 16 May 2019 18:30:32 -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 108083 invoked by uid 89); 16 May 2019 18:30:19 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.8 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=Claim, routing, Issues, owns X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 16 May 2019 18:30:09 +0000 Received: by mail-wr1-f65.google.com with SMTP id g12so4183057wro.8 for ; Thu, 16 May 2019 11:30: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:in-reply-to:user-agent; bh=wcAY+E74bwuPSWaziuagJ9UTXgQ4PhwLinL/uxYwoQU=; b=Euz3KZaH+9ZlZsMgj4LO3kGCjwk0KH508J/051Bcrr8eiHsO0Kv1dqSCGNg/iHSJwa x8h369WFPgkvZlErcFBDMI6ZmfmF/43p5KQErpjo31kDPdlS4+G3MgdqtKHL0Rz8cw9G 03MFFUj2s6gi8GAK+Ds6eE4KNRLDGUjZAkmgiUKyjFXtD+m2oHpUmtHJh/zIFL6CDfdP wIBRGQjlza0iLBtjq4AM7UZgILXAsNAfQsN843DfZK+p0OE0HmI//YOp7MqYO80fYoKd vTy/B6c5n5niGZk6c9l5DNCVd/yClghv/BoVpNXB8+4ia5TXvQCUPT1w0oLeBpe0ZIYL 1ZvQ== Return-Path: Received: from localhost (host86-180-62-212.range86-180.btcentralplus.com. [86.180.62.212]) by smtp.gmail.com with ESMTPSA id h15sm6716510wru.52.2019.05.16.11.30.05 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 16 May 2019 11:30:06 -0700 (PDT) Date: Thu, 16 May 2019 19:30:04 +0100 From: Andrew Burgess To: Alan Hayward Cc: "gdb-patches@sourceware.org" , nd Subject: Re: [PATCH] Supress SIGTTOU when handling errors Message-ID: <20190516183004.GT2568@embecosm.com> References: <20190516155150.71826-1-alan.hayward@arm.com> <20190516180640.GS2568@embecosm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190516180640.GS2568@embecosm.com> X-Fortune: In most instances, all an argument proves is that two people are present. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes 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. 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. Issues: 1. Why do I need to use target_terminal::ours instead of target_terminal::ours_for_output in event-loop.c??? 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). 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. 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??? 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); } /* 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); } } @@ -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 (). */ + target_terminal::ours (); + exception_print (gdb_stderr, ex); /* If any exception escaped to here, we better enable