Re: [PATCH 11/12] gdb: make displaced stepping implementation capable of managing multiple buffers

[Pedro Alves <pedro@palves.net>]

A few minor comments below.  Otherwise LGTM.

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

> @@ -44,86 +44,114 @@ show_debug_displaced (struct ui_file *file, int from_tty,
>  }
>  
>  displaced_step_prepare_status
> -displaced_step_buffer::prepare (thread_info *thread, CORE_ADDR &displaced_pc)
> +displaced_step_buffers::prepare (thread_info *thread, CORE_ADDR &displaced_pc)
>  {
>    gdb_assert (!thread->displaced_step_state.in_progress ());
>  
> -  /* Is a thread currently using the buffer?  */
> -  if (m_current_thread != nullptr)
> -    {
> -      /* If so, it better not be this thread.  */
> -      gdb_assert (thread != m_current_thread);
> -      return DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE;
> -    }
> +  /* Sanity check: the thread should not be using a buffer at this point.  */
> +  for (displaced_step_buffer &buf : m_buffers)
> +    gdb_assert (buf.current_thread != thread);
>  
>    regcache *regcache = get_thread_regcache (thread);
>    const address_space *aspace = regcache->aspace ();
>    gdbarch *arch = regcache->arch ();
>    ULONGEST len = gdbarch_max_insn_length (arch);
>  
> -  if (breakpoint_in_range_p (aspace, m_addr, len))
> -    {
> -      /* There's a breakpoint set in the scratch pad location range
> -	 (which is usually around the entry point).  We'd either
> -	 install it before resuming, which would overwrite/corrupt the
> -	 scratch pad, or if it was already inserted, this displaced
> -	 step would overwrite it.  The latter is OK in the sense that
> -	 we already assume that no thread is going to execute the code
> -	 in the scratch pad range (after initial startup) anyway, but
> -	 the former is unacceptable.  Simply punt and fallback to
> -	 stepping over this breakpoint in-line.  */
> -      displaced_debug_printf ("breakpoint set in scratch pad.  "
> -			      "Stepping over breakpoint in-line instead.");
> +  /* Search for an unused buffer.  */
> +  displaced_step_buffer *buffer = nullptr;
> +  displaced_step_prepare_status fail_status
> +    = DISPLACED_STEP_PREPARE_STATUS_ERROR;
>  
> -      return DISPLACED_STEP_PREPARE_STATUS_ERROR;
> +  for (displaced_step_buffer &candidate : m_buffers)
> +    {
> +      bool bp_in_range = breakpoint_in_range_p (aspace, candidate.addr, len);
> +      bool is_free = candidate.current_thread == nullptr;
> +
> +      if (!bp_in_range)
> +        {
> +          if (is_free)

tabs vs spaces.

> +	    {
> +	      buffer = &candidate;
> +	      break;
> +	    }
> +	  else
> +	    {
> +	      /* This buffer would be suitable, but it's used right now.  */
> +	      fail_status = DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE;
> +	    }
> +        }

tabs vs spaces.

> +      else
> +	{
> +	  /* There's a breakpoint set in the scratch pad location range
> +	     (which is usually around the entry point).  We'd either
> +	     install it before resuming, which would overwrite/corrupt the
> +	     scratch pad, or if it was already inserted, this displaced
> +	     step would overwrite it.  The latter is OK in the sense that
> +	     we already assume that no thread is going to execute the code
> +	     in the scratch pad range (after initial startup) anyway, but
> +	     the former is unacceptable.  Simply punt and fallback to
> +	     stepping over this breakpoint in-line.  */
> +	  displaced_debug_printf ("breakpoint set in displaced stepping "
> +				  "buffer at %s, can't use.",
> +				  paddress (arch, candidate.addr));
> +	}




> -/* Manage access to a single displaced stepping buffer.  */
> +/* Control access to multiple displaced stepping buffers at fixed addresses.  */
>  
> -struct displaced_step_buffer
> +struct displaced_step_buffers
>  {
> -  displaced_step_buffer (CORE_ADDR buffer_addr)
> -    : m_addr (buffer_addr)
> -  {}
> +  displaced_step_buffers (gdb::array_view<CORE_ADDR> buffer_addrs)
> +  {
> +    gdb_assert (buffer_addrs.size () > 0);
> +
> +    for (CORE_ADDR buffer_addr : buffer_addrs)
> +      m_buffers.emplace_back (buffer_addr);

Call:

      m_buffers.reserve (buffer_addrs.size ());

before.


But even better, the caller is allocating a temporary std::vector
and passing that:

 +      std::vector<CORE_ADDR> buffers;
 +      for (int i = 0; i < gdbarch_data->num_disp_step_buffers; i++)
 +       buffers.push_back (disp_step_buf_addr + i * buf_len);

So how about making the ctor above be instead:

  displaced_step_buffers (std::vector<CORE_ADDR> &&buffer_addrs);

and make the caller move the vector into displaced_step_buffers?

 +      per_inferior->disp_step_bufs.emplace (std::move (buffers));



> +  }
>  
>    displaced_step_prepare_status prepare (thread_info *thread,
>  					 CORE_ADDR &displaced_pc);
> @@ -163,14 +168,33 @@ struct displaced_step_buffer
>  
>  private:
>  
> -  CORE_ADDR m_original_pc = 0;
> -  const CORE_ADDR m_addr;
> +  /* State of a single buffer.  */
> +
> +  struct displaced_step_buffer
> +  {
> +    displaced_step_buffer (CORE_ADDR addr)

"explicit".

> +      : addr (addr)
> +    {}
> +
> +    const CORE_ADDR addr;
> +
> +    /* The original PC of the instruction currently begin stepped.  */

"begin" -> "being" ?

> +    CORE_ADDR original_pc = 0;
> +
> +    /* If set, the thread currently using the buffer.  If unset, the buffer is not
> +       used.  */
> +    thread_info *current_thread = nullptr;
> +
> +    /* Saved copy of the bytes in the displaced buffer, to be restored once the
> +       buffer is no longer used.  */
> +    gdb::byte_vector saved_copy;
>