Message ID | 20190109141020.GA3456@embecosm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 90689 invoked by alias); 9 Jan 2019 14:10:28 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 90651 invoked by uid 89); 9 Jan 2019 14:10:27 -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, SPF_PASS autolearn=ham version=3.3.2 spammy= 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; Wed, 09 Jan 2019 14:10:25 +0000 Received: by mail-wr1-f65.google.com with SMTP id t27so7798790wra.6 for <gdb-patches@sourceware.org>; Wed, 09 Jan 2019 06:10:24 -0800 (PST) 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=Fxcghn6gQ+qHczTsO1fvZ1gZCfvfcglW8h+Rpglqdtg=; b=hgwr/GTUpWCefi9sghtHSDW8K47hJATs2Pyt+3wQL20lpElKwKYZtTaR/9fN+i8VAG 1eILbJZuADRkjQLmUXufn+nXyEcF9wwl2ySK1Tyzt0AZRZDs//4lBHIkrEHVK8/IMZYG Da+XlsPbJ2bZ2+5Ia2+3APndKjffWKkFIGIhqkbUXfzLgoTPLgEDH0PUsqnF+JxCin4k F9H7ezI2Qv5KtVnC8j5VIwlgOjaUeiWUb++cICId/qq+kCh9LcI9IsN6DaxoMar1oC/+ PCVIkWj8H61xvZWsDYWuVjbP1Ol2Z8FSO3tXDrrM+Kw29uAV+eUTmCZqbvOYONQzFFHz 322Q== Return-Path: <andrew.burgess@embecosm.com> Received: from localhost (cust64-dsl91-135-5.idnet.net. [91.135.5.64]) by smtp.gmail.com with ESMTPSA id h10sm11963448wmf.44.2019.01.09.06.10.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 09 Jan 2019 06:10:22 -0800 (PST) Date: Wed, 9 Jan 2019 14:10:21 +0000 From: Andrew Burgess <andrew.burgess@embecosm.com> To: Pedro Alves <palves@redhat.com> Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 5/6] gdb: Remove cleanup from linux-fork.c:inferior_call_waitpid Message-ID: <20190109141020.GA3456@embecosm.com> References: <cover.1546382416.git.andrew.burgess@embecosm.com> <93fd47987a3ab8907c4745e4054d622b5122cede.1546382416.git.andrew.burgess@embecosm.com> <bc587b9c-3af4-1879-3f04-0d8ba18522f8@redhat.com> <20190109133323.GZ3456@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190109133323.GZ3456@embecosm.com> X-Fortune: What is the sound of one hand clapping? X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes |
Commit Message
Andrew Burgess
Jan. 9, 2019, 2:10 p.m. UTC
* Andrew Burgess <andrew.burgess@embecosm.com> [2019-01-09 13:33:23 +0000]: > * Pedro Alves <palves@redhat.com> [2019-01-09 12:54:59 +0000]: > > > On 01/01/2019 10:45 PM, Andrew Burgess wrote: > > > > > +/* Temporarily switch to the infrun state stored on the fork_info > > > + identified by a given ptid_t. When this object goes out of scope, > > > + restore the currently selected infrun state. */ > > > + > > > +class scoped_switch_fork_info > > > { > > > - struct fork_info *oldfp = (struct fork_info *) fp; > > > +public: > > > + /* Switch to the infrun state held on the fork_info identified by > > > + PPTID. If PPTID is the current inferior then no switch is done. */ > > > + scoped_switch_fork_info (ptid_t pptid) > > > > (Nit, "explicit scoped_switch_fork_info") > > > > > + : m_oldfp (nullptr) > > > + { > > > + if (pptid != inferior_ptid) > > > + { > > > + struct fork_info *newfp = nullptr; > > > + > > > + /* Switch to pptid. */ > > > + m_oldfp = find_fork_ptid (inferior_ptid); > > > + gdb_assert (m_oldfp != nullptr); > > > + newfp = find_fork_ptid (pptid); > > > + gdb_assert (newfp != nullptr); > > > + fork_save_infrun_state (m_oldfp, 1); > > > + remove_breakpoints (); > > > + fork_load_infrun_state (newfp); > > > + insert_breakpoints (); > > > + } > > > + } > > > > > > - if (oldfp) > > > - { > > > - /* Switch back to inferior_ptid. */ > > > - remove_breakpoints (); > > > - fork_load_infrun_state (oldfp); > > > - insert_breakpoints (); > > > - } > > > -} > > > + /* Restore the previously selected infrun state. If the constructor > > > + didn't need to switch states, then nothing is done here either. */ > > > + ~scoped_switch_fork_info () > > > + { > > > + if (m_oldfp != nullptr) > > > + { > > > + /* Switch back to inferior_ptid. */ > > > + remove_breakpoints (); > > > + fork_load_infrun_state (m_oldfp); > > > + insert_breakpoints (); > > > + } > > > > When writing destructors, we need to keep in mind that if an > > exception escapes, gdb is terminated on the spot. > > > > Things aren't usually correct in the cleanup version either, > > since an exception escaping while running cleanups leaves > > the cleanup chain with dangling cleanups, but I think that > > these conversions are the ideal time to fix it up. > > > > The solution will usually be to swallow exceptions in the > > dtor with try/catch(...) and try to limp along. > > Is it worth using `internal_warning` rather than silently dropping the > exception? Here's a patch using internal_warning. If you don't think that's a good idea then I can commit without internal_warning and place this comment in the catch instead: CATCH (ex, RETURN_MASK_ERROR) { /* It's not clear how we should recover from an exception at this point, so for now ignore the error and push on. */ } -- gdb: Improve scoped_switch_fork_info class After committing this patch I got this feedback: https://sourceware.org/ml/gdb-patches/2019-01/msg00181.html This patch makes the constructor of scoped_switch_fork_info explicit, and wraps the core of the destructor in a TRY/CATCH block. I've run this through the testsuite on X86-64/GNU Linux, however, this code is not exercised, so this patch is untested. gdb/ChangeLog: * linux-fork.c (scoped_switch_fork_info) <scoped_switch_fork_info>: Make explicit. <~scoped_switch_fork_info>: Wrap core in try/catch. --- gdb/ChangeLog | 6 ++++++ gdb/linux-fork.c | 18 ++++++++++++++---- 2 files changed, 20 insertions(+), 4 deletions(-)
Comments
On 01/09/2019 02:10 PM, Andrew Burgess wrote: > * Andrew Burgess <andrew.burgess@embecosm.com> [2019-01-09 13:33:23 +0000]: > >> * Pedro Alves <palves@redhat.com> [2019-01-09 12:54:59 +0000]: >> >>> On 01/01/2019 10:45 PM, Andrew Burgess wrote: >>> When writing destructors, we need to keep in mind that if an >>> exception escapes, gdb is terminated on the spot. >>> >>> Things aren't usually correct in the cleanup version either, >>> since an exception escaping while running cleanups leaves >>> the cleanup chain with dangling cleanups, but I think that >>> these conversions are the ideal time to fix it up. >>> >>> The solution will usually be to swallow exceptions in the >>> dtor with try/catch(...) and try to limp along. >> >> Is it worth using `internal_warning` rather than silently dropping the >> exception? > > Here's a patch using internal_warning. If you don't think that's a > good idea then I can commit without internal_warning and place this > comment in the catch instead: I don't think this would be an internal issue? For example, something external to GDB could SIGKILL the inferior process, and then calling lseek in the inferior within fork_load_infrun_state, could throw, for example. If we print something, it'd be pedantically better to not talk about gdb functions. Maybe something around: warning (_("Couldn't restore checkpoint state in %s: %s", target_pid_to_str (fp->ptid), ex.message); ? > > CATCH (ex, RETURN_MASK_ERROR) Use RETURN_MASK_ALL, since if a Quit/Ctrl-C escapes, RETURN_MASK_ERROR won't catch it, and GDB dies. Pedantically, raw C++ try/catch(...), would catch any kind of exception, even non-GDB ones, but that would mean losing the ex object, unless you complicate this some more. We don't throw around non-GDB exceptions (maybe a std::logic_error could be possible somewhere, but I'd call that a bug), so I think CATCH (ex, RETURN_MASK_ALL) is good enough. > { > /* It's not clear how we should recover from an exception at > this point, so for now ignore the error and push on. */ > } That's fine too. It's what I done in other similar spots, though I confess that I had assumed that warning() writes to gdb_stdout, and thus could paginate and cause another exception (ctrl-c/Quit), but now that I look, I see that warning() writes to gdb_stderr which doesn't paginate. There's even a comment about that: /* Print a warning message. The first argument STRING is the warning message, used as an fprintf format string, the second is the va_list of arguments for that string. A warning is unfiltered (not paginated) so that the user does not need to page through each screen full of warnings when there are lots of them. */ Eh. So yeah, a warning seems good. Thanks, Pedro Alves
* Pedro Alves <palves@redhat.com> [2019-01-09 18:44:54 +0000]: > On 01/09/2019 02:10 PM, Andrew Burgess wrote: > > * Andrew Burgess <andrew.burgess@embecosm.com> [2019-01-09 13:33:23 +0000]: > > > >> * Pedro Alves <palves@redhat.com> [2019-01-09 12:54:59 +0000]: > >> > >>> On 01/01/2019 10:45 PM, Andrew Burgess wrote: > > >>> When writing destructors, we need to keep in mind that if an > >>> exception escapes, gdb is terminated on the spot. > >>> > >>> Things aren't usually correct in the cleanup version either, > >>> since an exception escaping while running cleanups leaves > >>> the cleanup chain with dangling cleanups, but I think that > >>> these conversions are the ideal time to fix it up. > >>> > >>> The solution will usually be to swallow exceptions in the > >>> dtor with try/catch(...) and try to limp along. > >> > >> Is it worth using `internal_warning` rather than silently dropping the > >> exception? > > > > Here's a patch using internal_warning. If you don't think that's a > > good idea then I can commit without internal_warning and place this > > comment in the catch instead: > > I don't think this would be an internal issue? For example, something > external to GDB could SIGKILL the inferior process, and then calling lseek > in the inferior within fork_load_infrun_state, could throw, for example. > > If we print something, it'd be pedantically better to not talk about > gdb functions. > > Maybe something around: > > warning (_("Couldn't restore checkpoint state in %s: %s", > target_pid_to_str (fp->ptid), ex.message); > > ? > > > > > CATCH (ex, RETURN_MASK_ERROR) > > Use RETURN_MASK_ALL, since if a Quit/Ctrl-C escapes, RETURN_MASK_ERROR > won't catch it, and GDB dies. > > Pedantically, raw C++ try/catch(...), would catch any kind of exception, even > non-GDB ones, but that would mean losing the ex object, unless you complicate > this some more. We don't throw around non-GDB exceptions (maybe a > std::logic_error could be possible somewhere, but I'd call that a bug), > so I think > CATCH (ex, RETURN_MASK_ALL) > is good enough. > > > { > > /* It's not clear how we should recover from an exception at > > this point, so for now ignore the error and push on. */ > > } > That's fine too. It's what I done in other similar spots, though I confess > that I had assumed that warning() writes to gdb_stdout, and thus could > paginate and cause another exception (ctrl-c/Quit), but now that I look, > I see that warning() writes to gdb_stderr which doesn't paginate. > There's even a comment about that: > > /* Print a warning message. The first argument STRING is the warning > message, used as an fprintf format string, the second is the > va_list of arguments for that string. A warning is unfiltered (not > paginated) so that the user does not need to page through each > screen full of warnings when there are lots of them. */ > > Eh. So yeah, a warning seems good. I pushed this patch with the warning as you suggested. General apology to the list - I fluffed the commit, and had to send a small fix-up patch immediately afterwards. Sorry for that. Thanks, Andrew
diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c index f3231bae048..a71a429678c 100644 --- a/gdb/linux-fork.c +++ b/gdb/linux-fork.c @@ -446,7 +446,7 @@ class scoped_switch_fork_info public: /* Switch to the infrun state held on the fork_info identified by PPTID. If PPTID is the current inferior then no switch is done. */ - scoped_switch_fork_info (ptid_t pptid) + explicit scoped_switch_fork_info (ptid_t pptid) : m_oldfp (nullptr) { if (pptid != inferior_ptid) @@ -472,9 +472,19 @@ public: if (m_oldfp != nullptr) { /* Switch back to inferior_ptid. */ - remove_breakpoints (); - fork_load_infrun_state (m_oldfp); - insert_breakpoints (); + TRY + { + remove_breakpoints (); + fork_load_infrun_state (m_oldfp); + insert_breakpoints (); + } + CATCH (ex, RETURN_MASK_ERROR) + { + internal_warning (__FILE__, __LINE__, + "error during scoped_switch_fork_info cleanup: %s", + ex.message); + } + END_CATCH } }