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: Philippe Waroquiers <philippe dot waroquiers at skynet dot be>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, Pedro Alves <palves at redhat 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: Sun, 30 Dec 2018 13:59:35 +0100
- Subject: Re: [PATCH] Place displaced step data directly in inferior structure
- References: <20181123182429.9045-1-simon.marchi@ericsson.com> <ab298d38-3e4f-5acc-ed8c-8147deb6bf72@redhat.com> <ac328109-21e9-dc20-a7e9-d064da3ca63f@ericsson.com> <bf11d64a-816c-06a0-da99-d9e3c49ced17@ericsson.com>
Isn't/wasn't this ready to be pushed ?
Thanks
Philippe
On Fri, 2018-11-23 at 21:05 +0000, Simon Marchi wrote:
> On 2018-11-23 2:47 p.m., Simon Marchi wrote:
> > I agree it would be nice to avoid that duplication. I kept the asserts and
> > in-class initializations for step_thread and step_closure, because I think
> > having those asserts can be useful.
> >
> > I also changed step_saved_copy to be a vector, this makes the job a bit easier
> > and plugs the leak identified in
> >
> > https://sourceware.org/ml/gdb-patches/2018-11/msg00220.html
> >
> > Technically we don't need to clear it... but if we didn't the comment
> > above "reset" would be a lie :).
> >
> > Here's the updated patch (currently testing on the buildbot):
>
> Ah damn it, there's a case where the assertion does not hold (gdb.base/step-over-exit.exp).
> So here's a simpler version of the patch where we just blindly reset everything to the
> default values (similar to what we do today).
>
>
> From 1587bbc5c67349a1429542989cf33b102109746e Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Wed, 21 Nov 2018 21:35:16 -0500
> Subject: [PATCH] Place displaced step data directly in inferior structure
>
> 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.
>
> At the same time, I changed step_saved_copy to be a gdb::byte_vector, so
> it is automatically freed on destruction (which should plug the leak
> reported here [1]).
>
> [1] https://sourceware.org/ml/gdb-patches/2018-11/msg00202.html
>
> gdb/ChangeLog:
>
> * inferior.h (class inferior) <displaced_step_state>: New field.
> * infrun.h (struct displaced_step_state): Move here from
> infrun.c. Initialize fields, add constructor.
> <inf>: Remove field.
> <reset>: New method.
> * infrun.c (struct displaced_step_inferior_state): Move to
> infrun.h.
> (displaced_step_inferior_states): Remove.
> (get_displaced_stepping_state): Adust.
> (displaced_step_in_progress_any_inferior): Adjust.
> (displaced_step_in_progress_thread): Adjust.
> (displaced_step_in_progress): Adjust.
> (add_displaced_stepping_state): Remove.
> (get_displaced_step_closure_by_addr): Adjust.
> (remove_displaced_stepping_state): Remove.
> (infrun_inferior_exit): Call displaced_step_state.reset.
> (use_displaced_stepping): Don't check for NULL.
> (displaced_step_prepare_throw): Call
> get_displaced_stepping_state.
> (displaced_step_fixup): Don't check for NULL.
> (prepare_for_detach): Don't check for NULL.
> ---
> gdb/inferior.h | 3 ++
> gdb/infrun.c | 142 +++++++------------------------------------------
> gdb/infrun.h | 44 +++++++++++++++
> 3 files changed, 67 insertions(+), 122 deletions(-)
>
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index 33c2eac9d3c..1a3f579d8d2 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -503,6 +503,9 @@ public:
> this gdbarch. */
> struct gdbarch *gdbarch = NULL;
>
> + /* Data related to displaced stepping. */
> + displaced_step_inferior_state displaced_step_state;
> +
> /* Per inferior data-pointers required by other GDB modules. */
> REGISTRY_FIELDS;
> };
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 46a8985f860..cf216c7804e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1476,53 +1476,12 @@ step_over_info_valid_p (void)
>
> displaced_step_closure::~displaced_step_closure () = default;
>
> -/* Per-inferior displaced stepping state. */
> -struct displaced_step_inferior_state
> -{
> - /* The process this displaced step state refers to. */
> - inferior *inf;
> -
> - /* True if preparing a displaced step ever failed. If so, we won't
> - try displaced stepping for this inferior again. */
> - int failed_before;
> -
> - /* 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;
> -
> - /* The architecture the thread had when we stepped it. */
> - struct gdbarch *step_gdbarch;
> -
> - /* The closure provided gdbarch_displaced_step_copy_insn, to be used
> - for post-step cleanup. */
> - struct displaced_step_closure *step_closure;
> -
> - /* The address of the original instruction, and the copy we
> - made. */
> - CORE_ADDR step_original, step_copy;
> -
> - /* Saved contents of copy area. */
> - gdb_byte *step_saved_copy;
> -};
> -
> -/* The list of states of processes involved in displaced stepping
> - presently. */
> -static std::forward_list<displaced_step_inferior_state *>
> - displaced_step_inferior_states;
> -
> /* Get the displaced stepping state of process PID. */
>
> static displaced_step_inferior_state *
> get_displaced_stepping_state (inferior *inf)
> {
> - for (auto *state : displaced_step_inferior_states)
> - {
> - if (state->inf == inf)
> - return state;
> - }
> -
> - return nullptr;
> + return &inf->displaced_step_state;
> }
>
> /* Returns true if any inferior has a thread doing a displaced
> @@ -1531,9 +1490,9 @@ get_displaced_stepping_state (inferior *inf)
> static bool
> displaced_step_in_progress_any_inferior ()
> {
> - for (auto *state : displaced_step_inferior_states)
> + for (inferior *i : all_inferiors ())
> {
> - if (state->step_thread != nullptr)
> + if (i->displaced_step_state.step_thread != nullptr)
> return true;
> }
>
> @@ -1546,13 +1505,9 @@ displaced_step_in_progress_any_inferior ()
> static int
> displaced_step_in_progress_thread (thread_info *thread)
> {
> - struct displaced_step_inferior_state *displaced;
> -
> gdb_assert (thread != NULL);
>
> - displaced = get_displaced_stepping_state (thread->inf);
> -
> - return (displaced != NULL && displaced->step_thread == thread);
> + return get_displaced_stepping_state (thread->inf)->step_thread == thread;
> }
>
> /* Return true if process PID has a thread doing a displaced step. */
> @@ -1560,34 +1515,7 @@ displaced_step_in_progress_thread (thread_info *thread)
> static int
> displaced_step_in_progress (inferior *inf)
> {
> - struct displaced_step_inferior_state *displaced;
> -
> - displaced = get_displaced_stepping_state (inf);
> - if (displaced != NULL && displaced->step_thread != nullptr)
> - return 1;
> -
> - return 0;
> -}
> -
> -/* Add a new displaced stepping state for process PID to the displaced
> - stepping state list, or return a pointer to an already existing
> - entry, if it already exists. Never returns NULL. */
> -
> -static displaced_step_inferior_state *
> -add_displaced_stepping_state (inferior *inf)
> -{
> - displaced_step_inferior_state *state
> - = get_displaced_stepping_state (inf);
> -
> - if (state != nullptr)
> - return state;
> -
> - state = XCNEW (struct displaced_step_inferior_state);
> - state->inf = inf;
> -
> - displaced_step_inferior_states.push_front (state);
> -
> - return state;
> + return get_displaced_stepping_state (inf)->step_thread != nullptr;
> }
>
> /* If inferior is in displaced stepping, and ADDR equals to starting address
> @@ -1597,42 +1525,21 @@ add_displaced_stepping_state (inferior *inf)
> struct displaced_step_closure*
> get_displaced_step_closure_by_addr (CORE_ADDR addr)
> {
> - struct displaced_step_inferior_state *displaced
> + displaced_step_inferior_state *displaced
> = get_displaced_stepping_state (current_inferior ());
>
> /* If checking the mode of displaced instruction in copy area. */
> - if (displaced != NULL
> - && displaced->step_thread != nullptr
> + if (displaced->step_thread != nullptr
> && displaced->step_copy == addr)
> return displaced->step_closure;
>
> return NULL;
> }
>
> -/* Remove the displaced stepping state of process PID. */
> -
> -static void
> -remove_displaced_stepping_state (inferior *inf)
> -{
> - gdb_assert (inf != nullptr);
> -
> - displaced_step_inferior_states.remove_if
> - ([inf] (displaced_step_inferior_state *state)
> - {
> - if (state->inf == inf)
> - {
> - xfree (state);
> - return true;
> - }
> - else
> - return false;
> - });
> -}
> -
> static void
> infrun_inferior_exit (struct inferior *inf)
> {
> - remove_displaced_stepping_state (inf);
> + inf->displaced_step_state.reset ();
> }
>
> /* If ON, and the architecture supports it, GDB will use displaced
> @@ -1669,17 +1576,15 @@ use_displaced_stepping (struct thread_info *tp)
> {
> struct regcache *regcache = get_thread_regcache (tp);
> struct gdbarch *gdbarch = regcache->arch ();
> - struct displaced_step_inferior_state *displaced_state;
> -
> - displaced_state = get_displaced_stepping_state (tp->inf);
> + displaced_step_inferior_state *displaced_state
> + = get_displaced_stepping_state (tp->inf);
>
> return (((can_use_displaced_stepping == AUTO_BOOLEAN_AUTO
> && target_is_non_stop_p ())
> || can_use_displaced_stepping == AUTO_BOOLEAN_TRUE)
> && gdbarch_displaced_step_copy_insn_p (gdbarch)
> && find_record_target () == NULL
> - && (displaced_state == NULL
> - || !displaced_state->failed_before));
> + && !displaced_state->failed_before);
> }
>
> /* Clean out any stray displaced stepping state. */
> @@ -1734,14 +1639,12 @@ displaced_step_dump_bytes (struct ui_file *file,
> static int
> displaced_step_prepare_throw (thread_info *tp)
> {
> - struct cleanup *ignore_cleanups;
> regcache *regcache = get_thread_regcache (tp);
> struct gdbarch *gdbarch = regcache->arch ();
> const address_space *aspace = regcache->aspace ();
> CORE_ADDR original, copy;
> ULONGEST len;
> struct displaced_step_closure *closure;
> - struct displaced_step_inferior_state *displaced;
> int status;
>
> /* We should never reach this function if the architecture does not
> @@ -1760,7 +1663,8 @@ displaced_step_prepare_throw (thread_info *tp)
> /* We have to displaced step one thread at a time, as we only have
> access to a single scratch space per inferior. */
>
> - displaced = add_displaced_stepping_state (tp->inf);
> + displaced_step_inferior_state *displaced
> + = get_displaced_stepping_state (tp->inf);
>
> if (displaced->step_thread != nullptr)
> {
> @@ -1816,10 +1720,8 @@ displaced_step_prepare_throw (thread_info *tp)
> }
>
> /* Save the original contents of the copy area. */
> - displaced->step_saved_copy = (gdb_byte *) xmalloc (len);
> - ignore_cleanups = make_cleanup (free_current_contents,
> - &displaced->step_saved_copy);
> - status = target_read_memory (copy, displaced->step_saved_copy, len);
> + displaced->step_saved_copy.resize (len);
> + status = target_read_memory (copy, displaced->step_saved_copy.data (), len);
> if (status != 0)
> throw_error (MEMORY_ERROR,
> _("Error accessing memory address %s (%s) for "
> @@ -1830,7 +1732,7 @@ displaced_step_prepare_throw (thread_info *tp)
> fprintf_unfiltered (gdb_stdlog, "displaced: saved %s: ",
> paddress (gdbarch, copy));
> displaced_step_dump_bytes (gdb_stdlog,
> - displaced->step_saved_copy,
> + displaced->step_saved_copy.data (),
> len);
> };
>
> @@ -1841,7 +1743,6 @@ displaced_step_prepare_throw (thread_info *tp)
> /* The architecture doesn't know how or want to displaced step
> this instruction or instruction sequence. Fallback to
> stepping over the breakpoint in-line. */
> - do_cleanups (ignore_cleanups);
> return -1;
> }
>
> @@ -1853,7 +1754,8 @@ displaced_step_prepare_throw (thread_info *tp)
> displaced->step_original = original;
> displaced->step_copy = copy;
>
> - make_cleanup (displaced_step_clear_cleanup, displaced);
> + cleanup *ignore_cleanups
> + = make_cleanup (displaced_step_clear_cleanup, displaced);
>
> /* Resume execution at the copy. */
> regcache_write_pc (regcache, copy);
> @@ -1931,7 +1833,7 @@ displaced_step_restore (struct displaced_step_inferior_state *displaced,
> ULONGEST len = gdbarch_max_insn_length (displaced->step_gdbarch);
>
> write_memory_ptid (ptid, displaced->step_copy,
> - displaced->step_saved_copy, len);
> + displaced->step_saved_copy.data (), len);
> if (debug_displaced)
> fprintf_unfiltered (gdb_stdlog, "displaced: restored %s %s\n",
> target_pid_to_str (ptid),
> @@ -1953,10 +1855,6 @@ displaced_step_fixup (thread_info *event_thread, enum gdb_signal signal)
> = get_displaced_stepping_state (event_thread->inf);
> int ret;
>
> - /* Was any thread of this process doing a displaced step? */
> - if (displaced == NULL)
> - return 0;
> -
> /* Was this event for the thread we displaced? */
> if (displaced->step_thread != event_thread)
> return 0;
> @@ -3577,7 +3475,7 @@ prepare_for_detach (void)
>
> /* Is any thread of this process displaced stepping? If not,
> there's nothing else to do. */
> - if (displaced == NULL || displaced->step_thread == nullptr)
> + if (displaced->step_thread == nullptr)
> return;
>
> if (debug_infrun)
> diff --git a/gdb/infrun.h b/gdb/infrun.h
> index a701f0ca47f..1b243db057c 100644
> --- a/gdb/infrun.h
> +++ b/gdb/infrun.h
> @@ -258,4 +258,48 @@ struct buf_displaced_step_closure : displaced_step_closure
> gdb::byte_vector buf;
> };
>
> +/* Per-inferior displaced stepping state. */
> +struct displaced_step_inferior_state
> +{
> + displaced_step_inferior_state ()
> + {
> + reset ();
> + }
> +
> + /* Put this object back in its original state. */
> + void reset ()
> + {
> + failed_before = 0;
> + step_thread = nullptr;
> + step_gdbarch = nullptr;
> + step_closure = nullptr;
> + step_original = 0;
> + step_copy = 0;
> + step_saved_copy.clear ();
> + }
> +
> + /* True if preparing a displaced step ever failed. If so, we won't
> + try displaced stepping for this inferior again. */
> + int failed_before;
> +
> + /* 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;
> +
> + /* The architecture the thread had when we stepped it. */
> + gdbarch *step_gdbarch;
> +
> + /* The closure provided gdbarch_displaced_step_copy_insn, to be used
> + for post-step cleanup. */
> + displaced_step_closure *step_closure;
> +
> + /* The address of the original instruction, and the copy we
> + made. */
> + CORE_ADDR step_original, step_copy;
> +
> + /* Saved contents of copy area. */
> + gdb::byte_vector step_saved_copy;
> +};
> +
> #endif /* INFRUN_H */
> --
> 2.19.1
>