From patchwork Wed Jan 9 14:10:21 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 31015 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: 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 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 ; 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: 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 To: Pedro Alves 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: <93fd47987a3ab8907c4745e4054d622b5122cede.1546382416.git.andrew.burgess@embecosm.com> <20190109133323.GZ3456@embecosm.com> MIME-Version: 1.0 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 * Andrew Burgess [2019-01-09 13:33:23 +0000]: > * Pedro Alves [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) : 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(-) 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 } }