Re: [PATCH 09/12] gdb: move displaced stepping logic to gdbarch, allow starting concurrent displaced steps

Pedro Alves
2020-11-25T01:41:08Z

Hi,

So overall this is looking pretty good. A few comments below.

Please also see the comments in response to patch #12. This patch
causes a surprising slowdown.

On 11/10/20 9:46 PM, Simon Marchi via Gdb-patches wrote:

> - Ask the gdbarch for the displaced step buffer location
> - Save the existing bytes in the displaced step buffer
> - Ask the gdbarch to copy the instruction into the displaced step buffer
> - Set the pc of the thread to the beginning of the displaced step buffer
>
> Similarly, the "fixup" phase, executed after the instruction was
> successfully single-stepped, is driven by the infrun code (function
> displaced_step_fixup). The steps are roughly:

displaced_step_fixup -> displaced_step_finish. And maybe "finish" phase?

>
> - Restore the original bytes in the displaced stepping buffer
> - Ask the gdbarch to fixup the instruction result (adjust the target's
> registers or memory to do as if the instruction had been executed in
> its original location)
>
> The displaced_step_inferior_state::step_thread field indicates which
> thread (if any) is currently using the displaced stepping buffer, so it
> is used by displaced_step_prepare_throw to check if the displaced
> stepping buffer is free to use or not.
>
> This patch defers the whole task of preparing and cleaning up after a
> displaced step to the gdbarch. Two new main gdbarch methods are added,
> with the following sematics:

"sematics" -> "semantics"

>
> - gdbarch_displaced_step_prepare: Prepare for the given thread to
> execute a displaced step of the instruction located at its current PC.
> Upon return, everything should be ready for GDB to resume the thread
> (with either a single step or continue, as indicated by
> gdbarch_displaced_step_hw_singlestep) to make it displaced step the
> instruction.
>
> - gdbarch_displaced_step_finish: Called when the thread stopped after
> having started a displaced step. Verify if the instruction was
> executed, if so apply any fixup required to compensate for the fact
> that the instruction was executed at different place than its original

"at a different place"

> pc. Release any resources that was allocated for this displaced step.

"that were allocated"

> Upon return, everything should be ready for GDB to resume the
> thread in its "normal" code path.
>
> The displaced_step_prepare_throw function now pretty much just offloads
> to gdbarch_displaced_step_prepare and the displaced_step_finish function
> offloads to gdbarch_displaced_step_finish.
>
> The gdbarch_displaced_step_location method is now unnecessary, so is
> removed. Indeed, the core of GDB doesn't know how many displaced step
> buffers there are nor where they are.
>
> To keep the existing behavior for existing architectures, the logic that
> was previously implemented in infrun.c for preparing and finishing a
> displaced step is moved to displaced-stepping.c, to the
> displaced_step_buffer class. Architectures are modified to implement
> the new gdbarch methods using this class. The behavior is not expeicted
> to change.

"expeicted" -> "expected"

> @@ -37,6 +43,188 @@ show_debug_displaced (struct ui_file *file, int from_tty,
> fprintf_filtered (file, _("Displace stepping debugging is %s.\n"), value);
> }
>
> +displaced_step_prepare_status
> +displaced_step_buffer::prepare (thread_info *thread, CORE_ADDR &displaced_pc)
> +{

...

> +
> + m_original_pc = regcache_read_pc (regcache);
> + displaced_pc = m_addr;
> +
> + /* Save the original contents of the displaced stepping buffer. */
> + m_saved_copy.resize (len);
> +
> + int status = target_read_memory (m_addr, m_saved_copy.data (), len);
> + if (status != 0)
> + throw_error (MEMORY_ERROR,
> + _("Error accessing memory address %s (%s) for "
> + "displaced-stepping scratch space."),
> + paddress (arch, m_addr), safe_strerror (status));
> +
> + displaced_debug_printf ("saved %s: %s",
> + paddress (arch, m_addr),
> + displaced_step_dump_bytes
> + (m_saved_copy.data (), len).c_str ());
> +
> + m_copy_insn_closure = gdbarch_displaced_step_copy_insn (arch,
> + m_original_pc,
> + m_addr,
> + regcache);
> + if (m_copy_insn_closure == nullptr)
> + {
> + /* The architecture doesn't know how or want to displaced step
> + this instruction or instruction sequence. Fallback to
> + stepping over the breakpoint in-line. */
> + return DISPLACED_STEP_PREPARE_STATUS_ERROR;
> + }
> +
> + try
> + {
> + /* Resume execution at the copy. */
> + regcache_write_pc (regcache, m_addr);
> + }
> + catch (...)

I'd rather we don't use 'catch (...)' throughout gdb. It swallows
too much. We should catch gdb_exception_error in most cases or
gdb_exception when we also want to catch Ctrl-C/QUIT. If something
throws something else, I'd rather not swallow it, since it's most
probably a bug.

I'm a bit confused on error handling here. Before the patch, we used
displaced_step_reset_cleanup, but didn't swallow the error. After the patch,
the exception is swallowed and DISPLACED_STEP_PREPARE_STATUS_ERROR is
returned. Was that on purpose?

> + {
> + /* Failed to write the PC. Release the architecture's displaced
> + stepping resources and the thread's displaced stepping state. */
> + m_copy_insn_closure.reset ();
> +
> + return DISPLACED_STEP_PREPARE_STATUS_ERROR;
> + }
> +
> + /* This marks the buffer as being in use. */
> + m_current_thread = thread;
> +
> + return DISPLACED_STEP_PREPARE_STATUS_OK;
> +}

