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

[Andrew Burgess <andrew.burgess@embecosm.com>]

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
>