From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68524 invoked by alias); 27 Nov 2018 11:13:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 68491 invoked by uid 89); 27 Nov 2018 11:13:17 -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=H*Ad:D*ca, D*ca, burn, gracefully X-HELO: mail-wr1-f54.google.com Received: from mail-wr1-f54.google.com (HELO mail-wr1-f54.google.com) (209.85.221.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 27 Nov 2018 11:13:14 +0000 Received: by mail-wr1-f54.google.com with SMTP id v6so22229843wrr.12 for ; Tue, 27 Nov 2018 03:13:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=wCMJcLcJKV4OKbxSStY+E3IGg0TVworqpOy+rUp9ODc=; b=a13RH8JprAGdUdDso21ssD3Bhjbd3/PmS84Eba93FflX6If8ozEPZgkiX+FzJ7wef2 8gZvJ+xetE+JoCT/S2zlmvZg+LM6cWh8yyI9sLp9t7WYYCiyeLMIuA+Mcmk7hBAniqQv VXK7u24eeMmXMjQVl1YUvhcU+U17dMMi+egVYd+BxSv8c9oxygMt9hZn8R3spDuT2xh5 k2XRKE4cNqfIQgbgZmmuDkNmugEbbztZubPd9p/cEuYsiVL9XamgO2kubcROZn6IOGMQ gf7AUeyQ+XjCohjtL4v7cwJfbudVEjSgYykxvb+u161Q6sXBoECR97k4H15Uih1mdomF ClYw== Return-Path: Received: from localhost (host86-156-236-171.range86-156.btcentralplus.com. [86.156.236.171]) by smtp.gmail.com with ESMTPSA id j199sm8495251wmf.13.2018.11.27.03.13.10 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 27 Nov 2018 03:13:10 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Simon Marchi , Andrew Burgess Subject: [PATCHv2 0/3] Handle Errors While Preparing For Inferior Call Date: Tue, 27 Nov 2018 11:13:00 -0000 Message-Id: In-Reply-To: References: X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg00452.txt.bz2 * Simon Marchi [2018-11-25 21:48:35 -0500]: > On 2018-11-21 1:17 p.m., Andrew Burgess wrote: > > If during a call to reg_buffer::save GDB encounters an error trying to > > read a register then this should not cause GDB to crash, nor should it > > force the save to quit. Instead, GDB should just treat the register > > as unavailable and push on. > > > > The specific example I encountered was a RISC-V remote target that > > claimed in its target description to have floating point register > > support. However, this was not true, when GDB tried to read a > > floating point register the remote sent back an error. > > > > Mostly this was fine, the program I was testing were integer only, > > however, when trying to make an inferior call, GDB would try to > > preserve the current values of the floating point registers, this > > result in a read of a register that threw an error, and GDB would > > crash like this: > > > > (gdb) call some_inferior_function () > > ../../src/gdb/regcache.c:310: internal-error: void regcache::restore(readonly_detached_regcache*): Assertion `src != NULL' failed. > > A problem internal to GDB has been detected, > > further debugging may prove unreliable. > > Quit this debugging session? (y or n) > > > > I acknowledge that the target description sent back in this case is > > wrong, and the target should be fixed. However, I think that GDB > > should, at a minimum, not crash and burn in this case, and better, I > > think GDB can probably just push on, ignoring the registers that can't > > be read. > > > > The solution I propose in this patch is to catch errors in > > reg_buffer::save while calling cooked_read, and register that throws > > an error should be considered unavailable. GDB will not try to > > restore these registers after the inferior call. > > > > What I haven't done in this commit is provide any user feedback that > > GDB would like to backup a particular register, but can't. Right now > > I figure that if the user cares about this they would probably try 'p > > $reg_name' themselves, at which point it becomes obvious that the > > register can't be read. That said, I'm open to adding a warning that > > the regiter failed to save if that is thought important. > > > > I've tested this using on X86-64/Linux native, and for > > native-gdbserver with no regressions. Against my miss-behaving target > > I can now make inferior calls without any problems. > > > > gdb/ChangeLog: > > > > * regcache.c (reg_buffer::save): When saving the current register > > state, ignore registers that can't be read. > > --- > > gdb/ChangeLog | 5 +++++ > > gdb/regcache.c | 12 +++++++++++- > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/gdb/regcache.c b/gdb/regcache.c > > index 946035ae67a..b89be24ccb6 100644 > > --- a/gdb/regcache.c > > +++ b/gdb/regcache.c > > @@ -277,7 +277,17 @@ reg_buffer::save (register_read_ftype cooked_read) > > if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup)) > > { > > gdb_byte *dst_buf = register_buffer (regnum); > > - enum register_status status = cooked_read (regnum, dst_buf); > > + enum register_status status; > > + > > + TRY > > + { > > + status = cooked_read (regnum, dst_buf); > > + } > > + CATCH (ex, RETURN_MASK_ERROR) > > + { > > + status = REG_UNAVAILABLE; > > + } > > + END_CATCH > > > > gdb_assert (status != REG_UNKNOWN); > > > > > > > Hi Andrew, > > I think your fix makes sense. > > About the assertion you hit, I think it shows a weakness in infcall_suspend_state_up. > The deleter is not able to handle the state of infcall_suspend_state where the > registers field is NULL. > > So either: > > 1. We decide that an infcall_suspend_state with a NULL registers field is an invalid > state and we make sure to never have one in this state. > 2. We change the deleter (consequently restore_infcall_suspend_state) to have it > handle the possibility of registers == NULL. > > This means that even without your fix, GDB should ideally be able to handle the failure > more gracefully than it does now. The infcall should just be aborted and an error message > shown. > > Does that make sense? Yes it does. In this new series, patch #1 makes the prepare for inferior function call process more resistant to errors during the preparation phase. After this patch the case I addressed above would fail with an error (better than an assertion). In patch #2 I then handle the specific case I am encountering better, so that for the case that a register can't be read, GDB still performs the inferior function call. And patch #3 is a random fix I hit while testing the above patches. How does this look? Thanks, Andrew --- Andrew Burgess (3): gdb/infcall: Make infcall_suspend_state more class like gdb/regcache: When saving, ignore registers that can't be read gdb: Update test pattern to deal with native-extended-gdbserver gdb/ChangeLog | 25 +++++++ gdb/infrun.c | 132 ++++++++++++++++++++++--------------- gdb/regcache.c | 12 +++- gdb/testsuite/ChangeLog | 4 ++ gdb/testsuite/gdb.base/annota1.exp | 23 ++++++- 5 files changed, 141 insertions(+), 55 deletions(-) -- 2.14.5