>
> @@ -94,37 +99,78 @@ struct displaced_step_inferior_state
> /* Put this object back in its original state. */
> void reset ()
> {
> - failed_before = 0;
> - step_thread = nullptr;
> - step_gdbarch = nullptr;
> - step_closure.reset ();
> - step_original = 0;
> - step_copy = 0;
> - step_saved_copy.clear ();
> + failed_before = false;
> }
>
> /* True if preparing a displaced step ever failed. If so, we won't
> try displaced stepping for this inferior again. */
> - int failed_before;
> + bool failed_before;
> +};
> +
> +/* Per-thread displaced stepping state. */
> +
> +struct displaced_step_thread_state
> +{
> + /* Return true if this thread is currently executing a displaced step. */
> + bool in_progress () const
> + {
> + return m_original_gdbarch != nullptr;
> + }
> +
> + /* Return the gdbarch of the thread prior to the step. */
> + gdbarch *get_original_gdbarch () const
> + {
> + return m_original_gdbarch;
> + }
> +
> + /* Mark this thread as currently executing a displaced step.
> +
> + ORIGINAL_GDBARCH is the current gdbarch of the thread (before the step
> + is executed). */
> + void set (gdbarch *original_gdbarch)
> + {
> + m_original_gdbarch = original_gdbarch;
> + }
> +
> + /* Mark this thread as no longer executing a displaced step. */
> + void reset ()
> + {
> + m_original_gdbarch = nullptr;
> + }
> +
> +private:
> + gdbarch *m_original_gdbarch = nullptr;
> +};
> +
> +/* Manage access to a single displaced stepping buffer. */
> +
> +struct displaced_step_buffer
> +{
> + displaced_step_buffer (CORE_ADDR buffer_addr)

explicit.

> + : m_addr (buffer_addr)
> + {}
> +
> + displaced_step_prepare_status prepare (thread_info *thread,
> + CORE_ADDR &displaced_pc);
> +
> + displaced_step_finish_status finish (gdbarch *arch, thread_info *thread,
> + gdb_signal sig);
> +
> + const displaced_step_copy_insn_closure *
> + copy_insn_closure_by_addr (CORE_ADDR addr);
>
> - /* 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;
> + void restore_in_ptid (ptid_t ptid);
>
> - /* The architecture the thread had when we stepped it. */
> - gdbarch *step_gdbarch;
> +private:
>
> - /* The closure provided gdbarch_displaced_step_copy_insn, to be used
> - for post-step cleanup. */
> - displaced_step_copy_insn_closure_up step_closure;
> + CORE_ADDR m_original_pc = 0;
> + const CORE_ADDR m_addr;
>
> - /* The address of the original instruction, and the copy we
> - made. */
> - CORE_ADDR step_original, step_copy;
> + /* If set, the thread currently using the buffer. */
> + thread_info *m_current_thread = nullptr;
>
> - /* Saved contents of copy area. */
> - gdb::byte_vector step_saved_copy;
> + gdb::byte_vector m_saved_copy;
> + displaced_step_copy_insn_closure_up m_copy_insn_closure;

Missing comments?

> };
>
> #endif /* DISPLACED_STEPPING_H */

