From 6bf78e29a1dea7121f050c6ee4990298eec679dd Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Mon, 26 Nov 2018 12:48:05 +0000 Subject: [PATCH] gdb/infcall: Make infcall_suspend_state into a class 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::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::registers method. --- gdb/ChangeLog | 20 ++++++++ gdb/infrun.c | 134 ++++++++++++++++++++++++++++++-------------------- 2 files changed, 101 insertions(+), 53 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 687c5ae961e..dc705dc968d 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,23 @@ +2018-12-12 Andrew Burgess + + * infrun.c (infcall_suspend_state::infcall_suspend_state): New. + (infcall_suspend_state::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::registers method. + 2018-12-12 Andrew Burgess * riscv-tdep.c (riscv_call_arg_scalar_float): Unnamed (variadic) diff --git a/gdb/infrun.c b/gdb/infrun.c index 46a8985f860..e9336ab1303 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -8740,20 +8740,87 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var, ends (either successfully, or after it hits a breakpoint or signal) if the program is to properly continue where it left off. */ -struct infcall_suspend_state +class 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 *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 (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. The stop signal 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->registers (); } /* infcall_control_state contains state regarding gdb's control of the -- 2.30.2