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

[Pedro Alves <pedro@palves.net>]

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;
> +}