Re: [PATCH 06/12] gdb: introduce status enum for displaced step prepare/finish

Andrew Burgess
2020-11-11T23:36:34Z

I looked though all the patches up to and including this one. I don't
have too much knowledge of the displaced stepping mechanism, but so
far everything seemed good.

Thanks,
Andrew

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2020-11-10 16:46:08 -0500]:

> This is a preparatory patch to reduce the size of the diff of the
> upcoming main patch. It introduces enum types for the return values of
> displaced step "prepare" and "finish" operations. I find that this
> expresses better the intention of the code, rather than returning
> arbitrary integer values (-1, 0 and 1) which are difficult to remember.
> That makes the code easier to read.
>
> I put the new enum types in a new displaced-stepping.h file, because I
> introduce that file in a later patch anyway. Putting it there avoids
> having to move it later.
>
> There is one change in behavior for displaced_step_finish: it currently
> returns 0 if the thread wasn't doing a displaced step and 1 if the
> thread was doing a displaced step which was executed successfully. It
> turns out that this distinction is not needed by any caller, so I've
> merged these two cases into "_OK", rather than adding an extra
> enumerator.
>
> gdb/ChangeLog:
>
> * infrun.c (displaced_step_prepare_throw): Change return type to
> displaced_step_prepare_status.
> (displaced_step_prepare): Likewise.
> (displaced_step_finish): Change return type to
> displaced_step_finish_status.
> (resume_1): Adjust.
> (stop_all_threads): Adjust.
> * displaced-stepping.h: New file.
>
> Change-Id: I5c8fe07212cd398d5b486b5936d9d0807acd3788
> ---
> gdb/displaced-stepping.h | 46 +++++++++++++++++++++++++
> gdb/infrun.c | 72 +++++++++++++++++++++++-----------------
> 2 files changed, 88 insertions(+), 30 deletions(-)
> create mode 100644 gdb/displaced-stepping.h
>
> diff --git a/gdb/displaced-stepping.h b/gdb/displaced-stepping.h
> new file mode 100644
> index 00000000000..9c7e85c3769
> --- /dev/null
> +++ b/gdb/displaced-stepping.h
> @@ -0,0 +1,46 @@
> +/* Displaced stepping related things.
> +
> + Copyright (C) 2020 Free Software Foundation, Inc.
> +
> + This file is part of GDB.
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <http://www.gnu.org/licenses/>. */
> +
> +#ifndef DISPLACED_STEPPING_H
> +#define DISPLACED_STEPPING_H
> +
> +enum displaced_step_prepare_status
> +{
> + /* A displaced stepping buffer was successfully allocated and prepared. */
> + DISPLACED_STEP_PREPARE_STATUS_OK,
> +
> + /* Something bad happened. */
> + DISPLACED_STEP_PREPARE_STATUS_ERROR,
> +
> + /* Not enough resources are available at this time, try again later. */
> + DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE,
> +};
> +
> +enum displaced_step_finish_status
> +{
> + /* Either the instruction was stepped and fixed up, or the specified thread
> + wasn't executing a displaced step (in which case there's nothing to
> + finish). */
> + DISPLACED_STEP_FINISH_STATUS_OK,
> +
> + /* The thread was executing a displaced step, but didn't complete it. */
> + DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED,
> +};
> +
> +#endif /* DISPLACED_STEPPING_H */
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index cdf198bb307..ed54c4331d7 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -19,6 +19,7 @@
> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> #include "defs.h"
> +#include "displaced-stepping.h"
> #include "infrun.h"
> #include <ctype.h>
> #include "symtab.h"
> @@ -1652,11 +1653,13 @@ displaced_step_dump_bytes (const gdb_byte *buf, size_t len)
> Comments in the code for 'random signals' in handle_inferior_event
> explain how we handle this case instead.
>
> - Returns 1 if preparing was successful -- this thread is going to be
> - stepped now; 0 if displaced stepping this thread got queued; or -1
> - if this instruction can't be displaced stepped. */
> + Returns DISPLACED_STEP_PREPARE_STATUS_OK if preparing was successful -- this
> + thread is going to be stepped now; DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE
> + if displaced stepping this thread got queued; or
> + DISPLACED_STEP_PREPARE_STATUS_ERROR if this instruction can't be displaced
> + stepped. */
>
> -static int
> +static displaced_step_prepare_status
> displaced_step_prepare_throw (thread_info *tp)
> {
> regcache *regcache = get_thread_regcache (tp);
> @@ -1694,7 +1697,7 @@ displaced_step_prepare_throw (thread_info *tp)
> target_pid_to_str (tp->ptid).c_str ());
>
> global_thread_step_over_chain_enqueue (tp);
> - return 0;
> + return DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE;
> }
> else
> displaced_debug_printf ("stepping %s now",
> @@ -1725,7 +1728,7 @@ displaced_step_prepare_throw (thread_info *tp)
> displaced_debug_printf ("breakpoint set in scratch pad. "
> "Stepping over breakpoint in-line instead.");
>
> - return -1;
> + return DISPLACED_STEP_PREPARE_STATUS_ERROR;
> }
>
> /* Save the original contents of the copy area. */
> @@ -1749,7 +1752,7 @@ displaced_step_prepare_throw (thread_info *tp)
> /* 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 -1;
> + return DISPLACED_STEP_PREPARE_STATUS_ERROR;
> }
>
> /* Save the information we need to fix things up if the step
> @@ -1770,20 +1773,21 @@ displaced_step_prepare_throw (thread_info *tp)
>
> displaced_debug_printf ("displaced pc to %s", paddress (gdbarch, copy));
>
> - return 1;
> + return DISPLACED_STEP_PREPARE_STATUS_OK;
> }
>
> /* Wrapper for displaced_step_prepare_throw that disabled further
> attempts at displaced stepping if we get a memory error. */
>
> -static int
> +static displaced_step_prepare_status
> displaced_step_prepare (thread_info *thread)
> {
> - int prepared = -1;
> + displaced_step_prepare_status status
> + = DISPLACED_STEP_PREPARE_STATUS_ERROR;
>
> try
> {
> - prepared = displaced_step_prepare_throw (thread);
> + status = displaced_step_prepare_throw (thread);
> }
> catch (const gdb_exception_error &ex)
> {
> @@ -1810,7 +1814,7 @@ displaced_step_prepare (thread_info *thread)
> displaced_state->failed_before = 1;
> }
>
> - return prepared;
> + return status;
> }
>
> static void
> @@ -1840,22 +1844,24 @@ displaced_step_restore (struct displaced_step_inferior_state *displaced,
> displaced->step_copy));
> }
>
> -/* If we displaced stepped an instruction successfully, adjust
> - registers and memory to yield the same effect the instruction would
> - have had if we had executed it at its original address, and return
> - 1. If the instruction didn't complete, relocate the PC and return
> - -1. If the thread wasn't displaced stepping, return 0. */
> +/* If we displaced stepped an instruction successfully, adjust registers and
> + memory to yield the same effect the instruction would have had if we had
> + executed it at its original address, and return
> + DISPLACED_STEP_PREPARE_STATUS_OK. If the instruction didn't complete,
> + relocate the PC and return DISPLACED_STEP_PREPARE_STATUS_NOT_EXECUTED.
>
> -static int
> + If the thread wasn't displaced stepping, return
> + DISPLACED_STEP_PREPARE_STATUS_OK as well.. */
> +
> +static displaced_step_finish_status
> displaced_step_finish (thread_info *event_thread, enum gdb_signal signal)
> {
> struct displaced_step_inferior_state *displaced
> = get_displaced_stepping_state (event_thread->inf);
> - int ret;
>
> /* Was this event for the thread we displaced? */
> if (displaced->step_thread != event_thread)
> - return 0;
> + return DISPLACED_STEP_FINISH_STATUS_OK;
>
> /* Fixup may need to read memory/registers. Switch to the thread
> that we're fixing up. Also, target_stopped_by_watchpoint checks
> @@ -1879,7 +1885,8 @@ displaced_step_finish (thread_info *event_thread, enum gdb_signal signal)
> displaced->step_original,
> displaced->step_copy,
> get_thread_regcache (displaced->step_thread));
> - ret = 1;
> +
> + return DISPLACED_STEP_FINISH_STATUS_OK;
> }
> else
> {
> @@ -1890,10 +1897,9 @@ displaced_step_finish (thread_info *event_thread, enum gdb_signal signal)
>
> pc = displaced->step_original + (pc - displaced->step_copy);
> regcache_write_pc (regcache, pc);
> - ret = -1;
> - }
>
> - return ret;
> + return DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED;
> + }
> }
>
> /* Data to be passed around while handling an event. This data is
> @@ -2417,16 +2423,17 @@ resume_1 (enum gdb_signal sig)
> && sig == GDB_SIGNAL_0
> && !current_inferior ()->waiting_for_vfork_done)
> {
> - int prepared = displaced_step_prepare (tp);
> + displaced_step_prepare_status prepare_status
> + = displaced_step_prepare (tp);
>
> - if (prepared == 0)
> + if (prepare_status == DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE)
> {
> infrun_debug_printf ("Got placed in step-over queue");
>
> tp->control.trap_expected = 0;
> return;
> }
> - else if (prepared < 0)
> + else if (prepare_status == DISPLACED_STEP_PREPARE_STATUS_ERROR)
> {
> /* Fallback to stepping over the breakpoint in-line. */
>
> @@ -2440,7 +2447,7 @@ resume_1 (enum gdb_signal sig)
>
> insert_breakpoints ();
> }
> - else if (prepared > 0)
> + else if (prepare_status == DISPLACED_STEP_PREPARE_STATUS_OK)
> {
> /* Update pc to reflect the new address from which we will
> execute instructions due to displaced stepping. */
> @@ -2448,6 +2455,9 @@ resume_1 (enum gdb_signal sig)
>
> step = gdbarch_displaced_step_hw_singlestep (gdbarch);
> }
> + else
> + gdb_assert_not_reached (_("Invalid displaced_step_prepare_status "
> + "value."));
> }
>
> /* Do we need to do it the hard way, w/temp breakpoints? */
> @@ -4822,7 +4832,8 @@ stop_all_threads (void)
> t->suspend.waitstatus.kind = TARGET_WAITKIND_IGNORE;
> t->suspend.waitstatus_pending_p = 0;
>
> - if (displaced_step_finish (t, GDB_SIGNAL_0) < 0)
> + if (displaced_step_finish (t, GDB_SIGNAL_0)
> + == DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED)
> {
> /* Add it back to the step-over queue. */
> infrun_debug_printf
> @@ -4850,7 +4861,8 @@ stop_all_threads (void)
> sig = (event.ws.kind == TARGET_WAITKIND_STOPPED
> ? event.ws.value.sig : GDB_SIGNAL_0);
>
> - if (displaced_step_finish (t, sig) < 0)
> + if (displaced_step_finish (t, sig)
> + == DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED)
> {
> /* Add it back to the step-over queue. */
> t->control.trap_expected = 0;
> --
> 2.28.0
>