> @@ -1554,9 +1549,9 @@ show_can_use_displaced_stepping (struct ui_file *file, int from_tty,
> static bool
> gdbarch_supports_displaced_stepping (gdbarch *arch)
> {
> - /* Only check for the presence of step_copy_insn. Other required methods
> - are checked by the gdbarch validation. */
> - return gdbarch_displaced_step_copy_insn_p (arch);
> + /* Only check for the presence of `prepare`. `finish` is required by the
> + gdbarch verification to be provided if `prepare` is provided. */

This reads a little funny to me. I'd suggest:

/* Only check for the presence of `prepare`. The gdbarch verification ensures
that if `prepare` is provided, so is `finish`. */

> + return gdbarch_displaced_step_prepare_p (arch);
> }
>
> /* Return non-zero if displaced stepping can/should be used to step

> @@ -1669,96 +1662,52 @@ displaced_step_prepare_throw (thread_info *tp)
> jump/branch). */
> tp->control.may_range_step = 0;
>
> - /* We have to displaced step one thread at a time, as we only have
> - access to a single scratch space per inferior. */
> -
> - displaced_step_inferior_state *displaced
> - = get_displaced_stepping_state (tp->inf);
> -
> - if (displaced->step_thread != nullptr)
> - {
> - /* Already waiting for a displaced step to finish. Defer this
> - request and place in queue. */
> -
> - displaced_debug_printf ("deferring step of %s",
> - target_pid_to_str (tp->ptid).c_str ());
> -
> - global_thread_step_over_chain_enqueue (tp);
> - return DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE;
> - }
> - else
> - displaced_debug_printf ("stepping %s now",
> - target_pid_to_str (tp->ptid).c_str ());
> -
> - displaced_step_reset (displaced);
> + /* We are about to start a displaced step for this thread. If one is already
> + in progress, something's wrong.. */

Double period.

> + gdb_assert (!disp_step_thread_state->in_progress ());
>
> scoped_restore_current_thread restore_thread;
>
> switch_to_thread (tp);
>
> - original = regcache_read_pc (regcache);
> + CORE_ADDR original_pc = regcache_read_pc (regcache);
> + CORE_ADDR displaced_pc;
>
> - copy = gdbarch_displaced_step_location (gdbarch);
> - len = gdbarch_max_insn_length (gdbarch);
> + displaced_step_prepare_status status =
> + gdbarch_displaced_step_prepare (gdbarch, tp, displaced_pc);

" = " on the next line.

>
> - if (breakpoint_in_range_p (aspace, copy, len))
> + if (status == DISPLACED_STEP_PREPARE_STATUS_ERROR)

> @@ -1934,14 +1831,22 @@ static step_over_what thread_still_needs_step_over (struct thread_info *tp);
> static bool
> start_step_over (void)
> {
> - struct thread_info *tp, *next;
> + thread_info *next;
> + bool started = false;
>
> /* Don't start a new step-over if we already have an in-line
> step-over operation ongoing. */
> if (step_over_info_valid_p ())
> - return false;
> + return started;

I'd move the "bool started = false;" below this if line and continue
writing explicit "return false" here, as it's less indirection.

> +
> + /* Steal the global thread step over chain. */

It would be good expand the command explaining _why_ steal.

> + thread_info *threads_to_step = global_thread_step_over_chain_head;
> + global_thread_step_over_chain_head = NULL;
>
> - for (tp = global_thread_step_over_chain_head; tp != NULL; tp = next)
> + infrun_debug_printf ("stealing global queue of threads to step, length = %d",
> + thread_step_over_chain_length (threads_to_step));
> +
> + for (thread_info *tp = threads_to_step; tp != NULL; tp = next)
> {
> struct execution_control_state ecss;
> struct execution_control_state *ecs = &ecss;

> @@ -2005,13 +1902,27 @@ start_step_over (void)
> if (!ecs->wait_some_more)
> error (_("Command aborted."));
>
> - gdb_assert (tp->resumed);
> + /* If the thread's step over could not be initiated, it was re-added
> + to the global step over chain. */
> + if (tp->resumed)
> + {
> + infrun_debug_printf ("[%s] was resumed.",
> + target_pid_to_str (tp->ptid).c_str ());
> + gdb_assert (!thread_is_in_step_over_chain (tp));
> + }
> + else
> + {
> + infrun_debug_printf ("[%s] was NOT resumed.",
> + target_pid_to_str (tp->ptid).c_str ());
> + gdb_assert (thread_is_in_step_over_chain (tp));
> + }
>
> /* If we started a new in-line step-over, we're done. */
> if (step_over_info_valid_p ())
> {
> gdb_assert (tp->control.trap_expected);
> - return true;
> + started = true;
> + break;
> }
>
> if (!target_is_non_stop_p ())
> @@ -2024,7 +1935,8 @@ start_step_over (void)
> /* With remote targets (at least), in all-stop, we can't
> issue any further remote commands until the program stops
> again. */
> - return true;
> + started = true;
> + break;
> }
>
> /* Either the thread no longer needed a step-over, or a new
> @@ -2033,7 +1945,30 @@ start_step_over (void)
> displaced step on a thread of other process. */
> }
>
> - return false;
> + /* If there are threads left in the THREADS_TO_STEP list, but we have
> + detected that we can't start anything more, put back these threads
> + in the global list. */

Do we also need to do this if an exception happens to escape the function?
We might end up pretty bonkers anyhow if that happens, though...

> + if (threads_to_step == NULL)
> + infrun_debug_printf ("step-over queue now empty");
> + else
> + {
> + infrun_debug_printf ("putting back %d threads to step in global queue",
> + thread_step_over_chain_length (threads_to_step));
> +
> + while (threads_to_step != nullptr)
> + {
> + thread_info *thread = threads_to_step;
> +
> + /* Remove from that list. */
> + thread_step_over_chain_remove (&threads_to_step, thread);
> +
> + /* Add to global list. */
> + global_thread_step_over_chain_enqueue (thread);
> +

Spurious empty line. But, did you look into splicing the whole threads_to_step
chain into the global chain in O(1), with just some prev/next pointer
adjustments?

> + }
> + }
> +
> + return started;
> }
>
> /* Update global variables holding ptids to hold NEW_PTID if they were
> @@ -3618,18 +3553,16 @@ prepare_for_detach (void)
> struct inferior *inf = current_inferior ();
> ptid_t pid_ptid = ptid_t (inf->pid);
>
> - displaced_step_inferior_state *displaced = get_displaced_stepping_state (inf);
> -
> /* Is any thread of this process displaced stepping? If not,
> there's nothing else to do. */
> - if (displaced->step_thread == nullptr)
> + if (displaced_step_in_progress (inf))
> return;
>
> infrun_debug_printf ("displaced-stepping in-process while detaching");
>
> scoped_restore restore_detaching = make_scoped_restore (&inf->detaching, true);
>
> - while (displaced->step_thread != nullptr)
> + while (displaced_step_in_progress (inf))
> {
> struct execution_control_state ecss;
> struct execution_control_state *ecs;
> @@ -5293,25 +5226,23 @@ handle_inferior_event (struct execution_control_state *ecs)
> {
> struct regcache *regcache = get_thread_regcache (ecs->event_thread);
> struct gdbarch *gdbarch = regcache->arch ();
> + inferior *parent_inf = find_inferior_ptid (ecs->target, ecs->ptid);
> +
> + if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
> + {
> + /* Restore in the child process any displaced stepping buffers that
> + were in use at the time of the fork. */
> + gdbarch_displaced_step_restore_all_in_ptid
> + (gdbarch, parent_inf, ecs->ws.value.related_pid);
> + }

Why was this moved out of the displaced_step_in_progress_thread check
below?

>
> /* If checking displaced stepping is supported, and thread
> ecs->ptid is displaced stepping. */
> if (displaced_step_in_progress_thread (ecs->event_thread))
> {
> - struct inferior *parent_inf
> - = find_inferior_ptid (ecs->target, ecs->ptid);
> struct regcache *child_regcache;
> CORE_ADDR parent_pc;
>
> - if (ecs->ws.kind == TARGET_WAITKIND_FORKED)
> - {
> - struct displaced_step_inferior_state *displaced
> - = get_displaced_stepping_state (parent_inf);
> -
> - /* Restore scratch pad for child process. */
> - displaced_step_restore (displaced, ecs->ws.value.related_pid);
> - }
> -
> /* GDB has got TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED,
> indicating that the displaced stepping of syscall instruction
> has been done. Perform cleanup for parent process here. Note
> diff --git a/gdb/infrun.h b/gdb/infrun.h
> index c83cb333083..d5e6d279f1a 100644
> --- a/gdb/infrun.h
> +++ b/gdb/infrun.h
> @@ -226,9 +226,6 @@ extern void clear_exit_convenience_vars (void);
> /* Dump LEN bytes at BUF in hex to a string and return it. */
> extern std::string displaced_step_dump_bytes (const gdb_byte *buf, size_t len);
>
> -extern struct displaced_step_copy_insn_closure *
> - get_displaced_step_copy_insn_closure_by_addr (CORE_ADDR addr);
> -
> extern void update_observer_mode (void);
>
> extern void signal_catch_update (const unsigned int *);
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index c57181765e0..f3d1ccb4d1c 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -199,6 +199,9 @@ struct linux_info
> yet. Positive if we tried looking it up, and found it. Negative
> if we tried looking it up but failed. */
> int vsyscall_range_p = 0;
> +
> + /* Inferior's displaced step buffer. */
> + gdb::optional<displaced_step_buffer> disp_step_buf;
> };
>
> /* Per-inferior data key. */
> @@ -2530,6 +2533,61 @@ linux_displaced_step_location (struct gdbarch *gdbarch)
> return addr;
> }
>
> +/* Implementation gdbarch_displaced_step_prepare. */

"Implementation of" ?

> +
> +displaced_step_prepare_status
> +linux_displaced_step_prepare (gdbarch *arch, thread_info *thread,
> + CORE_ADDR &displaced_pc)
> +{
> + linux_info *per_inferior = get_linux_inferior_data (thread->inf);
> +
> + if (!per_inferior->disp_step_buf.has_value ())
> + {
> + CORE_ADDR disp_step_buf_addr
> + = linux_displaced_step_location (thread->inf->gdbarch);
> +
> + per_inferior->disp_step_buf.emplace (disp_step_buf_addr);
> + }
> +
> + return per_inferior->disp_step_buf->prepare (thread, displaced_pc);
> +}
> +
> +/* Implementation gdbarch_displaced_step_finish. */

Ditto.

> +
> +displaced_step_finish_status
> +linux_displaced_step_finish (gdbarch *arch, thread_info *thread, gdb_signal sig)
> +{
> + linux_info *per_inferior = get_linux_inferior_data (thread->inf);
> +
> + gdb_assert (per_inferior->disp_step_buf.has_value ());
> +
> + return per_inferior->disp_step_buf->finish (arch, thread, sig);
> +}
> +
> +const displaced_step_copy_insn_closure *
> +linux_displaced_step_copy_insn_closure_by_addr (inferior *inf, CORE_ADDR addr)

Ditto on the immaginary comment. :-)

