This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Place displaced step data directly in inferior structure
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Cc: "dblaikie at gmail dot com" <dblaikie at gmail dot com>
- Date: Fri, 23 Nov 2018 19:01:24 +0000
- Subject: Re: [PATCH] Place displaced step data directly in inferior structure
- References: <20181123182429.9045-1-simon.marchi@ericsson.com>
On 11/23/2018 06:24 PM, Simon Marchi wrote:
> This patch moves the per-inferior data related to displaced stepping to
> be directly in the inferior structure, rather than in a container on the
> side.
>
> On notable difference is that previously, we deleted the state on
> inferior exit, which guaranteed a clean state if re-using the inferior
> for a new run or attach. We now need to reset the state manually.
>
Thanks. This LGTM. See a couple nits below.
>
> /* Returns true if any inferior has a thread doing a displaced
> @@ -1531,9 +1490,11 @@ get_displaced_stepping_state (inferior *inf)
> static bool
> displaced_step_in_progress_any_inferior ()
> {
> - for (auto *state : displaced_step_inferior_states)
> + inferior *i;
> +
> + ALL_INFERIORS (i)
This won't compile in current master. :-)
Use
for (inferior *inf : all_inferiors ())
> {
> - if (state->step_thread != nullptr)
> + if (i->displaced_step_state.step_thread != nullptr)
> return true;
> }
> if (debug_infrun)
> diff --git a/gdb/infrun.h b/gdb/infrun.h
> index a701f0ca47f..81b3c326163 100644
> --- a/gdb/infrun.h
> +++ b/gdb/infrun.h
> @@ -258,4 +258,51 @@ struct buf_displaced_step_closure : displaced_step_closure
> gdb::byte_vector buf;
> };
>
> +/* Per-inferior displaced stepping state. */
> +struct displaced_step_inferior_state
> +{
> + /* Put this object back in its original state. s*/
Typo: the "s" at the end.
> +/* Per-inferior displaced stepping state. */
> +struct displaced_step_inferior_state
> +{
> + /* Put this object back in its original state. s*/
> + void reset ()
> + {
> + /* These should have been cleaned after the last displaced step
> + operation, so if there are still set, it's a bug. */
> + gdb_assert (step_thread == nullptr);
> + gdb_assert (step_closure == nullptr);
> +
> + failed_before = 0;
> + step_gdbarch = nullptr;
> + step_original = 0;
> + step_copy = 0;
> +
> + if (step_saved_copy != nullptr)
> + {
> + xfree (step_saved_copy);
> + step_saved_copy = nullptr;
> + }
> + }
> +
> + /* True if preparing a displaced step ever failed. If so, we won't
> + try displaced stepping for this inferior again. */
> + int failed_before = 0;
> +
> + /* If this is not nullptr, this is the thread carrying out a
> + displaced single-step in process PID. This thread's state will
> + require fixing up once it has completed its step. */
> + thread_info *step_thread = nullptr;
> +
> + /* The architecture the thread had when we stepped it. */
> + gdbarch *step_gdbarch = nullptr;
> +
> + /* The closure provided gdbarch_displaced_step_copy_insn, to be used
> + for post-step cleanup. */
> + displaced_step_closure *step_closure = nullptr;
> +
> + /* The address of the original instruction, and the copy we
> + made. */
> + CORE_ADDR step_original = 0, step_copy = 0;
> +
> + /* Saved contents of copy area. */
> + gdb_byte *step_saved_copy = nullptr;
I'm fine with this, but I thought I'd mention another possibility,
which is to not initialize the field in-class, but instead add
a ctor that calls reset(), thus keeping the initializations
all in the same place.
Thanks,
Pedro Alves