From patchwork Wed Dec 12 15:16:13 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 30647 Received: (qmail 124631 invoked by alias); 12 Dec 2018 15:16:29 -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 124537 invoked by uid 89); 12 Dec 2018 15:16:29 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.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, UNSUBSCRIBE_BODY autolearn=ham version=3.3.2 spammy=thrown, prove X-HELO: mail-wr1-f42.google.com Received: from mail-wr1-f42.google.com (HELO mail-wr1-f42.google.com) (209.85.221.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 12 Dec 2018 15:16:24 +0000 Received: by mail-wr1-f42.google.com with SMTP id c14so18112750wrr.0 for ; Wed, 12 Dec 2018 07:16:23 -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 :in-reply-to:references; bh=NkTmrMGtBajEGB+t9G+L/TQobhOfE5ji45yRudHQQkU=; b=L/WRs6XYFZVWPMTpGR7Zk9eBvSfhcQbLu6fU3lUa0sLNppfbXqxDGjxoEWhXy5TXEV o+tPDiv+lrzDK6vQzNBububUuJzumBrv8xwCVeoKzPKDlZg4fGpfvhGwhvSm6UwdaSii H+PKJIeGlSD8UG0bQgS6p6GShF+o8/4I+qqHcjqwrJ3ObkXB3dHzo7A4qoXdQQXCjMzc 9EmGRpeTm4zDcaoZ6XQeXUXZzvaOiFpisv7L6/WxgG6Eneub/cTNcS/K1PKOoXCAueaJ 59yJoVWV/JAqkpBJS6Er+oEgkK4TWVecvTA8oJnvkDmbLN8bnyMhq+6F6NWbEGhnE4aQ CS9g== Return-Path: Received: from localhost ([2a02:390:741d:1:3665:d267:b319:d766]) by smtp.gmail.com with ESMTPSA id z17sm14549610wrv.2.2018.12.12.07.16.20 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 12 Dec 2018 07:16:20 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCHv3 1/2] gdb/infcall: Make infcall_suspend_state more class like Date: Wed, 12 Dec 2018 15:16:13 +0000 Message-Id: In-Reply-To: References: In-Reply-To: References: X-IsSubscribed: yes I ran into a situation where attempting to make an inferior function call would trigger an assertion, 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) The problem that triggers the assertion is that in the function save_infcall_suspend_state, we basically did this: 1. Create empty infcall_suspend_state object. 2. Fill fields of infcall_suspend_state object. The problem is causes is that if filling any of the fields triggered an exception then the infcall_suspend_state object would be deleted while in a partially filled in state. In the specific case I encountered, I had a remote RISC-V target that claimed in its target description to support floating point registers. However, this was not true, and when GDB tried to read a floating point register the remote sent back an error. This error would cause an exception to be thrown while creating the readonly_detached_regcache, which in turn caused GDB to try and delete an infcall_suspend_state which didn't have any register state, and this triggered the assertion. To prevent this problem we have two possibilities, either, rewrite the restore code the handle partially initialised infcall_suspend_state objects, or, prevent partially initialised infcall_suspend_state objects from existing. The second of these seems like a better solution. So, in this patch, I move the filling in of the different infcall_suspend_state fields within a new constructor for infcall_suspend_state. Now, if generating one of those fields fails the destructor for infcall_suspend_state will not be executed and GDB will not try to restore the partially saved state. With this patch in place GDB now behaves like this: (gdb) call some_inferior_function () Could not fetch register "ft0"; remote failure reply 'E99' (gdb) The inferior function call is aborted due to the error. This has been tested against x86-64/Linux native, native-gdbserver, and native-extended-gdbserver with no regressions. I've manually tested this against my baddly behaving target and confirmed the inferior function call is aborted as described above. gdb/ChangeLog: * infrun.c (infcall_suspend_state::infcall_suspend_state): New. (infcall_suspend_state::get_registers): New. (infcall_suspend_state::restore): New. (infcall_suspend_state::thread_suspend): Rename to... (infcall_suspend_state::m_thread_suspend): ...this. (infcall_suspend_state::registers): Rename to... (infcall_suspend_state::m_registers): ...this. (infcall_suspend_state::siginfo_gdbarch): Rename to... (infcall_suspend_state::m_siginfo_gdbarch): ...this. (infcall_suspend_state::siginfo_data): Rename to... (infcall_suspend_state::m_siginfo_data): ...this. (save_infcall_suspend_state): Rewrite to use infcall_suspend_state constructor. (restore_infcall_suspend_state): Rewrite to use infcall_suspend_state::restore method. (get_infcall_suspend_state_regcache): Use infcall_suspend_state::get_registers method. --- gdb/ChangeLog | 20 +++++++++ gdb/infrun.c | 132 +++++++++++++++++++++++++++++++++++----------------------- 2 files changed, 100 insertions(+), 52 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 46a8985f860..a9d7fa17aaf 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -8742,18 +8742,85 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var, struct infcall_suspend_state { - struct thread_suspend_state thread_suspend; +public: + /* Capture state from GDBARCH, TP, and REGCACHE that must be restored + once the inferior function call has finished. */ + infcall_suspend_state (struct gdbarch *gdbarch, + const struct thread_info *tp, + struct regcache *regcache) + : m_thread_suspend (tp->suspend), + m_registers (new readonly_detached_regcache (*regcache)) + { + gdb::unique_xmalloc_ptr siginfo_data; - /* Other fields: */ - std::unique_ptr registers; + if (gdbarch_get_siginfo_type_p (gdbarch)) + { + struct type *type = gdbarch_get_siginfo_type (gdbarch); + size_t len = TYPE_LENGTH (type); + + siginfo_data.reset ((gdb_byte *) xmalloc (len)); + + if (target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL, + siginfo_data.get (), 0, len) != len) + { + /* Errors ignored. */ + siginfo_data.reset (nullptr); + } + } + + if (siginfo_data) + { + m_siginfo_gdbarch = gdbarch; + m_siginfo_data = std::move (siginfo_data); + } + } + + /* Return a pointer to the stored register state. */ + + readonly_detached_regcache * get_registers () const + { + return m_registers.get (); + } + + /* Restores the stored state into GDBARCH, TP, and REGCACHE. */ + + void restore (struct gdbarch *gdbarch, + struct thread_info *tp, + struct regcache *regcache) const + { + tp->suspend = m_thread_suspend; + + if (m_siginfo_gdbarch == gdbarch) + { + struct type *type = gdbarch_get_siginfo_type (gdbarch); + + /* Errors ignored. */ + target_write (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL, + m_siginfo_data.get (), 0, TYPE_LENGTH (type)); + } + + /* The inferior can be gone if the user types "print exit(0)" + (and perhaps other times). */ + if (target_has_execution) + /* NB: The register write goes through to the target. */ + regcache->restore (get_registers ()); + } + +private: + /* How the current thread stopped before the inferior function call was + executed. */ + struct thread_suspend_state m_thread_suspend; + + /* The registers before the inferior function call was executed. */ + std::unique_ptr m_registers; /* Format of SIGINFO_DATA or NULL if it is not present. */ - struct gdbarch *siginfo_gdbarch = nullptr; + struct gdbarch *m_siginfo_gdbarch = nullptr; /* The inferior format depends on SIGINFO_GDBARCH and it has a length of TYPE_LENGTH (gdbarch_get_siginfo_type ()). For different gdbarch the content would be invalid. */ - gdb::unique_xmalloc_ptr siginfo_data; + gdb::unique_xmalloc_ptr m_siginfo_data; }; infcall_suspend_state_up @@ -8762,39 +8829,16 @@ save_infcall_suspend_state () struct thread_info *tp = inferior_thread (); struct regcache *regcache = get_current_regcache (); struct gdbarch *gdbarch = regcache->arch (); - gdb::unique_xmalloc_ptr siginfo_data; - - if (gdbarch_get_siginfo_type_p (gdbarch)) - { - struct type *type = gdbarch_get_siginfo_type (gdbarch); - size_t len = TYPE_LENGTH (type); - - siginfo_data.reset ((gdb_byte *) xmalloc (len)); - - if (target_read (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL, - siginfo_data.get (), 0, len) != len) - { - /* Errors ignored. */ - siginfo_data.reset (nullptr); - } - } - - infcall_suspend_state_up inf_state (new struct infcall_suspend_state); - if (siginfo_data) - { - inf_state->siginfo_gdbarch = gdbarch; - inf_state->siginfo_data = std::move (siginfo_data); - } - - inf_state->thread_suspend = tp->suspend; + infcall_suspend_state_up inf_state + (new struct infcall_suspend_state (gdbarch, tp, regcache)); - /* run_inferior_call will not use the signal due to its `proceed' call with - GDB_SIGNAL_0 anyway. */ + /* Having saved the current state, adjust the thread state, discarding + any stop signal information, this is not useful when starting an + inferior function call and run_inferior_call will not use the signal + due to its `proceed' call with GDB_SIGNAL_0. */ tp->suspend.stop_signal = GDB_SIGNAL_0; - inf_state->registers.reset (new readonly_detached_regcache (*regcache)); - return inf_state; } @@ -8807,23 +8851,7 @@ restore_infcall_suspend_state (struct infcall_suspend_state *inf_state) struct regcache *regcache = get_current_regcache (); struct gdbarch *gdbarch = regcache->arch (); - tp->suspend = inf_state->thread_suspend; - - if (inf_state->siginfo_gdbarch == gdbarch) - { - struct type *type = gdbarch_get_siginfo_type (gdbarch); - - /* Errors ignored. */ - target_write (current_top_target (), TARGET_OBJECT_SIGNAL_INFO, NULL, - inf_state->siginfo_data.get (), 0, TYPE_LENGTH (type)); - } - - /* The inferior can be gone if the user types "print exit(0)" - (and perhaps other times). */ - if (target_has_execution) - /* NB: The register write goes through to the target. */ - regcache->restore (inf_state->registers.get ()); - + inf_state->restore (gdbarch, tp, regcache); discard_infcall_suspend_state (inf_state); } @@ -8836,7 +8864,7 @@ discard_infcall_suspend_state (struct infcall_suspend_state *inf_state) readonly_detached_regcache * get_infcall_suspend_state_regcache (struct infcall_suspend_state *inf_state) { - return inf_state->registers.get (); + return inf_state->get_registers (); } /* infcall_control_state contains state regarding gdb's control of the