Message ID | 20181121181704.15929-1-andrew.burgess@embecosm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 75908 invoked by alias); 21 Nov 2018 18:17:19 -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 75893 invoked by uid 89); 21 Nov 2018 18:17:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=AWL, 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=burn, acknowledge, 2777, encountered 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, 21 Nov 2018 18:17:16 +0000 Received: by mail-wr1-f65.google.com with SMTP id p4so6700181wrt.7 for <gdb-patches@sourceware.org>; Wed, 21 Nov 2018 10:17:16 -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; bh=yMPs6D0ffqzPSLQClh9MdcXTsXx17mdewS0FQsVmd/U=; b=UdaHoZl2bR2SmwsZhDngaRj41tzeLJjE0pp9Bi+un2WuCY94itVOg0xFdWisfeKHT4 1gCb45gNYs3fLg/OPqGHvsGuCA0JlZKTBXe8oc70XzWViylMlglhuX8s/A95Q3zW6QqD OC2XPSRHIRdAd7w8lUw9zaH0UbWFBql14xyas1/5BbuS/+8/Woi1LyYZWHGznVANu9DR kBq+7BztEjkh9z6djVMKyw/WA4AIvA6bKX3uhB4fly/uYO/lYThmelPUhc6lc3UulQDr +bjrVjHjqi4DhTWXBD3QG09E3AcG33tr9/ond0POaaeHbhXrL6ZtFU0FCS1GytRmuzMc Ddyg== Return-Path: <andrew.burgess@embecosm.com> Received: from localhost (host81-156-111-139.range81-156.btcentralplus.com. [81.156.111.139]) by smtp.gmail.com with ESMTPSA id v62-v6sm2151563wme.3.2018.11.21.10.17.12 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 21 Nov 2018 10:17:13 -0800 (PST) From: Andrew Burgess <andrew.burgess@embecosm.com> To: gdb-patches@sourceware.org Cc: Andrew Burgess <andrew.burgess@embecosm.com> Subject: [PATCH] gdb/regcache: When saving, ignore registers that can't be read Date: Wed, 21 Nov 2018 18:17:04 +0000 Message-Id: <20181121181704.15929-1-andrew.burgess@embecosm.com> X-IsSubscribed: yes |
Commit Message
Andrew Burgess
Nov. 21, 2018, 6:17 p.m. UTC
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(-)
Comments
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? Simon
* Simon Marchi <simark@simark.ca> [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(-)
This revision doesn't change much over v2, except that I have dropped the previous patch #2 in response to Pedro's feedback. I still think that patch #1 is worth while, it _is_ possible to throw errors while reading registers, and ideally this wouldn't cause an assertion (which with patch #1 it no longer will). What is now patch #2, but was previously #3 is just a small cleanup of test results. --- Andrew Burgess (2): gdb/infcall: Make infcall_suspend_state more class like gdb: Update test pattern to deal with native-extended-gdbserver gdb/ChangeLog | 20 ++++++ gdb/infrun.c | 132 ++++++++++++++++++++++--------------- gdb/testsuite/ChangeLog | 4 ++ gdb/testsuite/gdb.base/annota1.exp | 23 ++++++- 4 files changed, 125 insertions(+), 54 deletions(-)
On 12/12/2018 03:16 PM, Andrew Burgess wrote: > This revision doesn't change much over v2, except that I have dropped > the previous patch #2 in response to Pedro's feedback. > > I still think that patch #1 is worth while, it _is_ possible to throw > errors while reading registers, and ideally this wouldn't cause an > assertion (which with patch #1 it no longer will). > > What is now patch #2, but was previously #3 is just a small cleanup of > test results. Sorry for the delay, I did not intend to block this. I've reviewed the patches now. Thanks, Pedro Alves
* Pedro Alves <palves@redhat.com> [2018-12-12 16:14:45 +0000]: > On 12/12/2018 03:16 PM, Andrew Burgess wrote: > > This revision doesn't change much over v2, except that I have dropped > > the previous patch #2 in response to Pedro's feedback. > > > > I still think that patch #1 is worth while, it _is_ possible to throw > > errors while reading registers, and ideally this wouldn't cause an > > assertion (which with patch #1 it no longer will). > > > > What is now patch #2, but was previously #3 is just a small cleanup of > > test results. > > Sorry for the delay, I did not intend to block this. I've reviewed > the patches now. Absolutely not a problem. Thanks for the review. Pushed with the fixes you identified. Andrew
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);