> +{
> + linux_info *per_inferior = linux_inferior_data.get (inf);
> +
> + if (per_inferior == nullptr
> + || !per_inferior->disp_step_buf.has_value ())
> + return nullptr;
> +
> + return per_inferior->disp_step_buf->copy_insn_closure_by_addr (addr);
> +}
> +
> +void
> +linux_displaced_step_restore_all_in_ptid (inferior *parent_inf, ptid_t ptid)
> +{
> + linux_info *per_inferior = linux_inferior_data.get (parent_inf);
> +

...

> /* See gdbthread.h. */
> @@ -403,9 +411,32 @@ thread_is_in_step_over_chain (struct thread_info *tp)
>
> /* See gdbthread.h. */
>
> +int
> +thread_step_over_chain_length (thread_info *tp)
> +{
> + if (tp == nullptr)
> + return 0;
> +
> + int num = 1;

Should we add:

gdb_assert (thread_is_in_step_over_chain (tp));

?

But then again, it's a bit odd to allow tp == nullptr,
but not allow tp->step_over_next == nullptr?

> + thread_info *iter = tp->step_over_next;
> +
> + while (iter != tp)
> + {
> + num++;
> + iter = iter->step_over_next;
> + }

This seems to be begging for a "for":

int num = 1;
for (thread_info *iter = tp->step_over_next;
iter != tp;
iter = iter->step_over_next)
num++;

> +
> + return num;
> +}