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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2020-11-24 8:40 p.m., Pedro Alves wrote:
> 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?

Fixed.

>>
>> - 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"

Fixed.

>>
>>   - 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"

Fixed.

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

Fixed.

>
>>     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"

Fixed.

>> +
>> +  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?

Hmm, I probably did this to ensure that any error would be reported by
returning a status code, rather than some errors returning _ERROR and
some errors throwing an exception.  So just for consistency.

Would it make sense to remove the _ERROR status code and report all
errors using exceptions then?

>> +/* Manage access to a single displaced stepping buffer.  */
>> +
>> +struct displaced_step_buffer
>> +{
>> +  displaced_step_buffer (CORE_ADDR buffer_addr)
>
> explicit.

Fixed.

>> +    : 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?

Fixed.

>> @@ -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`.  */

Fixed to use that.

>> +  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.

Fixed.

>> +  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.

Fixed.

>> @@ -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.

Done.

>
>> +
>> +  /* Steal the global thread step over chain.  */
>
> It would be good expand the command explaining _why_ steal.

How about:

  /* Steal the global thread step over chain.  As we try to initiate displaced
     steps, threads will be enqueued in the global chain if no buffers are
     available.  If we iterated on the global chain directly, we might\x13 iterate
     indefinitely.  */

>> @@ -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...

Yes, I realized that when playing with this code again yesterday.  A
scope_exit would help for this I think.

But I'm also wondering if we should enqueue back the thread TP that was
dequeued, that caused an error.  This one has already been dequeued from
THREADS_TO_STEP.  For example, if thread_still_needs_step_over throws
(it reads the PC, so that could happen), should we put back the thread
in the global chain?  If we do, and the same error happens over and
over, the thread will never leave the step over chain and be the first
in line, preventing any other displaced step to happen.

If, on the other hand, the error was just an intermittent one and we
don't put it back in the queue, the next time we'll resume the thread
we'll realize it needs a step over and enqueue it again.  So it sounds
less risky to me to just not enqueue back the thread on error.  I'm
really not sure, I find it difficult to reason about all the possible
cases.

Also, re-reading that code, I noticed that we only return "true" when an
inline step over is started, or if a displaced step on an all-stop
target was started.  That's the case even before the patch.  I suppose
that's on purpose, because the caller wants to know whether it can
resume more stuff or not?  If so, I think we should update
start_step_over's documentation:

  /* Are there any pending step-over requests?  If so, run all we can
     now and return true.  Otherwise, return false.  */

>> +    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?

Fixed that in an experimental patch for now, but it would make sense to
do so, so I'll probably end up folding it into this one.

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

1. thread 1 is doing a displaced step
2. thread 2 does a fork, not letting time for thread 1 to complete

event_thread is thread 2, and it is not doing a displaced step, so we
don't enter the

  if (displaced_step_in_progress_thread (ecs->event_thread))

But we still want to restore the bytes in the child used as the
displaced stepping buffer for thread 1.  Does that make sense?  Is it
possible that the current code is not correct in that regard?

>> @@ -2530,6 +2533,61 @@ linux_displaced_step_location (struct gdbarch *gdbarch)
>>    return addr;
>>  }
>>
>> +/* Implementation gdbarch_displaced_step_prepare.  */
>
> "Implementation of" ?

Changed for

  /* See linux-tdep.h.  */

>
>> +
>> +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.

Changed for "See...".

>
>> +
>> +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.  :-)

Added "See...".

>
>> +{
>> +  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);
>> +

Same here.

>>  /* 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?

Well, a pointer to an empty thread step over chain is a nullptr: when
the global chain is empty, global_thread_step_over_chain is nullptr.
Same for threads_to_step that is local to start_step_over.  So
it makes sense that we can do thread_step_over_chain_length(nullptr).

However, it would be a mistake to pass a non-nullptr pointer to a thread
that is not in a step over chain, so I think it would be a good idea to
add the assert you propose.

I will clarify the documentation of the function:

  /* Return the length of the the step over chain TP is in.

     If TP is non-nullptr, the thread must be in a step over chain.
     TP may be nullptr, in which case it denotes an empty list, so a length of
     0.  */

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

True, changed for:

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

Simon