Re: [PATCH 01/31] displaced step: pass down target_waitstatus instead of gdb_signal

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> This commit tweaks displaced_step_finish & friends to pass down a
> target_waitstatus instead of a gdb_signal.  This needed because a

missing word: "This IS needed".

> patch later in the series will want to make
> displaced_step_buffers::finish handle TARGET_WAITKIND_THREAD_EXITED.
> It also helps with the TARGET_WAITKIND_THREAD_CLONED patch.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
> Change-Id: I4c5d338647b028071bc498c4e47063795a2db4c0
> ---
>  gdb/displaced-stepping.c  | 11 ++++++-----
>  gdb/displaced-stepping.h  |  2 +-
>  gdb/gdbarch-components.py |  2 +-
>  gdb/gdbarch-gen.h         |  4 ++--
>  gdb/gdbarch.c             |  4 ++--
>  gdb/infrun.c              | 17 +++++++----------
>  gdb/linux-tdep.c          |  5 +++--
>  gdb/linux-tdep.h          |  2 +-
>  gdb/rs6000-tdep.c         |  4 ++--
>  9 files changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
> index 7dfd63d8716..7b5d327008d 100644
> --- a/gdb/displaced-stepping.c
> +++ b/gdb/displaced-stepping.c
> @@ -192,10 +192,11 @@ write_memory_ptid (ptid_t ptid, CORE_ADDR memaddr,
>  }
>  
>  static bool
> -displaced_step_instruction_executed_successfully (gdbarch *arch,
> -						  gdb_signal signal)
> +displaced_step_instruction_executed_successfully
> +  (gdbarch *arch, const target_waitstatus &status)
>  {
> -  if (signal != GDB_SIGNAL_TRAP)
> +  if (status.kind () != TARGET_WAITKIND_STOPPED
> +      || status.sig () != GDB_SIGNAL_TRAP)
>      return false;
>  
>    if (target_stopped_by_watchpoint ())
> @@ -210,7 +211,7 @@ displaced_step_instruction_executed_successfully (gdbarch *arch,
>  
>  displaced_step_finish_status
>  displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
> -				gdb_signal sig)
> +				const target_waitstatus &status)
>  {
>    gdb_assert (thread->displaced_step_state.in_progress ());
>  
> @@ -256,7 +257,7 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
>    regcache *rc = get_thread_regcache (thread);
>  
>    bool instruction_executed_successfully
> -    = displaced_step_instruction_executed_successfully (arch, sig);
> +    = displaced_step_instruction_executed_successfully (arch, status);
>  
>    if (instruction_executed_successfully)
>      {
> diff --git a/gdb/displaced-stepping.h b/gdb/displaced-stepping.h
> index de40ae2f3d8..e23a8d6736b 100644
> --- a/gdb/displaced-stepping.h
> +++ b/gdb/displaced-stepping.h
> @@ -168,7 +168,7 @@ struct displaced_step_buffers
>  					 CORE_ADDR &displaced_pc);
>  
>    displaced_step_finish_status finish (gdbarch *arch, thread_info *thread,
> -				       gdb_signal sig);
> +				       const target_waitstatus &status);
>  
>    const displaced_step_copy_insn_closure *
>      copy_insn_closure_by_addr (CORE_ADDR addr);
> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index e7230949aad..5d60f7677f0 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -1829,7 +1829,7 @@ Clean up after a displaced step of THREAD.
>  """,
>      type="displaced_step_finish_status",
>      name="displaced_step_finish",
> -    params=[("thread_info *", "thread"), ("gdb_signal", "sig")],
> +    params=[("thread_info *", "thread"), ("const target_waitstatus &", "ws")],
>      predefault="NULL",
>      invalid="(! gdbarch->displaced_step_finish) != (! gdbarch->displaced_step_prepare)",
>  )
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index a663316df16..5c9390ea6b3 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1080,8 +1080,8 @@ extern void set_gdbarch_displaced_step_prepare (struct gdbarch *gdbarch, gdbarch
>  
>  /* Clean up after a displaced step of THREAD. */
>  
> -typedef displaced_step_finish_status (gdbarch_displaced_step_finish_ftype) (struct gdbarch *gdbarch, thread_info *thread, gdb_signal sig);
> -extern displaced_step_finish_status gdbarch_displaced_step_finish (struct gdbarch *gdbarch, thread_info *thread, gdb_signal sig);
> +typedef displaced_step_finish_status (gdbarch_displaced_step_finish_ftype) (struct gdbarch *gdbarch, thread_info *thread, const target_waitstatus &ws);
> +extern displaced_step_finish_status gdbarch_displaced_step_finish (struct gdbarch *gdbarch, thread_info *thread, const target_waitstatus &ws);
>  extern void set_gdbarch_displaced_step_finish (struct gdbarch *gdbarch, gdbarch_displaced_step_finish_ftype *displaced_step_finish);
>  
>  /* Return the closure associated to the displaced step buffer that is at ADDR. */
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index ddb8dec1c72..559b1dea0d9 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -4097,13 +4097,13 @@ set_gdbarch_displaced_step_prepare (struct gdbarch *gdbarch,
>  }
>  
>  displaced_step_finish_status
> -gdbarch_displaced_step_finish (struct gdbarch *gdbarch, thread_info *thread, gdb_signal sig)
> +gdbarch_displaced_step_finish (struct gdbarch *gdbarch, thread_info *thread, const target_waitstatus &ws)
>  {
>    gdb_assert (gdbarch != NULL);
>    gdb_assert (gdbarch->displaced_step_finish != NULL);
>    if (gdbarch_debug >= 2)
>      gdb_printf (gdb_stdlog, "gdbarch_displaced_step_finish called\n");
> -  return gdbarch->displaced_step_finish (gdbarch, thread, sig);
> +  return gdbarch->displaced_step_finish (gdbarch, thread, ws);
>  }
>  
>  void
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index b24cc6d932d..0590310ffac 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1894,7 +1894,8 @@ displaced_step_prepare (thread_info *thread)
>     DISPLACED_STEP_FINISH_STATUS_OK as well.  */
>  
>  static displaced_step_finish_status
> -displaced_step_finish (thread_info *event_thread, enum gdb_signal signal)
> +displaced_step_finish (thread_info *event_thread,
> +		       const target_waitstatus &event_status)
>  {
>    displaced_step_thread_state *displaced = &event_thread->displaced_step_state;
>  
> @@ -1916,7 +1917,7 @@ displaced_step_finish (thread_info *event_thread, enum gdb_signal signal)
>    /* Do the fixup, and release the resources acquired to do the displaced
>       step. */
>    return gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
> -					event_thread, signal);
> +					event_thread, event_status);
>  }
>  
>  /* Data to be passed around while handling an event.  This data is
> @@ -5068,7 +5069,7 @@ handle_one (const wait_one_event &event)
>  	  /* We caught the event that we intended to catch, so
>  	     there's no event to save as pending.  */
>  
> -	  if (displaced_step_finish (t, GDB_SIGNAL_0)
> +	  if (displaced_step_finish (t, event.ws)
>  	      == DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED)
>  	    {
>  	      /* Add it back to the step-over queue.  */
> @@ -5083,7 +5084,6 @@ handle_one (const wait_one_event &event)
>  	}
>        else
>  	{
> -	  enum gdb_signal sig;
>  	  struct regcache *regcache;
>  
>  	  infrun_debug_printf
> @@ -5094,10 +5094,7 @@ handle_one (const wait_one_event &event)
>  	  /* Record for later.  */
>  	  save_waitstatus (t, event.ws);
>  
> -	  sig = (event.ws.kind () == TARGET_WAITKIND_STOPPED
> -		 ? event.ws.sig () : GDB_SIGNAL_0);
> -
> -	  if (displaced_step_finish (t, sig)
> +	  if (displaced_step_finish (t, event.ws)
>  	      == DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED)
>  	    {
>  	      /* Add it back to the step-over queue.  */
> @@ -5699,7 +5696,7 @@ handle_inferior_event (struct execution_control_state *ecs)
>  	       has been done.  Perform cleanup for parent process here.  Note
>  	       that this operation also cleans up the child process for vfork,
>  	       because their pages are shared.  */
> -	    displaced_step_finish (ecs->event_thread, GDB_SIGNAL_TRAP);
> +	    displaced_step_finish (ecs->event_thread, ecs->ws);

This change is interesting.

If I understand the code correctly, this call will eventually end up in
displaced_step_buffers::finish (displaced-stepping.c), which in turn
calls displaced_step_instruction_executed_successfully.

Previously, we always passed GDB_SIGNAL_TRAP here, which (if we ignore
the watchpoint check in
displaced_step_instruction_executed_successfully) means that
displaced_step_instruction_executed_successfully would always return
true, and then displaced_step_buffers::finish would call
gdbarch_displaced_step_fixup.

After this change, we know that esc->ws.kind is either
TARGET_WAITKIND_FORKED or  TARGET_WAITKIND_VFORKED, so we know that
displaced_step_instruction_executed_successfully will always return
false, and displaced_step_buffers::finish will no longer call
gdbarch_displaced_step_fixup.

What I don't understand well enough is what this actually means for a
running inferior.

It's odd because the comment in infrun.c (just above your change)
indicates that to get to this point the displaced step must have
completed successfully, while after this change, the new code path in
displaced_step_buffers::finish indicates we believe the displaced step
didn't complete successfully:

  /* Since the instruction didn't complete, all we can do is relocate the
     PC.  */

Do you know if any of our test cases hit this path?

Thanks,
Andrew

>  	    /* Start a new step-over in another thread if there's one
>  	       that needs it.  */
>  	    start_step_over ();
> @@ -6064,7 +6061,7 @@ resumed_thread_with_pending_status (struct thread_info *tp,
>  static int
>  finish_step_over (struct execution_control_state *ecs)
>  {
> -  displaced_step_finish (ecs->event_thread, ecs->event_thread->stop_signal ());
> +  displaced_step_finish (ecs->event_thread, ecs->ws);
>  
>    bool had_step_over_info = step_over_info_valid_p ();
>  
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index c30d9fb13f8..8a1d701d7c9 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -2621,13 +2621,14 @@ linux_displaced_step_prepare (gdbarch *arch, thread_info *thread,
>  /* See linux-tdep.h.  */
>  
>  displaced_step_finish_status
> -linux_displaced_step_finish (gdbarch *arch, thread_info *thread, gdb_signal sig)
> +linux_displaced_step_finish (gdbarch *arch, thread_info *thread,
> +			     const target_waitstatus &status)
>  {
>    linux_info *per_inferior = get_linux_inferior_data (thread->inf);
>  
>    gdb_assert (per_inferior->disp_step_bufs.has_value ());
>  
> -  return per_inferior->disp_step_bufs->finish (arch, thread, sig);
> +  return per_inferior->disp_step_bufs->finish (arch, thread, status);
>  }
>  
>  /* See linux-tdep.h.  */
> diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
> index 95cc29c828c..cba67351574 100644
> --- a/gdb/linux-tdep.h
> +++ b/gdb/linux-tdep.h
> @@ -72,7 +72,7 @@ extern displaced_step_prepare_status linux_displaced_step_prepare
>  /* Implementation of gdbarch_displaced_step_finish.  */
>  
>  extern displaced_step_finish_status linux_displaced_step_finish
> -  (gdbarch *arch, thread_info *thread, gdb_signal sig);
> +  (gdbarch *arch, thread_info *thread, const target_waitstatus &status);
>  
>  /* Implementation of gdbarch_displaced_step_copy_insn_closure_by_addr.  */
>  
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index cbd84514795..dc0f78ed9ab 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1088,13 +1088,13 @@ ppc_displaced_step_prepare  (gdbarch *arch, thread_info *thread,
>  
>  static displaced_step_finish_status
>  ppc_displaced_step_finish (gdbarch *arch, thread_info *thread,
> -			   gdb_signal sig)
> +			   const target_waitstatus &status)
>  {
>    ppc_inferior_data *per_inferior = get_ppc_per_inferior (thread->inf);
>  
>    gdb_assert (per_inferior->disp_step_buf.has_value ());
>  
> -  return per_inferior->disp_step_buf->finish (arch, thread, sig);
> +  return per_inferior->disp_step_buf->finish (arch, thread, status);
>  }
>  
>  /* Implementation of gdbarch_displaced_step_restore_all_in_ptid.  */
> -- 
> 2.36.0