Message ID | 1554915489.1446.6.camel@skynet.be |
---|---|
State | New, archived |
Headers |
Received: (qmail 48622 invoked by alias); 10 Apr 2019 16:58:16 -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 48610 invoked by uid 89); 10 Apr 2019 16:58:15 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.0 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=82, 8.2, pushing, saves X-HELO: mailsec118.isp.belgacom.be Received: from mailsec118.isp.belgacom.be (HELO mailsec118.isp.belgacom.be) (195.238.20.114) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 10 Apr 2019 16:58:11 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1554915491; x=1586451491; h=message-id:subject:from:to:date:mime-version: content-transfer-encoding; bh=ZMoQQ8fZ18trdmymcqBP03hkw64SbHFB5OPFoOS5XRc=; b=YXQDwa938BTCyxTmXwZ5yTrbMdHoTsSUS2DXY4ht5prATEMncI+3+kzB bwfhD1L/fTL8rq3mTgMxSGob5vPR/Q==; Received: from unknown (HELO md) ([109.131.135.212]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 10 Apr 2019 18:58:09 +0200 Message-ID: <1554915489.1446.6.camel@skynet.be> Subject: 8.3 regression: terminate called after throwing an instance of 'gdb_exception_error' From: Philippe Waroquiers <philippe.waroquiers@skynet.be> To: gdb-patches@sourceware.org Date: Wed, 10 Apr 2019 18:58:09 +0200 Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Philippe Waroquiers
April 10, 2019, 4:58 p.m. UTC
This problem was found when running the valgrind gdbserver tests with GDB 8.3 (the valgrind test gdbserver_tests/mcinfcallWSRU hangs due to GDB crashing). An easy way 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_error' Aborted The problem is due to the fact that sometimes, Valgrind gdbserver refuses to change registers : when GDB has 'waken up' the valgrind gdbserver while the valgrind process is blocked in a syscall, it is not possible to change the (simulated) valgrind registers, as in this state, valgrind has "switched" to the real registers. So, in such a state, the valgrind gdbserver returns an error to the P packet. With GDB 8.2, that gives: (gdb) p printf("make sleep print something\n") Could not write register "rdi"; remote failure reply 'E. ERROR changing register rdi regno 5 gdb commands changing registers (pc, sp, ...) (e.g. 'jump', set pc, calling from gdb a function in the debugged process, ...) can only be accepted if the thread is VgTs_Runnable or VgTs_Yielding state Thread status is VgTs_WaitSys ' (gdb) With 8.3, GDB aborts with this 'terminate called ...' message. As far as I can see, the problem is: Before pushing the inferior call, GDB 8.3 saves the state of the inferior, using the below type: typedef std::unique_ptr<infcall_suspend_state, infcall_suspend_state_deleter> infcall_suspend_state_up; Then the push of inferior call tries to modify a register. It fails with the above message, causing a throw. This throw causes the infcall_suspend_state_up destructor to be called, calling the deleter. The deleter tries to restore the state by modifying back the register to the saved value. It similarly fails and causes an exception in the destructor. That then causes the above abort. The below patch avoids the crash. Not very clear to me if that is the correct way to do. Note that the valgrind test then still fails, because the below produces more messages than the GDB 8.2. So, maybe we could/should just silently ignore errors in the exception handler ? (IIUC, in GDB 8.2, this was done with a cleanup that was ignoring such errors). Philippe
Comments
On 4/10/19 5:58 PM, Philippe Waroquiers wrote: > This problem was found when running the valgrind gdbserver tests with > GDB 8.3 (the valgrind test gdbserver_tests/mcinfcallWSRU hangs > due to GDB crashing). > > An easy way 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_error' > Aborted I think that that error message must have been printed by gdb master, since that type doesn't exist in the 8.3 branch? That was only added very recently, with the elimination of the TRY/CATCH macros. > > > The problem is due to the fact that sometimes, Valgrind gdbserver > refuses to change registers : when GDB has 'waken up' the valgrind > gdbserver while the valgrind process is blocked in a syscall, it is not possible > to change the (simulated) valgrind registers, as in this state, valgrind > has "switched" to the real registers. > > So, in such a state, the valgrind gdbserver returns an error to the P packet. > With GDB 8.2, that gives: > (gdb) p printf("make sleep print something\n") > Could not write register "rdi"; remote failure reply 'E. > ERROR changing register rdi regno 5 > gdb commands changing registers (pc, sp, ...) (e.g. 'jump', > set pc, calling from gdb a function in the debugged process, ...) > can only be accepted if the thread is VgTs_Runnable or VgTs_Yielding state > Thread status is VgTs_WaitSys > ' > (gdb) > > With 8.3, GDB aborts with this 'terminate called ...' message. > > As far as I can see, the problem is: > Before pushing the inferior call, GDB 8.3 saves the state of the inferior, > using the below type: > typedef std::unique_ptr<infcall_suspend_state, infcall_suspend_state_deleter> > infcall_suspend_state_up; > > Then the push of inferior call tries to modify a register. It fails with > the above message, causing a throw. > This throw causes the infcall_suspend_state_up destructor to be called, > calling the deleter. > The deleter tries to restore the state by modifying back the register > to the saved value. It similarly fails and causes an exception in the > destructor. > That then causes the above abort. > > The below patch avoids the crash. Not very clear to me if that > is the correct way to do. > Note that the valgrind test then still fails, because the below > produces more messages than the GDB 8.2. > So, maybe we could/should just silently ignore errors in the exception > handler ? Yeah, I guess silent is fine. Unless we fail to restore the registers in the normal, non-exception path? Maybe print the warning if std::uncaught_exception() is false? That is, if we're existing due to an exception, then we'll already print some error. Otherwise, if we're exiting the scope normally, and that fails, print a warning. We'll still print if some destructor somewhere decides to do an infcall while unwinding due to an exception, and the infcall succeeds (std::uncaught_exception is true due to the outer exception), which is kind of the reason why std::uncaught_exceptions (plural) was invented in C++17, but that seems like a corner case that's not very concerning. The only downside is printing too much. > (IIUC, in GDB 8.2, this was done with a cleanup that was ignoring > such errors). > > Philippe > > > diff --git a/gdb/inferior.h b/gdb/inferior.h > index 2d1bb97a28..d20db63103 100644 > --- a/gdb/inferior.h > +++ b/gdb/inferior.h > @@ -68,7 +68,15 @@ 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) > + { > + warning (_("failed to restore_infcall_suspend_state: %s"), e.message); > + } For current master, you'll need to replace the TRY/CATCH with try/catch. Also, the warning text should not refer to gdb internals, since it's going to be seen by regular users. Thanks, Pedro Alves
diff --git a/gdb/inferior.h b/gdb/inferior.h index 2d1bb97a28..d20db63103 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -68,7 +68,15 @@ 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) +      { + warning (_("failed to restore_infcall_suspend_state: %s"), e.message); +      } +    END_CATCH    }  }; Â