This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCHv3 1/2] gdb/infcall: Make infcall_suspend_state more class like
Hi Andrew,
LGTM with the nits below addressed.
On 12/12/2018 03:16 PM, Andrew Burgess wrote:
> 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
Can you s/struct/class/ while at it? Just for aesthetic reasons,
since all fields/methods are covered with public/private.
"struct" hints at a type that is mostly public data.
> {
> - 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<gdb_byte> siginfo_data;
>
> - /* Other fields: */
> - std::unique_ptr<readonly_detached_regcache> 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
No space between '*' and get_registers.
A "get_" prefix is not customary in gdb (except in global functions),
IMHO it doesn't add much other than horizontal space. :-)
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 (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<readonly_detached_regcache> 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<gdb_byte> siginfo_data;
> + gdb::unique_xmalloc_ptr<gdb_byte> 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<gdb_byte> 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. */
Long combined sentences like that are confusing to read, at least to me.
If think that if we spell out what the "this" in "this is not useful" is
referring to, and add a comma before 'and' since it's connecting two
independent clauses, it becomes clearer. Something like:
/* 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->get_registers ();
> }
>
> /* infcall_control_state contains state regarding gdb's control of the
>
Thanks,
Pedro Alves