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

  This commit tweaks displaced_step_finish & friends to pass down a
target_waitstatus instead of a gdb_signal.  This needed because a
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(-)
  

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
  

On 2023-02-03 10:44 a.m., Andrew Burgess wrote:
> 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".

Fixed.

>> @@ -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.
> 

Good catch!

I was tweaking the change to address your comment, and was coming to the
conclusion that what I really wanted was this:

static bool
displaced_step_instruction_executed_successfully
  (gdbarch *arch, const target_waitstatus &status)
{
  if (status.kind () == TARGET_WAITKIND_STOPPED
      && status.sig () != GDB_SIGNAL_TRAP)
    return false;

  /* All other waitkinds can only happen if the instruction fully
     executed.  For example, a fork, or a syscall entry can only
     happen if the syscall instruction actually executed.  */

(the comment is new)

And then, I remembered, I actually wrote that early if like that originally,
and I changed it in response to this review:

 https://inbox.sourceware.org/gdb-patches/44f74af8-248b-1af8-3612-980c08607bf4@simark.ca/

The review was totally right, it was my response that was misguided.

But I'm confused because I am pretty sure that I wrote a reply to that
message, saying that I did not intend to change the behavior, so I'd "fix" it.
I can't find it in my outbox either, I guess I erroneously canceled my
email window instead of sending the message...

Anyhow, looks like I made it worse while trying to address Simon's comment.  :-P

So I think I should go back to what I had, like before, and my response
to Simon should have been instead:

 - yes, the change is intended.  If we stopped for an event other than
TARGET_WAITKIND_STOPPED, like for instance, TARGET_WAITKIND_FORKED,
TARGET_WAITKIND_SYSCALL_ENTRY, then it must be that the instruction
executed successfully, otherwise the syscall wouldn't have triggered.

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

I know that you posted a series for this, which I plan to take a good look
at (I actually planned on doing that earlier this week, but I had a couple
major distractions, sorry).

WDYT of the version below?

From 0ccfd6822ef83eddb48c6f7bf21533858f177bc9 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Tue, 22 Jun 2021 15:42:51 +0100
Subject: [PATCH] displaced step: pass down target_waitstatus instead of
 gdb_signal

This commit tweaks displaced_step_finish & friends to pass down a
target_waitstatus instead of a gdb_signal.  This is needed because a
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 later in
the series.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
Change-Id: I4c5d338647b028071bc498c4e47063795a2db4c0
---
 gdb/displaced-stepping.c  | 16 +++++++++++-----
 gdb/displaced-stepping.h  |  2 +-
 gdb/gdbarch-gen.h         |  4 ++--
 gdb/gdbarch.c             |  4 ++--
 gdb/gdbarch_components.py |  2 +-
 gdb/infrun.c              | 17 +++++++----------
 gdb/linux-tdep.c          |  5 +++--
 gdb/linux-tdep.h          |  2 +-
 gdb/rs6000-tdep.c         |  4 ++--
 9 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
index 06b32a80f6a..08ae1ce4397 100644
--- a/gdb/displaced-stepping.c
+++ b/gdb/displaced-stepping.c
@@ -192,12 +192,18 @@ 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;
 
+  /* All other (thread event) waitkinds can only happen if the
+     instruction fully executed.  For example, a fork, or a syscall
+     entry can only happen if the syscall instruction actually
+     executed.  */
+
   if (target_stopped_by_watchpoint ())
     {
       if (gdbarch_have_nonsteppable_watchpoint (arch)
@@ -210,7 +216,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 +262,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 e154927ad92..d2c3fc1b2b4 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-gen.h b/gdb/gdbarch-gen.h
index ddb97f60315..5805dcb75ef 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1103,8 +1103,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 7e4e34d5aca..0b122a86055 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -4096,13 +4096,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/gdbarch_components.py b/gdb/gdbarch_components.py
index caa65c334ec..8d72bff347e 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -1878,7 +1878,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/infrun.c b/gdb/infrun.c
index ab77300f1ff..99f2a8e039d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1895,7 +1895,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;
 
@@ -1917,7 +1918,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
@@ -5122,7 +5123,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.  */
@@ -5137,7 +5138,6 @@ handle_one (const wait_one_event &event)
 	}
       else
 	{
-	  enum gdb_signal sig;
 	  struct regcache *regcache;
 
 	  infrun_debug_printf
@@ -5148,10 +5148,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.  */
@@ -5753,7 +5750,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);
 	    /* Start a new step-over in another thread if there's one
 	       that needs it.  */
 	    start_step_over ();
@@ -6118,7 +6115,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 e6ce13a1c67..036ea9f931d 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 16e1b806b26..e09a6ef32b1 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 104515de030..f9bbc435184 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.  */

base-commit: 2562954ede66f32bff7d985e752b8052c2ae5775
  

Pedro Alves <pedro@palves.net> writes:

> On 2023-02-03 10:44 a.m., Andrew Burgess wrote:
>> 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".
>
> Fixed.
>
>>> @@ -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.
>> 
>
> Good catch!
>
> I was tweaking the change to address your comment, and was coming to the
> conclusion that what I really wanted was this:
>
> static bool
> displaced_step_instruction_executed_successfully
>   (gdbarch *arch, const target_waitstatus &status)
> {
>   if (status.kind () == TARGET_WAITKIND_STOPPED
>       && status.sig () != GDB_SIGNAL_TRAP)
>     return false;
>
>   /* All other waitkinds can only happen if the instruction fully
>      executed.  For example, a fork, or a syscall entry can only
>      happen if the syscall instruction actually executed.  */
>
> (the comment is new)
>
> And then, I remembered, I actually wrote that early if like that originally,
> and I changed it in response to this review:
>
>  https://inbox.sourceware.org/gdb-patches/44f74af8-248b-1af8-3612-980c08607bf4@simark.ca/
>
> The review was totally right, it was my response that was misguided.
>
> But I'm confused because I am pretty sure that I wrote a reply to that
> message, saying that I did not intend to change the behavior, so I'd "fix" it.
> I can't find it in my outbox either, I guess I erroneously canceled my
> email window instead of sending the message...
>
> Anyhow, looks like I made it worse while trying to address Simon's comment.  :-P
>
> So I think I should go back to what I had, like before, and my response
> to Simon should have been instead:
>
>  - yes, the change is intended.  If we stopped for an event other than
> TARGET_WAITKIND_STOPPED, like for instance, TARGET_WAITKIND_FORKED,
> TARGET_WAITKIND_SYSCALL_ENTRY, then it must be that the instruction
> executed successfully, otherwise the syscall wouldn't have triggered.
>
>> 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?
>> 
>
> I know that you posted a series for this, which I plan to take a good look
> at (I actually planned on doing that earlier this week, but I had a couple
> major distractions, sorry).
>
> WDYT of the version below?

I took a look through and I'm happy with it.  But I would like you to
consider holding off until my displaced step patch has some feedback.

I'm currently rebasing the patch.  My patch deletes
displaced_step_instruction_executed_successfully, which I realised makes
passing the signal (or now target_waitstatus) redundant.  As such I'm
just testing an additional patch in the series which touches every place
you're touching - but removes the signal instead of changing it.

I hope to post my updated series later today (once testing completes).

Thanks,
Andrew

>
> From 0ccfd6822ef83eddb48c6f7bf21533858f177bc9 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Tue, 22 Jun 2021 15:42:51 +0100
> Subject: [PATCH] displaced step: pass down target_waitstatus instead of
>  gdb_signal
>
> This commit tweaks displaced_step_finish & friends to pass down a
> target_waitstatus instead of a gdb_signal.  This is needed because a
> 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 later in
> the series.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
> Change-Id: I4c5d338647b028071bc498c4e47063795a2db4c0
> ---
>  gdb/displaced-stepping.c  | 16 +++++++++++-----
>  gdb/displaced-stepping.h  |  2 +-
>  gdb/gdbarch-gen.h         |  4 ++--
>  gdb/gdbarch.c             |  4 ++--
>  gdb/gdbarch_components.py |  2 +-
>  gdb/infrun.c              | 17 +++++++----------
>  gdb/linux-tdep.c          |  5 +++--
>  gdb/linux-tdep.h          |  2 +-
>  gdb/rs6000-tdep.c         |  4 ++--
>  9 files changed, 30 insertions(+), 26 deletions(-)
>
> diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
> index 06b32a80f6a..08ae1ce4397 100644
> --- a/gdb/displaced-stepping.c
> +++ b/gdb/displaced-stepping.c
> @@ -192,12 +192,18 @@ 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;
>  
> +  /* All other (thread event) waitkinds can only happen if the
> +     instruction fully executed.  For example, a fork, or a syscall
> +     entry can only happen if the syscall instruction actually
> +     executed.  */
> +
>    if (target_stopped_by_watchpoint ())
>      {
>        if (gdbarch_have_nonsteppable_watchpoint (arch)
> @@ -210,7 +216,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 +262,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 e154927ad92..d2c3fc1b2b4 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-gen.h b/gdb/gdbarch-gen.h
> index ddb97f60315..5805dcb75ef 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1103,8 +1103,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 7e4e34d5aca..0b122a86055 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -4096,13 +4096,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/gdbarch_components.py b/gdb/gdbarch_components.py
> index caa65c334ec..8d72bff347e 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -1878,7 +1878,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/infrun.c b/gdb/infrun.c
> index ab77300f1ff..99f2a8e039d 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1895,7 +1895,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;
>  
> @@ -1917,7 +1918,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
> @@ -5122,7 +5123,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.  */
> @@ -5137,7 +5138,6 @@ handle_one (const wait_one_event &event)
>  	}
>        else
>  	{
> -	  enum gdb_signal sig;
>  	  struct regcache *regcache;
>  
>  	  infrun_debug_printf
> @@ -5148,10 +5148,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.  */
> @@ -5753,7 +5750,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);
>  	    /* Start a new step-over in another thread if there's one
>  	       that needs it.  */
>  	    start_step_over ();
> @@ -6118,7 +6115,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 e6ce13a1c67..036ea9f931d 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 16e1b806b26..e09a6ef32b1 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 104515de030..f9bbc435184 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.  */
>
> base-commit: 2562954ede66f32bff7d985e752b8052c2ae5775
> -- 
> 2.36.0
  

Andrew Burgess <aburgess@redhat.com> writes:

> Pedro Alves <pedro@palves.net> writes:
>
>> On 2023-02-03 10:44 a.m., Andrew Burgess wrote:
>>> 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".
>>
>> Fixed.
>>
>>>> @@ -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.
>>> 
>>
>> Good catch!
>>
>> I was tweaking the change to address your comment, and was coming to the
>> conclusion that what I really wanted was this:
>>
>> static bool
>> displaced_step_instruction_executed_successfully
>>   (gdbarch *arch, const target_waitstatus &status)
>> {
>>   if (status.kind () == TARGET_WAITKIND_STOPPED
>>       && status.sig () != GDB_SIGNAL_TRAP)
>>     return false;
>>
>>   /* All other waitkinds can only happen if the instruction fully
>>      executed.  For example, a fork, or a syscall entry can only
>>      happen if the syscall instruction actually executed.  */
>>
>> (the comment is new)
>>
>> And then, I remembered, I actually wrote that early if like that originally,
>> and I changed it in response to this review:
>>
>>  https://inbox.sourceware.org/gdb-patches/44f74af8-248b-1af8-3612-980c08607bf4@simark.ca/
>>
>> The review was totally right, it was my response that was misguided.
>>
>> But I'm confused because I am pretty sure that I wrote a reply to that
>> message, saying that I did not intend to change the behavior, so I'd "fix" it.
>> I can't find it in my outbox either, I guess I erroneously canceled my
>> email window instead of sending the message...
>>
>> Anyhow, looks like I made it worse while trying to address Simon's comment.  :-P
>>
>> So I think I should go back to what I had, like before, and my response
>> to Simon should have been instead:
>>
>>  - yes, the change is intended.  If we stopped for an event other than
>> TARGET_WAITKIND_STOPPED, like for instance, TARGET_WAITKIND_FORKED,
>> TARGET_WAITKIND_SYSCALL_ENTRY, then it must be that the instruction
>> executed successfully, otherwise the syscall wouldn't have triggered.
>>
>>> 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?
>>> 
>>
>> I know that you posted a series for this, which I plan to take a good look
>> at (I actually planned on doing that earlier this week, but I had a couple
>> major distractions, sorry).
>>
>> WDYT of the version below?
>
> I took a look through and I'm happy with it.  But I would like you to
> consider holding off until my displaced step patch has some feedback.
>
> I'm currently rebasing the patch.  My patch deletes
> displaced_step_instruction_executed_successfully, which I realised makes
> passing the signal (or now target_waitstatus) redundant.  As such I'm
> just testing an additional patch in the series which touches every place
> you're touching - but removes the signal instead of changing it.
>
> I hope to post my updated series later today (once testing completes).

Pedro,

Thanks for your feedback on my displaced stepping series. You're right
that just checking the $pc isn't going to be enough.  So I'm now
thinking that I should be passing the gdb_signal through to the
gdbarch_displaced_step_fixup function.

Rather than change things to pass through the gdb_signal though, and
then have this patch come along and s/gdb_signal/target_waitstatus/, I
wonder how you'd feel about merging this patch sooner rather than later?

I'm planning to rebase my displaced stepping series off this patch -- I
just want to check you'd be OK with this patch possibly landing before
the rest of this series?

Thanks,
Andrew
  

Hi!

On 2023-03-22 9:29 p.m., Andrew Burgess wrote:
> Andrew Burgess <aburgess@redhat.com> writes:
> 
...
>> I took a look through and I'm happy with it.  But I would like you to
>> consider holding off until my displaced step patch has some feedback.
>>
>> I'm currently rebasing the patch.  My patch deletes
>> displaced_step_instruction_executed_successfully, which I realised makes
>> passing the signal (or now target_waitstatus) redundant.  As such I'm
>> just testing an additional patch in the series which touches every place
>> you're touching - but removes the signal instead of changing it.
>>
>> I hope to post my updated series later today (once testing completes).
> 
> Pedro,
> 
> Thanks for your feedback on my displaced stepping series. You're right
> that just checking the $pc isn't going to be enough.  So I'm now
> thinking that I should be passing the gdb_signal through to the
> gdbarch_displaced_step_fixup function.

I guess passing down a boolean (or enum) indicating "success/failure" would
also be an alternative.  That may avoid every arch having to do the same check.

> 
> Rather than change things to pass through the gdb_signal though, and
> then have this patch come along and s/gdb_signal/target_waitstatus/, I
> wonder how you'd feel about merging this patch sooner rather than later?
> 
> I'm planning to rebase my displaced stepping series off this patch -- I
> just want to check you'd be OK with this patch possibly landing before
> the rest of this series?

I'm absolutely fine with putting this in before the rest of this series.

How about I just merge it immediately, so none of us have to carry it around?

I've updated the commit log a bit so it doesn't assume it is being merged along
the rest of the series.

From 7abd5c8ee74055621dc3a73fbcf2e8940712bb01 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Tue, 22 Jun 2021 15:42:51 +0100
Subject: [PATCH] displaced step: pass down target_waitstatus instead of
 gdb_signal

This commit tweaks displaced_step_finish & friends to pass down a
target_waitstatus instead of a gdb_signal.  This is needed because a
patch later in the step-over-{thread-exit,clone] series will want to
make displaced_step_buffers::finish handle
TARGET_WAITKIND_THREAD_EXITED.  It also helps with the
TARGET_WAITKIND_THREAD_CLONED patch later in that same series.

It's also a bit more logical this way, as we don't have to pass down
signals when the thread didn't actually stop for a signal.  So we can
also think of it as a clean up.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
Change-Id: I4c5d338647b028071bc498c4e47063795a2db4c0
Approved-By: Andrew Burgess <aburgess@redhat.com>
---
 gdb/displaced-stepping.c  | 16 +++++++++++-----
 gdb/displaced-stepping.h  |  2 +-
 gdb/gdbarch-gen.h         |  4 ++--
 gdb/gdbarch.c             |  4 ++--
 gdb/gdbarch_components.py |  2 +-
 gdb/infrun.c              | 17 +++++++----------
 gdb/linux-tdep.c          |  5 +++--
 gdb/linux-tdep.h          |  2 +-
 gdb/rs6000-tdep.c         |  4 ++--
 9 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
index 9f98ea8c35b..3fefdf322d8 100644
--- a/gdb/displaced-stepping.c
+++ b/gdb/displaced-stepping.c
@@ -192,12 +192,18 @@ 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;
 
+  /* All other (thread event) waitkinds can only happen if the
+     instruction fully executed.  For example, a fork, or a syscall
+     entry can only happen if the syscall instruction actually
+     executed.  */
+
   if (target_stopped_by_watchpoint ())
     {
       if (gdbarch_have_nonsteppable_watchpoint (arch)
@@ -210,7 +216,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 +262,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 e154927ad92..d2c3fc1b2b4 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-gen.h b/gdb/gdbarch-gen.h
index a3fc0b9272b..bbf2376fc4b 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -1101,8 +1101,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 b676e346fd0..84beb087336 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -4098,13 +4098,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/gdbarch_components.py b/gdb/gdbarch_components.py
index 2b1a2b4f602..52beaeaa245 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -1819,7 +1819,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/infrun.c b/gdb/infrun.c
index 5c9babb9104..ee812baf8da 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1895,7 +1895,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;
 
@@ -1917,7 +1918,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
@@ -5128,7 +5129,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.  */
@@ -5143,7 +5144,6 @@ handle_one (const wait_one_event &event)
 	}
       else
 	{
-	  enum gdb_signal sig;
 	  struct regcache *regcache;
 
 	  infrun_debug_printf
@@ -5154,10 +5154,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.  */
@@ -5759,7 +5756,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);
 	    /* Start a new step-over in another thread if there's one
 	       that needs it.  */
 	    start_step_over ();
@@ -6124,7 +6121,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 d0bda5ad9c4..1fc9cb6faee 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -2626,13 +2626,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 16e1b806b26..e09a6ef32b1 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 52dcc89b2df..8b400047cfb 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1089,13 +1089,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.  */

base-commit: 91ffa03af1cc32515190c3b52d7b964f5abead5f
  

Pedro Alves <pedro@palves.net> writes:

> Hi!
>
> On 2023-03-22 9:29 p.m., Andrew Burgess wrote:
>> Andrew Burgess <aburgess@redhat.com> writes:
>> 
> ...
>>> I took a look through and I'm happy with it.  But I would like you to
>>> consider holding off until my displaced step patch has some feedback.
>>>
>>> I'm currently rebasing the patch.  My patch deletes
>>> displaced_step_instruction_executed_successfully, which I realised makes
>>> passing the signal (or now target_waitstatus) redundant.  As such I'm
>>> just testing an additional patch in the series which touches every place
>>> you're touching - but removes the signal instead of changing it.
>>>
>>> I hope to post my updated series later today (once testing completes).
>> 
>> Pedro,
>> 
>> Thanks for your feedback on my displaced stepping series. You're right
>> that just checking the $pc isn't going to be enough.  So I'm now
>> thinking that I should be passing the gdb_signal through to the
>> gdbarch_displaced_step_fixup function.
>
> I guess passing down a boolean (or enum) indicating "success/failure" would
> also be an alternative.  That may avoid every arch having to do the
> same check.

That's what I did in the end.  I posted a V3 for my displaced step series.

>
>> 
>> Rather than change things to pass through the gdb_signal though, and
>> then have this patch come along and s/gdb_signal/target_waitstatus/, I
>> wonder how you'd feel about merging this patch sooner rather than later?
>> 
>> I'm planning to rebase my displaced stepping series off this patch -- I
>> just want to check you'd be OK with this patch possibly landing before
>> the rest of this series?
>
> I'm absolutely fine with putting this in before the rest of this series.
>
> How about I just merge it immediately, so none of us have to carry it
> around?

That would be great, as our patches will have a minor merge conflict in
displaced-stepping.c.

>
> I've updated the commit log a bit so it doesn't assume it is being merged along
> the rest of the series.
>
> From 7abd5c8ee74055621dc3a73fbcf2e8940712bb01 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Tue, 22 Jun 2021 15:42:51 +0100
> Subject: [PATCH] displaced step: pass down target_waitstatus instead of
>  gdb_signal
>
> This commit tweaks displaced_step_finish & friends to pass down a
> target_waitstatus instead of a gdb_signal.  This is needed because a
> patch later in the step-over-{thread-exit,clone] series will want to
> make displaced_step_buffers::finish handle
> TARGET_WAITKIND_THREAD_EXITED.  It also helps with the
> TARGET_WAITKIND_THREAD_CLONED patch later in that same series.
>
> It's also a bit more logical this way, as we don't have to pass down
> signals when the thread didn't actually stop for a signal.  So we can
> also think of it as a clean up.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
> Change-Id: I4c5d338647b028071bc498c4e47063795a2db4c0
> Approved-By: Andrew Burgess <aburgess@redhat.com>

LGTM.

Thanks,
Andrew

> ---
>  gdb/displaced-stepping.c  | 16 +++++++++++-----
>  gdb/displaced-stepping.h  |  2 +-
>  gdb/gdbarch-gen.h         |  4 ++--
>  gdb/gdbarch.c             |  4 ++--
>  gdb/gdbarch_components.py |  2 +-
>  gdb/infrun.c              | 17 +++++++----------
>  gdb/linux-tdep.c          |  5 +++--
>  gdb/linux-tdep.h          |  2 +-
>  gdb/rs6000-tdep.c         |  4 ++--
>  9 files changed, 30 insertions(+), 26 deletions(-)
>
> diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
> index 9f98ea8c35b..3fefdf322d8 100644
> --- a/gdb/displaced-stepping.c
> +++ b/gdb/displaced-stepping.c
> @@ -192,12 +192,18 @@ 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;
>  
> +  /* All other (thread event) waitkinds can only happen if the
> +     instruction fully executed.  For example, a fork, or a syscall
> +     entry can only happen if the syscall instruction actually
> +     executed.  */
> +
>    if (target_stopped_by_watchpoint ())
>      {
>        if (gdbarch_have_nonsteppable_watchpoint (arch)
> @@ -210,7 +216,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 +262,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 e154927ad92..d2c3fc1b2b4 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-gen.h b/gdb/gdbarch-gen.h
> index a3fc0b9272b..bbf2376fc4b 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1101,8 +1101,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 b676e346fd0..84beb087336 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -4098,13 +4098,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/gdbarch_components.py b/gdb/gdbarch_components.py
> index 2b1a2b4f602..52beaeaa245 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -1819,7 +1819,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/infrun.c b/gdb/infrun.c
> index 5c9babb9104..ee812baf8da 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1895,7 +1895,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;
>  
> @@ -1917,7 +1918,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
> @@ -5128,7 +5129,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.  */
> @@ -5143,7 +5144,6 @@ handle_one (const wait_one_event &event)
>  	}
>        else
>  	{
> -	  enum gdb_signal sig;
>  	  struct regcache *regcache;
>  
>  	  infrun_debug_printf
> @@ -5154,10 +5154,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.  */
> @@ -5759,7 +5756,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);
>  	    /* Start a new step-over in another thread if there's one
>  	       that needs it.  */
>  	    start_step_over ();
> @@ -6124,7 +6121,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 d0bda5ad9c4..1fc9cb6faee 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -2626,13 +2626,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 16e1b806b26..e09a6ef32b1 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 52dcc89b2df..8b400047cfb 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1089,13 +1089,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.  */
>
> base-commit: 91ffa03af1cc32515190c3b52d7b964f5abead5f
> -- 
> 2.36.0
  

On 2023-03-27 1:40 p.m., Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:

>> I'm absolutely fine with putting this in before the rest of this series.
>>
>> How about I just merge it immediately, so none of us have to carry it
>> around?
> 
> That would be great, as our patches will have a minor merge conflict in
> displaced-stepping.c.

...

> 
> LGTM.

Alright, pushed.

Thanks,
Pedro Alves