Message ID | 20190413082817.29343-1-philippe.waroquiers@skynet.be |
---|---|
State | New, archived |
Headers |
Received: (qmail 78356 invoked by alias); 13 Apr 2019 08:28:29 -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 78344 invoked by uid 89); 13 Apr 2019 08:28:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.1 spammy=H*F:D*be, sk:uncaugh, sleep, HContent-Transfer-Encoding:8bit X-HELO: mailsec114.isp.belgacom.be Received: from mailsec114.isp.belgacom.be (HELO mailsec114.isp.belgacom.be) (195.238.20.110) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 13 Apr 2019 08:28:25 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1555144105; x=1586680105; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=nLGT7ZrUVt/SFSpCXpajCvTyF0ZycBaFIlpR/5epH9U=; b=paW0rXa6IvWuSyYNjVvpzi1P1G+3MeQgltraLaSoZpZFiqCSBRg6sBRC bO2/qg8ViYsL9Nry9YovDW1hO47FuA==; Received: from 212.135-131-109.adsl-dyn.isp.belgacom.be (HELO md.home) ([109.131.135.212]) by relay.skynet.be with ESMTP/TLS/DHE-RSA-AES128-GCM-SHA256; 13 Apr 2019 10:28:22 +0200 From: Philippe Waroquiers <philippe.waroquiers@skynet.be> To: gdb-patches@sourceware.org Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be> Subject: [RFA] Fix GDB 8.3 regression crash when registers cannot be modified. Date: Sat, 13 Apr 2019 10:28:17 +0200 Message-Id: <20190413082817.29343-1-philippe.waroquiers@skynet.be> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Philippe Waroquiers
April 13, 2019, 8:28 a.m. UTC
This crash was detected when using GDB with the valgrind gdbserver. To reproduce: valgrind sleep 10000 In another window: gdb target remote | vgdb p printf("make sleep print something\n") => terminate called after throwing an instance of 'gdb_exception_RETURN_MASK_ERROR' Aborted The problem is that the valgrind gdbserver does not allow to change registers when the inferior is blocked in a system call. GDB then raises an exception. The exception causes the destructor of typedef std::unique_ptr<infcall_suspend_state, infcall_suspend_state_deleter> infcall_suspend_state_up; to be called. This destructor itself tries to restore the value of the registers, and fails similarly. We must catch the exception in the destructor to avoid crashing GDB. If the destructor encounters a problem, no warning is produced if there is an uncaught exception, as in this case, the user will already be informed of a problem via this exception. With this change, no crash anymore, and all the valgrind 3.15 tests pass succesfully. Note: when this patch is approved, I will push an equivalent patch on master, but with TRY/CATCH/e.message () replaced by try/catch/e.what (). gdb/ChangeLog struct infcall_suspend_state_deleter 2019-04-13 Philippe Waroquiers <philippe.waroquiers@skynet.be> * inferior.h (struct infcall_suspend_state_deleter): Catch exception in destructor to avoid crash. --- gdb/inferior.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
Comments
Hi, On Sat, Apr 13, 2019 at 10:28:17AM +0200, Philippe Waroquiers wrote: > This crash was detected when using GDB with the valgrind gdbserver. > To reproduce: > > valgrind sleep 10000 > > In another window: > gdb > target remote | vgdb > p printf("make sleep print something\n") > => > terminate called after throwing an instance of 'gdb_exception_RETURN_MASK_ERROR' > Aborted > > The problem is that the valgrind gdbserver does not allow to change > registers when the inferior is blocked in a system call. > GDB then raises an exception. The exception causes the destructor > of > typedef std::unique_ptr<infcall_suspend_state, infcall_suspend_state_deleter> > infcall_suspend_state_up; > to be called. This destructor itself tries to restore the value of > the registers, and fails similarly. We must catch the exception in > the destructor to avoid crashing GDB. > If the destructor encounters a problem, no warning is produced if > there is an uncaught exception, as in this case, the user will already > be informed of a problem via this exception. > > With this change, no crash anymore, and all the valgrind 3.15 tests > pass succesfully. > > Note: when this patch is approved, I will push an equivalent patch > on master, but with TRY/CATCH/e.message () replaced by > try/catch/e.what (). > > gdb/ChangeLog > > struct infcall_suspend_state_deleter > 2019-04-13 Philippe Waroquiers <philippe.waroquiers@skynet.be> > > * inferior.h (struct infcall_suspend_state_deleter): > Catch exception in destructor to avoid crash. What is the current status of this patch? We really need something like this to enable the valgrind gdbserver integration testing again on Fedora 30/rawhide. Thanks, Mark > --- > gdb/inferior.h | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/gdb/inferior.h b/gdb/inferior.h > index 2d1bb97a28..4d84afac6a 100644 > --- a/gdb/inferior.h > +++ b/gdb/inferior.h > @@ -68,7 +68,19 @@ struct infcall_suspend_state_deleter > { > void operator() (struct infcall_suspend_state *state) const > { > - restore_infcall_suspend_state (state); > + TRY > + { > + restore_infcall_suspend_state (state); > + } > + CATCH (e, RETURN_MASK_ALL) > + { > + /* If we are restoring the inferior state due to an exception, > + some error message will be printed. So, only warn the user > + when we cannot restore during normal execution. */ > + if (!std::uncaught_exception ()) > + warning (_("Failed to restore inferior state: %s"), e.message); > + } > + END_CATCH > } > }; > > -- > 2.20.1
On Fri, 2019-04-19 at 13:03 +0200, Mark Wielaard wrote: > > gdb/ChangeLog > > > > struct infcall_suspend_state_deleter > > 2019-04-13 Philippe Waroquiers <philippe.waroquiers@skynet.be> > > > > * inferior.h (struct infcall_suspend_state_deleter): > > Catch exception in destructor to avoid crash. > > What is the current status of this patch? > We really need something like this to enable the valgrind gdbserver > integration testing again on Fedora 30/rawhide. This is still in state RFA, waiting for review. Philippe
On 4/19/19 12:36 PM, Philippe Waroquiers wrote: > On Fri, 2019-04-19 at 13:03 +0200, Mark Wielaard wrote: >>> gdb/ChangeLog >>> >>> struct infcall_suspend_state_deleter >>> 2019-04-13 Philippe Waroquiers <philippe.waroquiers@skynet.be> >>> >>> * inferior.h (struct infcall_suspend_state_deleter): >>> Catch exception in destructor to avoid crash. >> >> What is the current status of this patch? >> We really need something like this to enable the valgrind gdbserver >> integration testing again on Fedora 30/rawhide. > This is still in state RFA, waiting for review. I had not realized you had posted this updated patch. This version is OK. Thanks, Pedro Alves
On Fri, 2019-04-19 at 12:48 +0100, Pedro Alves wrote:
> This version is OK.
Thanks for the review and the suggestion to use
std::uncaught_exception ().
Pushed to 8.3 branch.
Pushed an equivalent patch to master,
with TRY/CATCH/e.message replaced by try/catch/e.what ().
Philippe
On Friday, April 19 2019, Philippe Waroquiers wrote: > On Fri, 2019-04-19 at 12:48 +0100, Pedro Alves wrote: >> This version is OK. > Thanks for the review and the suggestion to use > std::uncaught_exception (). > > Pushed to 8.3 branch. > Pushed an equivalent patch to master, > with TRY/CATCH/e.message replaced by try/catch/e.what (). Today is a holiday here, but I'll rebase F30 GDB first thing tomorrow. Thanks.
diff --git a/gdb/inferior.h b/gdb/inferior.h index 2d1bb97a28..4d84afac6a 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -68,7 +68,19 @@ struct infcall_suspend_state_deleter { void operator() (struct infcall_suspend_state *state) const { - restore_infcall_suspend_state (state); + TRY + { + restore_infcall_suspend_state (state); + } + CATCH (e, RETURN_MASK_ALL) + { + /* If we are restoring the inferior state due to an exception, + some error message will be printed. So, only warn the user + when we cannot restore during normal execution. */ + if (!std::uncaught_exception ()) + warning (_("Failed to restore inferior state: %s"), e.message); + } + END_CATCH } };