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

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

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