[PATCH 0/3] Add displaced stepping support for AMD GPUs

[PATCH 0/3] Add displaced stepping support for AMD GPUs

[simon.marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

The AMD GPU port is still very barebone (i.e. not useful as-is), what's
blocking is mostly waiting for the DWARF extensions proposed by AMD to
be standardized.  But displaced stepping is a small piece that can be
upstreamed and tested without needing DWARF.

Simon Marchi (3):
  gdb/amd-dbgapi: use gdb::unordered_map
  gdb: add target displaced stepping support
  gdb/amd-dbgapi: add displaced stepping support

 gdb/amd-dbgapi-target.c                       | 143 +++++++++++++++++-
 gdb/displaced-stepping.c                      |  43 ++++++
 gdb/displaced-stepping.h                      |  67 ++++++++
 gdb/infrun.c                                  |  42 ++---
 gdb/target-debug.h                            |  18 +++
 gdb/target-delegates-gen.c                    | 108 +++++++++++++
 gdb/target.h                                  |  19 +++
 gdb/testsuite/gdb.rocm/displaced-stepping.exp |  54 +++++++
 8 files changed, 465 insertions(+), 29 deletions(-)
 create mode 100644 gdb/testsuite/gdb.rocm/displaced-stepping.exp


base-commit: d3685ec080cc5bfb9646cdc1f5ddda0c3da92b76
-- 
2.47.1

[PATCH 1/3] gdb/amd-dbgapi: use gdb::unordered_map

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Since we have gdb::unordered_map, swap std::unordered_map for that.

Change-Id: If2ef652fe18c1a440a25cff6131d29e37091bdbe
---
 gdb/amd-dbgapi-target.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
index 2bb79acd76f0..153a35f788ed 100644
--- a/gdb/amd-dbgapi-target.c
+++ b/gdb/amd-dbgapi-target.c
@@ -24,6 +24,7 @@
 #include "cli/cli-cmds.h"
 #include "cli/cli-decode.h"
 #include "cli/cli-style.h"
+#include "gdbsupport/unordered_map.h"
 #include "inf-loop.h"
 #include "inferior.h"
 #include "objfiles.h"
@@ -207,7 +208,7 @@ struct amd_dbgapi_inferior_info
     bool enabled = false;
   } precise_memory;
 
-  std::unordered_map<decltype (amd_dbgapi_breakpoint_id_t::handle),
+  gdb::unordered_map<decltype (amd_dbgapi_breakpoint_id_t::handle),
 		     struct breakpoint *>
     breakpoint_map;
 
@@ -221,7 +222,7 @@ struct amd_dbgapi_inferior_info
 
      wave_info objects are added when we first see the wave, and
      removed from a thread_deleted observer.  */
-  std::unordered_map<decltype (amd_dbgapi_wave_id_t::handle), wave_info>
+  gdb::unordered_map<decltype (amd_dbgapi_wave_id_t::handle), wave_info>
     wave_info_map;
 };
 
-- 
2.47.1

Re: [PATCH 1/3] gdb/amd-dbgapi: use gdb::unordered_map

[Lancelot SIX]

On 14/01/2025 19:16, simon.marchi@polymtl.ca wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> Since we have gdb::unordered_map, swap std::unordered_map for that.
> 

Hi,

That looks good to me, thanks.

Since it only touches the amdgpu backend,
Approved-By: Lancelot Six <lancelot.six@amd.com> (amdgpu)

Best,
Lancelot.

> Change-Id: If2ef652fe18c1a440a25cff6131d29e37091bdbe
> ---
>   gdb/amd-dbgapi-target.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
> index 2bb79acd76f0..153a35f788ed 100644
> --- a/gdb/amd-dbgapi-target.c
> +++ b/gdb/amd-dbgapi-target.c
> @@ -24,6 +24,7 @@
>   #include "cli/cli-cmds.h"
>   #include "cli/cli-decode.h"
>   #include "cli/cli-style.h"
> +#include "gdbsupport/unordered_map.h"
>   #include "inf-loop.h"
>   #include "inferior.h"
>   #include "objfiles.h"
> @@ -207,7 +208,7 @@ struct amd_dbgapi_inferior_info
>       bool enabled = false;
>     } precise_memory;
> 
> -  std::unordered_map<decltype (amd_dbgapi_breakpoint_id_t::handle),
> +  gdb::unordered_map<decltype (amd_dbgapi_breakpoint_id_t::handle),
>                       struct breakpoint *>
>       breakpoint_map;
> 
> @@ -221,7 +222,7 @@ struct amd_dbgapi_inferior_info
> 
>        wave_info objects are added when we first see the wave, and
>        removed from a thread_deleted observer.  */
> -  std::unordered_map<decltype (amd_dbgapi_wave_id_t::handle), wave_info>
> +  gdb::unordered_map<decltype (amd_dbgapi_wave_id_t::handle), wave_info>
>       wave_info_map;
>   };
> 
> --
> 2.47.1
> 

Re: [PATCH 1/3] gdb/amd-dbgapi: use gdb::unordered_map

[Simon Marchi]

On 2025-01-15 05:55, Lancelot SIX wrote:
> 
> 
> On 14/01/2025 19:16, simon.marchi@polymtl.ca wrote:
>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> Since we have gdb::unordered_map, swap std::unordered_map for that.
>>
> 
> Hi,
> 
> That looks good to me, thanks.
> 
> Since it only touches the amdgpu backend,
> Approved-By: Lancelot Six <lancelot.six@amd.com> (amdgpu)

Thanks, added.

Is the (amdgpu) some format we use?  I haven't seen it used anywhere
else.  I don't mind, for instance, Eli could use Approved-By with (doc).

Simon

Re: [PATCH 1/3] gdb/amd-dbgapi: use gdb::unordered_map

[Simon Marchi]

On 2025-01-15 13:41, Simon Marchi wrote:
> 
> 
> On 2025-01-15 05:55, Lancelot SIX wrote:
>>
>>
>> On 14/01/2025 19:16, simon.marchi@polymtl.ca wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> From: Simon Marchi <simon.marchi@efficios.com>
>>>
>>> Since we have gdb::unordered_map, swap std::unordered_map for that.
>>>
>>
>> Hi,
>>
>> That looks good to me, thanks.
>>
>> Since it only touches the amdgpu backend,
>> Approved-By: Lancelot Six <lancelot.six@amd.com> (amdgpu)
> 
> Thanks, added.
> 
> Is the (amdgpu) some format we use?  I haven't seen it used anywhere
> else.  I don't mind, for instance, Eli could use Approved-By with (doc).

Just note that `b4 am` doesn't preserve the parenthesis, so it's not so
handy actually.

Simon

Re: [PATCH 1/3] gdb/amd-dbgapi: use gdb::unordered_map

[Guinevere Larsen]

On 1/15/25 3:46 PM, Simon Marchi wrote:
>
> On 2025-01-15 13:41, Simon Marchi wrote:
>>
>> On 2025-01-15 05:55, Lancelot SIX wrote:
>>>
>>> On 14/01/2025 19:16, simon.marchi@polymtl.ca wrote:
>>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>>
>>>>
>>>> From: Simon Marchi <simon.marchi@efficios.com>
>>>>
>>>> Since we have gdb::unordered_map, swap std::unordered_map for that.
>>>>
>>> Hi,
>>>
>>> That looks good to me, thanks.
>>>
>>> Since it only touches the amdgpu backend,
>>> Approved-By: Lancelot Six <lancelot.six@amd.com> (amdgpu)
>> Thanks, added.
>>
>> Is the (amdgpu) some format we use?  I haven't seen it used anywhere
>> else.  I don't mind, for instance, Eli could use Approved-By with (doc).

It was the thing we agreed on when I added the trailer information to 
the maintainers file. The examples specifically use a comma separated 
list of areas in parenthesis after the tag.

> Just note that `b4 am` doesn't preserve the parenthesis, so it's not so
> handy actually.
>
> Simon
>
Ah, that's unfortunate. We might want to update that format then, but we 
should also update the maintainers file

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

Re: [PATCH 1/3] gdb/amd-dbgapi: use gdb::unordered_map

[Lancelot SIX]

On 15/01/2025 18:41, Simon Marchi wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On 2025-01-15 05:55, Lancelot SIX wrote:
>>
>>
>> On 14/01/2025 19:16, simon.marchi@polymtl.ca wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> From: Simon Marchi <simon.marchi@efficios.com>
>>>
>>> Since we have gdb::unordered_map, swap std::unordered_map for that.
>>>
>>
>> Hi,
>>
>> That looks good to me, thanks.
>>
>> Since it only touches the amdgpu backend,
>> Approved-By: Lancelot Six <lancelot.six@amd.com> (amdgpu)
> 
> Thanks, added.
> 
> Is the (amdgpu) some format we use?  I haven't seen it used anywhere
> else.  I don't mind, for instance, Eli could use Approved-By with (doc).
> 
> Simon

This was discussed when introducing the trailer tags for reviews, and is 
documented in the gdb/MAINTAINERS file (thanks Gwen for that).

   - Approved-By:

    […]
    This trailer can be specific to one or more areas of the project, as
    defined by the "Responsible maintainers" section of this file.  If
    that is the case, the area(s) should be added at the end of the tag
    in parenthesis in a comma separated list.

There are a couple of other instances of that already:

     $ git log origin/master |grep -i Approved-by: | sort |uniq|grep \(
       Approved-By: G... L... <...> (record-full)
       Approved-By: L... S... <...> (amdgpu)
       Approved-By: L... M... <...> (aarch64/arm)

Best,
Lancelot.

[PATCH 2/3] gdb: add target displaced stepping support

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

The amd-dbgapi library, used in the AMD GPU port, has the capability to
prepare and cleanup displaced step operations.  In order to use it, add
the following target_ops methods:

 - supports_displaced_step
 - displaced_step_prepare
 - displaced_step_finish
 - displaced_step_restore_all_in_ptid

Prior to this patch, displaced stepping preparation and cleanup is done
solely by gdbarches.  Update infrun to use these new target methods
instead of gdbarch hooks.  To keep the behavior for other architectures
unchanged, make the default implementations of the new target_ops method
forward to the thread's gdbarch.

displaced_step_restore_all_in_ptid won't be needed for the AMD GPU port,
but was added for completeness.  It would be weird for infrun displaced
stepping code to call target methods except for that one thing where it
calls a gdbarch method.

Since this patch only adds infrastructure, no behavior change is
expected.

Change-Id: I07c68dddb5759a55cd137a711d2679eedc0d9285
---
 gdb/displaced-stepping.c   |  43 +++++++++++++++
 gdb/displaced-stepping.h   |  67 +++++++++++++++++++++++
 gdb/infrun.c               |  42 ++++++---------
 gdb/target-debug.h         |  18 +++++++
 gdb/target-delegates-gen.c | 108 +++++++++++++++++++++++++++++++++++++
 gdb/target.h               |  19 +++++++
 6 files changed, 270 insertions(+), 27 deletions(-)

diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
index 7869ebae7714..ff7f91011082 100644
--- a/gdb/displaced-stepping.c
+++ b/gdb/displaced-stepping.c
@@ -328,3 +328,46 @@ When non-zero, displaced stepping specific debugging is enabled."),
 			    show_debug_displaced,
 			    &setdebuglist, &showdebuglist);
 }
+
+/* See displaced-stepping.h.  */
+
+bool
+default_supports_displaced_step (target_ops *target, thread_info *thread)
+{
+  /* Only check for the presence of `prepare`.  The gdbarch verification ensures
+     that if `prepare` is provided, so is `finish`.  */
+  gdbarch *arch = get_thread_regcache (thread)->arch ();
+  return gdbarch_displaced_step_prepare_p (arch);
+}
+
+/* See displaced-stepping.h.  */
+
+displaced_step_prepare_status
+default_displaced_step_prepare (target_ops *target, thread_info *thread,
+				CORE_ADDR &displaced_pc)
+{
+  gdbarch *arch = get_thread_regcache (thread)->arch ();
+  return gdbarch_displaced_step_prepare (arch, thread, displaced_pc);
+}
+
+/* See displaced-stepping.h.  */
+
+displaced_step_finish_status
+default_displaced_step_finish (target_ops *target,
+			       thread_info *thread,
+			       const target_waitstatus &status)
+{
+  gdbarch *arch = thread->displaced_step_state.get_original_gdbarch ();
+  return gdbarch_displaced_step_finish (arch, thread, status);
+}
+
+/* See displaced-stepping.h.  */
+
+void
+default_displaced_step_restore_all_in_ptid (target_ops *target,
+					    inferior *parent_inf,
+					    ptid_t child_ptid)
+{
+  return gdbarch_displaced_step_restore_all_in_ptid (parent_inf->arch (),
+						     parent_inf, child_ptid);
+}
diff --git a/gdb/displaced-stepping.h b/gdb/displaced-stepping.h
index d7a537a58c92..7949e5dae13f 100644
--- a/gdb/displaced-stepping.h
+++ b/gdb/displaced-stepping.h
@@ -24,6 +24,8 @@
 #include "gdbsupport/byte-vector.h"
 
 struct gdbarch;
+struct inferior;
+struct target_ops;
 struct thread_info;
 
 /* True if we are debugging displaced stepping.  */
@@ -48,6 +50,26 @@ enum displaced_step_prepare_status
   DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE,
 };
 
+/* Return a string representation of STATUS.  */
+
+static inline const char *
+displaced_step_prepare_status_str (displaced_step_prepare_status status)
+{
+  switch (status)
+  {
+  case DISPLACED_STEP_PREPARE_STATUS_OK:
+    return "OK";
+
+  case DISPLACED_STEP_PREPARE_STATUS_CANT:
+    return "CANT";
+
+  case DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE:
+    return "UNAVAILABLE";
+  }
+
+  gdb_assert_not_reached ("invalid displaced_step_prepare_status value");
+}
+
 enum displaced_step_finish_status
 {
   /* Either the instruction was stepped and fixed up, or the specified thread
@@ -59,6 +81,23 @@ enum displaced_step_finish_status
   DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED,
 };
 
+/* Return a string representation of STATUS.  */
+
+static inline const char *
+displaced_step_finish_status_str (displaced_step_finish_status status)
+{
+  switch (status)
+  {
+  case DISPLACED_STEP_FINISH_STATUS_OK:
+    return "OK";
+
+  case DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED:
+    return "NOT_EXECUTED";
+  }
+
+  gdb_assert_not_reached ("invalid displaced_step_finish_status value");
+}
+
 /* Data returned by a gdbarch displaced_step_copy_insn method, to be passed to
    the matching displaced_step_fixup method.  */
 
@@ -207,4 +246,32 @@ struct displaced_step_buffers
   std::vector<displaced_step_buffer> m_buffers;
 };
 
+/* Default implemention of target_ops::supports_displaced_step.
+
+   Forwards the call to the architecture of THREAD.  */
+
+bool default_supports_displaced_step (target_ops *target, thread_info *thread);
+
+/* Default implementation of target_ops::displaced_step_prepare.
+
+   Forwards the call to the architecture of THREAD.  */
+
+displaced_step_prepare_status default_displaced_step_prepare
+  (target_ops *target, thread_info *thread, CORE_ADDR &displaced_pc);
+
+/* Default implementation of target_ops::displaced_step_finish.
+
+   Forwards the call to the architecture of THREAD.  */
+
+displaced_step_finish_status default_displaced_step_finish
+  (target_ops *target, thread_info *thread, const target_waitstatus &status);
+
+/* Default implementation of target_ops::displaced_step_restore_all_in_ptid.
+
+   Forwards the call to the architecture of PARENT_INF.  */
+
+void default_displaced_step_restore_all_in_ptid (target_ops *target,
+						 inferior *parent_inf,
+						 ptid_t child_ptid);
+
 #endif /* GDB_DISPLACED_STEPPING_H */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4687ee6edb39..79fa31e5ee24 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1699,15 +1699,12 @@ show_can_use_displaced_stepping (struct ui_file *file, int from_tty,
 		  "to step over breakpoints is %s.\n"), value);
 }
 
-/* Return true if the gdbarch implements the required methods to use
-   displaced stepping.  */
+/* Return true if the target behind THREAD supports displaced stepping.  */
 
 static bool
-gdbarch_supports_displaced_stepping (gdbarch *arch)
+target_supports_displaced_stepping (thread_info *thread)
 {
-  /* Only check for the presence of `prepare`.  The gdbarch verification ensures
-     that if `prepare` is provided, so is `finish`.  */
-  return gdbarch_displaced_step_prepare_p (arch);
+  return thread->inf->top_target ()->supports_displaced_step (thread);
 }
 
 /* Return non-zero if displaced stepping can/should be used to step
@@ -1726,11 +1723,8 @@ use_displaced_stepping (thread_info *tp)
       && !target_is_non_stop_p ())
     return false;
 
-  gdbarch *gdbarch = get_thread_regcache (tp)->arch ();
-
-  /* If the architecture doesn't implement displaced stepping, don't use
-     it.  */
-  if (!gdbarch_supports_displaced_stepping (gdbarch))
+  /* If the target doesn't support displaced stepping, don't use it.  */
+  if (!target_supports_displaced_stepping (tp))
     return false;
 
   /* If recording, don't use displaced stepping.  */
@@ -1784,9 +1778,9 @@ displaced_step_prepare_throw (thread_info *tp)
   displaced_step_thread_state &disp_step_thread_state
     = tp->displaced_step_state;
 
-  /* We should never reach this function if the architecture does not
+  /* We should never reach this function if the target does not
      support displaced stepping.  */
-  gdb_assert (gdbarch_supports_displaced_stepping (gdbarch));
+  gdb_assert (target_supports_displaced_stepping (tp));
 
   /* Nor if the thread isn't meant to step over a breakpoint.  */
   gdb_assert (tp->control.trap_expected);
@@ -1847,8 +1841,8 @@ displaced_step_prepare_throw (thread_info *tp)
 				paddress (gdbarch, original_pc), dislen);
     }
 
-  displaced_step_prepare_status status
-    = gdbarch_displaced_step_prepare (gdbarch, tp, displaced_pc);
+  auto status
+    = tp->inf->top_target ()->displaced_step_prepare (tp, displaced_pc);
 
   if (status == DISPLACED_STEP_PREPARE_STATUS_CANT)
     {
@@ -2028,6 +2022,7 @@ displaced_step_finish (thread_info *event_thread,
 {
   /* Check whether the parent is displaced stepping.  */
   inferior *parent_inf = event_thread->inf;
+  target_ops *top_target = parent_inf->top_target ();
 
   /* If this was a fork/vfork/clone, this event indicates that the
      displaced stepping of the syscall instruction has been done, so
@@ -2044,15 +2039,10 @@ displaced_step_finish (thread_info *event_thread,
      gdbarch_displaced_step_restore_all_in_ptid.  This is not enforced
      during gdbarch validation to support architectures which support
      displaced stepping but not forks.  */
-  if (event_status.kind () == TARGET_WAITKIND_FORKED)
-    {
-      struct regcache *parent_regcache = get_thread_regcache (event_thread);
-      struct gdbarch *gdbarch = parent_regcache->arch ();
-
-      if (gdbarch_supports_displaced_stepping (gdbarch))
-	gdbarch_displaced_step_restore_all_in_ptid
-	  (gdbarch, parent_inf, event_status.child_ptid ());
-    }
+  if (event_status.kind () == TARGET_WAITKIND_FORKED
+      && target_supports_displaced_stepping (event_thread))
+    top_target->displaced_step_restore_all_in_ptid
+      (parent_inf, event_status.child_ptid ());
 
   displaced_step_thread_state *displaced = &event_thread->displaced_step_state;
 
@@ -2075,9 +2065,7 @@ displaced_step_finish (thread_info *event_thread,
 
   /* Do the fixup, and release the resources acquired to do the displaced
      step. */
-  displaced_step_finish_status status
-    = gdbarch_displaced_step_finish (displaced->get_original_gdbarch (),
-				     event_thread, event_status);
+  auto status = top_target->displaced_step_finish (event_thread, event_status);
 
   if (event_status.kind () == TARGET_WAITKIND_FORKED
       || event_status.kind () == TARGET_WAITKIND_VFORKED
diff --git a/gdb/target-debug.h b/gdb/target-debug.h
index 7530cf06f762..aee7d17f706d 100644
--- a/gdb/target-debug.h
+++ b/gdb/target-debug.h
@@ -145,6 +145,10 @@ static std::string
 target_debug_print_CORE_ADDR_p (CORE_ADDR *p)
 { return core_addr_to_string (*p); }
 
+static std::string
+target_debug_print_CORE_ADDR_r (CORE_ADDR &p)
+{ return core_addr_to_string (p); }
+
 static std::string
 target_debug_print_int_p (int *p)
 { return plongest (*p); }
@@ -306,6 +310,10 @@ static std::string
 target_debug_print_target_waitstatus_p (struct target_waitstatus *status)
 { return status->to_string (); }
 
+static std::string
+target_debug_print_const_target_waitstatus_r (const target_waitstatus &status)
+{ return status.to_string (); }
+
 /* Functions that are used via TARGET_DEBUG_PRINTER.  */
 
 static std::string
@@ -379,4 +387,14 @@ target_debug_print_x86_xsave_layout (const x86_xsave_layout &layout)
 
   return s;
 }
+
+static std::string
+target_debug_print_displaced_step_finish_status (displaced_step_finish_status s)
+{ return displaced_step_finish_status_str (s); }
+
+static std::string
+target_debug_print_displaced_step_prepare_status
+  (displaced_step_prepare_status s)
+{ return displaced_step_prepare_status_str (s); }
+
 #endif /* GDB_TARGET_DEBUG_H */
diff --git a/gdb/target-delegates-gen.c b/gdb/target-delegates-gen.c
index dd20e1404c32..7d34f80ab2df 100644
--- a/gdb/target-delegates-gen.c
+++ b/gdb/target-delegates-gen.c
@@ -199,6 +199,10 @@ struct dummy_target : public target_ops
   bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
   bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override;
   x86_xsave_layout fetch_x86_xsave_layout () override;
+  bool supports_displaced_step (thread_info *arg0) override;
+  displaced_step_prepare_status displaced_step_prepare (thread_info *arg0, CORE_ADDR &arg1) override;
+  displaced_step_finish_status displaced_step_finish (thread_info *arg0, const target_waitstatus &arg1) override;
+  void displaced_step_restore_all_in_ptid (inferior *arg0, ptid_t arg1) override;
 };
 
 struct debug_target : public target_ops
@@ -376,6 +380,10 @@ struct debug_target : public target_ops
   bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
   bool is_address_tagged (gdbarch *arg0, CORE_ADDR arg1) override;
   x86_xsave_layout fetch_x86_xsave_layout () override;
+  bool supports_displaced_step (thread_info *arg0) override;
+  displaced_step_prepare_status displaced_step_prepare (thread_info *arg0, CORE_ADDR &arg1) override;
+  displaced_step_finish_status displaced_step_finish (thread_info *arg0, const target_waitstatus &arg1) override;
+  void displaced_step_restore_all_in_ptid (inferior *arg0, ptid_t arg1) override;
 };
 
 void
@@ -4411,3 +4419,103 @@ debug_target::fetch_x86_xsave_layout ()
 	      target_debug_print_x86_xsave_layout (result).c_str ());
   return result;
 }
+
+bool
+target_ops::supports_displaced_step (thread_info *arg0)
+{
+  return this->beneath ()->supports_displaced_step (arg0);
+}
+
+bool
+dummy_target::supports_displaced_step (thread_info *arg0)
+{
+  return default_supports_displaced_step (this, arg0);
+}
+
+bool
+debug_target::supports_displaced_step (thread_info *arg0)
+{
+  target_debug_printf_nofunc ("-> %s->supports_displaced_step (...)", this->beneath ()->shortname ());
+  bool result
+    = this->beneath ()->supports_displaced_step (arg0);
+  target_debug_printf_nofunc ("<- %s->supports_displaced_step (%s) = %s",
+	      this->beneath ()->shortname (),
+	      target_debug_print_thread_info_p (arg0).c_str (),
+	      target_debug_print_bool (result).c_str ());
+  return result;
+}
+
+displaced_step_prepare_status
+target_ops::displaced_step_prepare (thread_info *arg0, CORE_ADDR &arg1)
+{
+  return this->beneath ()->displaced_step_prepare (arg0, arg1);
+}
+
+displaced_step_prepare_status
+dummy_target::displaced_step_prepare (thread_info *arg0, CORE_ADDR &arg1)
+{
+  return default_displaced_step_prepare (this, arg0, arg1);
+}
+
+displaced_step_prepare_status
+debug_target::displaced_step_prepare (thread_info *arg0, CORE_ADDR &arg1)
+{
+  target_debug_printf_nofunc ("-> %s->displaced_step_prepare (...)", this->beneath ()->shortname ());
+  displaced_step_prepare_status result
+    = this->beneath ()->displaced_step_prepare (arg0, arg1);
+  target_debug_printf_nofunc ("<- %s->displaced_step_prepare (%s, %s) = %s",
+	      this->beneath ()->shortname (),
+	      target_debug_print_thread_info_p (arg0).c_str (),
+	      target_debug_print_CORE_ADDR_r (arg1).c_str (),
+	      target_debug_print_displaced_step_prepare_status (result).c_str ());
+  return result;
+}
+
+displaced_step_finish_status
+target_ops::displaced_step_finish (thread_info *arg0, const target_waitstatus &arg1)
+{
+  return this->beneath ()->displaced_step_finish (arg0, arg1);
+}
+
+displaced_step_finish_status
+dummy_target::displaced_step_finish (thread_info *arg0, const target_waitstatus &arg1)
+{
+  return default_displaced_step_finish (this, arg0, arg1);
+}
+
+displaced_step_finish_status
+debug_target::displaced_step_finish (thread_info *arg0, const target_waitstatus &arg1)
+{
+  target_debug_printf_nofunc ("-> %s->displaced_step_finish (...)", this->beneath ()->shortname ());
+  displaced_step_finish_status result
+    = this->beneath ()->displaced_step_finish (arg0, arg1);
+  target_debug_printf_nofunc ("<- %s->displaced_step_finish (%s, %s) = %s",
+	      this->beneath ()->shortname (),
+	      target_debug_print_thread_info_p (arg0).c_str (),
+	      target_debug_print_const_target_waitstatus_r (arg1).c_str (),
+	      target_debug_print_displaced_step_finish_status (result).c_str ());
+  return result;
+}
+
+void
+target_ops::displaced_step_restore_all_in_ptid (inferior *arg0, ptid_t arg1)
+{
+  this->beneath ()->displaced_step_restore_all_in_ptid (arg0, arg1);
+}
+
+void
+dummy_target::displaced_step_restore_all_in_ptid (inferior *arg0, ptid_t arg1)
+{
+  default_displaced_step_restore_all_in_ptid (this, arg0, arg1);
+}
+
+void
+debug_target::displaced_step_restore_all_in_ptid (inferior *arg0, ptid_t arg1)
+{
+  target_debug_printf_nofunc ("-> %s->displaced_step_restore_all_in_ptid (...)", this->beneath ()->shortname ());
+  this->beneath ()->displaced_step_restore_all_in_ptid (arg0, arg1);
+  target_debug_printf_nofunc ("<- %s->displaced_step_restore_all_in_ptid (%s, %s)",
+	      this->beneath ()->shortname (),
+	      target_debug_print_inferior_p (arg0).c_str (),
+	      target_debug_print_ptid_t (arg1).c_str ());
+}
diff --git a/gdb/target.h b/gdb/target.h
index 5b45f152b51f..0729e32b1495 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1376,6 +1376,25 @@ struct target_ops
     /* Return the x86 XSAVE extended state area layout.  */
     virtual x86_xsave_layout fetch_x86_xsave_layout ()
       TARGET_DEFAULT_RETURN (x86_xsave_layout ());
+
+    /* Return true if the target supports displaced stepping for THREAD.  */
+    virtual bool supports_displaced_step (thread_info *thread)
+      TARGET_DEFAULT_FUNC (default_supports_displaced_step);
+
+    /* See documentation of gdbarch_displaced_step_prepare.  */
+    virtual displaced_step_prepare_status displaced_step_prepare (thread_info *thread,
+								  CORE_ADDR &displaced_pc)
+      TARGET_DEFAULT_FUNC (default_displaced_step_prepare);
+
+    /* See documentation of gdbarch_displaced_step_finish.  */
+    virtual displaced_step_finish_status displaced_step_finish
+      (thread_info *thread, const target_waitstatus &status)
+      TARGET_DEFAULT_FUNC (default_displaced_step_finish);
+
+    /* See documentation of gdbarch_displaced_step_restore_all_in_ptid.  */
+    virtual void displaced_step_restore_all_in_ptid (inferior *parent_inf,
+						     ptid_t child_ptid)
+      TARGET_DEFAULT_FUNC (default_displaced_step_restore_all_in_ptid);
   };
 
 /* Deleter for std::unique_ptr.  See comments in
-- 
2.47.1

[PATCH 3/3] gdb/amd-dbgapi: add displaced stepping support

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Implement the target_ops displaced stepping methods to add displaced
stepping support when debugging AMD GPU programs.  The knowledge of how
to prepare and finish displaced steps is provided by the amd-dbgapi
library, so the code here is relatively straightforward.  No need to
parse instructions or handle fixups, that is done by the lib  We just
need to remember, for each thread doing a displaced step, the displaced
stepping id given by the library.

Add a test to exercise the new functionality.  The compiler generates
DWARF that GDB doesn't understand yet [1], so trying to step over a
breakpoint with DWARF present gives:

    (gdb) si
    Unhandled dwarf expression opcode 0xe9

The test purposefully builds the binary without DWARF info to circumvent
this.

[1] https://llvm.org/docs/AMDGPUDwarfExtensionsForHeterogeneousDebugging.html

Change-Id: I53f459221a42d4b02a6041eadb8cf554500e2162
---
 gdb/amd-dbgapi-target.c                       | 138 ++++++++++++++++++
 gdb/testsuite/gdb.rocm/displaced-stepping.exp |  54 +++++++
 2 files changed, 192 insertions(+)
 create mode 100644 gdb/testsuite/gdb.rocm/displaced-stepping.exp

diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
index 153a35f788ed..996e16abf591 100644
--- a/gdb/amd-dbgapi-target.c
+++ b/gdb/amd-dbgapi-target.c
@@ -24,6 +24,7 @@
 #include "cli/cli-cmds.h"
 #include "cli/cli-decode.h"
 #include "cli/cli-style.h"
+#include "gdbcore.h"
 #include "gdbsupport/unordered_map.h"
 #include "inf-loop.h"
 #include "inferior.h"
@@ -215,6 +216,12 @@ struct amd_dbgapi_inferior_info
   /* List of pending events the amd-dbgapi target retrieved from the dbgapi.  */
   std::list<std::pair<ptid_t, target_waitstatus>> wave_events;
 
+  /* Map of threads with ongoing displaced steps to corresponding amd-dbgapi
+     displaced stepping handles.  */
+  gdb::unordered_map<thread_info *,
+		     decltype (amd_dbgapi_displaced_stepping_id_t::handle)>
+    stepping_id_map;
+
   /* Map of wave ID to wave_info.  We cache wave_info objects because
      we need to access the info after the wave is gone, in the thread
      exit nofication.  E.g.:
@@ -291,6 +298,21 @@ struct amd_dbgapi_target final : public target_ops
   bool stopped_by_sw_breakpoint () override;
   bool stopped_by_hw_breakpoint () override;
 
+  bool supports_displaced_step (thread_info *thread) override
+  {
+    /* Handle displaced stepping for GPU threads only.  */
+    if (!ptid_is_gpu (thread->ptid))
+      return beneath ()->supports_displaced_step (thread);
+
+    return true;
+  }
+
+  displaced_step_prepare_status displaced_step_prepare
+    (thread_info *thread, CORE_ADDR &displaced_pc) override;
+
+  displaced_step_finish_status displaced_step_finish
+    (thread_info *thread, const target_waitstatus &status) override;
+
 private:
   /* True if we must report thread events.  */
   bool m_report_thread_events = false;
@@ -1897,6 +1919,122 @@ amd_dbgapi_target::update_thread_list ()
   this->beneath ()->update_thread_list ();
 }
 
+displaced_step_prepare_status
+amd_dbgapi_target::displaced_step_prepare (thread_info *thread,
+					   CORE_ADDR &displaced_pc)
+{
+  if (!ptid_is_gpu (thread->ptid))
+    return beneath ()->displaced_step_prepare (thread, displaced_pc);
+
+  gdb_assert (!thread->displaced_step_state.in_progress ());
+
+  /* Read the bytes that were overwritten by the breakpoint instruction being
+     stepped over.  */
+  CORE_ADDR original_pc = regcache_read_pc (get_thread_regcache (thread));
+  gdbarch *arch = get_thread_regcache (thread)->arch ();
+  size_t size = get_amdgpu_gdbarch_tdep (arch)->breakpoint_instruction_size;
+  gdb::byte_vector overwritten_bytes (size);
+
+  read_memory (original_pc, overwritten_bytes.data (), size);
+
+  /* Ask dbgapi to start the displaced step.  */
+  amd_dbgapi_wave_id_t wave_id = get_amd_dbgapi_wave_id (thread->ptid);
+  amd_dbgapi_displaced_stepping_id_t stepping_id;
+  amd_dbgapi_status_t status
+    = amd_dbgapi_displaced_stepping_start (wave_id, overwritten_bytes.data (),
+					   &stepping_id);
+
+  switch (status)
+   {
+    case AMD_DBGAPI_STATUS_SUCCESS:
+      break;
+
+    case AMD_DBGAPI_STATUS_ERROR_DISPLACED_STEPPING_BUFFER_NOT_AVAILABLE:
+      return DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE;
+
+    case AMD_DBGAPI_STATUS_ERROR_ILLEGAL_INSTRUCTION:
+      return DISPLACED_STEP_PREPARE_STATUS_CANT;
+
+    default:
+      error (_("amd_dbgapi_displaced_stepping_start failed (%s)"),
+	   get_status_string (status));
+    }
+
+  /* Save the displaced stepping id in the per-inferior info.  */
+  amd_dbgapi_inferior_info *info = get_amd_dbgapi_inferior_info (thread->inf);
+
+  bool inserted
+    = info->stepping_id_map.emplace (thread, stepping_id.handle).second;
+  gdb_assert (inserted);
+
+  /* Get the new (displaced) PC.  */
+  status = amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_PC,
+				     sizeof (displaced_pc), &displaced_pc);
+  if (status != AMD_DBGAPI_STATUS_SUCCESS)
+    {
+      amd_dbgapi_displaced_stepping_complete (wave_id, stepping_id);
+      error (_("amd_dbgapi_wave_get_info failed (%s), could not get the "
+	       "thread's displaced PC."),
+	     get_status_string (status));
+    }
+
+  displaced_debug_printf ("selected buffer at %#lx", displaced_pc);
+
+  /* We may have written some registers, so flush the register cache.  */
+  registers_changed_thread (thread);
+
+  return DISPLACED_STEP_PREPARE_STATUS_OK;
+}
+
+displaced_step_finish_status
+amd_dbgapi_target::displaced_step_finish (thread_info *thread,
+					  const target_waitstatus &ws)
+{
+  if (!ptid_is_gpu (thread->ptid))
+    return beneath ()->displaced_step_finish (thread, ws);
+
+  gdb_assert (thread->displaced_step_state.in_progress ());
+
+  /* Find the displaced stepping id for this thread.  */
+  amd_dbgapi_inferior_info *info = get_amd_dbgapi_inferior_info (thread->inf);
+  auto entry = info->stepping_id_map.extract (thread);
+
+  gdb_assert (entry.has_value ());
+  amd_dbgapi_displaced_stepping_id_t stepping_id {entry->second};
+
+  /* If the thread exited while stepping, we are done.  The code above
+     cleared our associated resources.  We don't want to call dbgapi
+     below: since the thread is gone, we wouldn't be able to find the
+     necessary wave ID.  dbgapi already took care of releasing its
+     displaced-stepping-related resources when it deleted the
+     wave.  */
+  if (ws.kind () == TARGET_WAITKIND_THREAD_EXITED)
+    return DISPLACED_STEP_FINISH_STATUS_OK;
+
+  amd_dbgapi_wave_id_t wave_id = get_amd_dbgapi_wave_id (thread->ptid);
+  amd_dbgapi_wave_stop_reasons_t stop_reason;
+  amd_dbgapi_status_t status
+    = amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_STOP_REASON,
+				sizeof (stop_reason), &stop_reason);
+
+  if (status != AMD_DBGAPI_STATUS_SUCCESS)
+    error (_("wave_get_info for wave_%ld failed (%s)"), wave_id.handle,
+	   get_status_string (status));
+
+  status = amd_dbgapi_displaced_stepping_complete (wave_id, stepping_id);
+
+  if (status != AMD_DBGAPI_STATUS_SUCCESS)
+    error (_("amd_dbgapi_displaced_stepping_complete failed (%s)"),
+	   get_status_string (status));
+
+  /* We may have written some registers, so flush the register cache.  */
+  registers_changed_thread (thread);
+
+  return (stop_reason & AMD_DBGAPI_WAVE_STOP_REASON_SINGLE_STEP) != 0
+	   ? DISPLACED_STEP_FINISH_STATUS_OK
+	   : DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED;
+}
+
 /* inferior_created observer.  */
 
 static void
diff --git a/gdb/testsuite/gdb.rocm/displaced-stepping.exp b/gdb/testsuite/gdb.rocm/displaced-stepping.exp
new file mode 100644
index 000000000000..6b26e1adfd7a
--- /dev/null
+++ b/gdb/testsuite/gdb.rocm/displaced-stepping.exp
@@ -0,0 +1,54 @@
+# Copyright 2024 Free Software Foundation, Inc.
+
+# 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/>.
+
+# Test displaced stepping on AMD GPUs.
+
+load_lib rocm.exp
+
+standard_testfile simple.cpp
+
+require allow_hipcc_tests
+
+# Since GDB doesn't yet understand DWARF expressions generated by the HIP
+# compiler, purposefully generate the binary without debug info.
+if {[build_executable "failed to prepare" $testfile $srcfile {hip}]} {
+    return
+}
+
+proc do_test {} {
+    clean_restart $::binfile
+
+    with_rocm_gpu_lock {
+	if ![runto_main] {
+	    return
+	}
+
+	gdb_test "with breakpoint pending on -- break do_an_addition" \
+	    "Breakpoint $::decimal \\(do_an_addition\\) pending."
+
+	gdb_test "continue" \
+	    "Thread $::decimal hit Breakpoint $::decimal, $::hex in do_an_addition.*"
+
+	gdb_test_no_output "set debug displaced on"
+
+	gdb_test "stepi" "displaced_step_prepare_throw: prepared successfully.*$::hex in do_an_addition.*"
+
+	gdb_test "continue" \
+	    "Inferior 1 .* exited normally.*" \
+	    "continue to end"
+    }
+}
+
+do_test
-- 
2.47.1

Re: [PATCH 3/3] gdb/amd-dbgapi: add displaced stepping support

[Lancelot SIX]

Hi Simon,

I have a couple of minor remarks below.

On 14/01/2025 19:16, simon.marchi@polymtl.ca wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> Implement the target_ops displaced stepping methods to add displaced
> stepping support when debugging AMD GPU programs.  The knowledge of how
> to prepare and finish displaced steps is provided by the amd-dbgapi
> library, so the code here is relatively straightforward.  No need to
> parse instructions or handle fixups, that is done by the lib  We just
> need to remember, for each thread doing a displaced step, the displaced
> stepping id given by the library.
> 
> Add a test to exercise the new functionality.  The compiler generates
> DWARF that GDB doesn't understand yet [1], so trying to step over a
> breakpoint with DWARF present gives:
> 
>      (gdb) si
>      Unhandled dwarf expression opcode 0xe9
> 
> The test purposefully builds the binary without DWARF info to circumvent
> this.
> 
> [1] https://llvm.org/docs/AMDGPUDwarfExtensionsForHeterogeneousDebugging.html
> 
> Change-Id: I53f459221a42d4b02a6041eadb8cf554500e2162
> ---
>   gdb/amd-dbgapi-target.c                       | 138 ++++++++++++++++++
>   gdb/testsuite/gdb.rocm/displaced-stepping.exp |  54 +++++++
>   2 files changed, 192 insertions(+)
>   create mode 100644 gdb/testsuite/gdb.rocm/displaced-stepping.exp
> 
> diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
> index 153a35f788ed..996e16abf591 100644
> --- a/gdb/amd-dbgapi-target.c
> +++ b/gdb/amd-dbgapi-target.c
> @@ -24,6 +24,7 @@
>   #include "cli/cli-cmds.h"
>   #include "cli/cli-decode.h"
>   #include "cli/cli-style.h"
> +#include "gdbcore.h"
>   #include "gdbsupport/unordered_map.h"
>   #include "inf-loop.h"
>   #include "inferior.h"
> @@ -215,6 +216,12 @@ struct amd_dbgapi_inferior_info
>     /* List of pending events the amd-dbgapi target retrieved from the dbgapi.  */
>     std::list<std::pair<ptid_t, target_waitstatus>> wave_events;
> 
> +  /* Map of threads with ongoing displaced steps to corresponding amd-dbgapi
> +     displaced stepping handles.  */
> +  gdb::unordered_map<thread_info *,
> +                    decltype (amd_dbgapi_displaced_stepping_id_t::handle)>
> +    stepping_id_map;
> +
>     /* Map of wave ID to wave_info.  We cache wave_info objects because
>        we need to access the info after the wave is gone, in the thread
>        exit nofication.  E.g.:
> @@ -291,6 +298,21 @@ struct amd_dbgapi_target final : public target_ops
>     bool stopped_by_sw_breakpoint () override;
>     bool stopped_by_hw_breakpoint () override;
> 
> +  bool supports_displaced_step (thread_info *thread) override
> +  {
> +    /* Handle displaced stepping for GPU threads only.  */
> +    if (!ptid_is_gpu (thread->ptid))
> +      return beneath ()->supports_displaced_step (thread);
> +
> +    return true;
> +  }
> +
> +  displaced_step_prepare_status displaced_step_prepare
> +    (thread_info *thread, CORE_ADDR &displaced_pc) override;
> +
> +  displaced_step_finish_status displaced_step_finish
> +    (thread_info *thread, const target_waitstatus &status) override;
> +
>   private:
>     /* True if we must report thread events.  */
>     bool m_report_thread_events = false;
> @@ -1897,6 +1919,122 @@ amd_dbgapi_target::update_thread_list ()
>     this->beneath ()->update_thread_list ();
>   }
> 
> +displaced_step_prepare_status
> +amd_dbgapi_target::displaced_step_prepare (thread_info *thread,
> +                                          CORE_ADDR &displaced_pc)
> +{
> +  if (!ptid_is_gpu (thread->ptid))
> +    return beneath ()->displaced_step_prepare (thread, displaced_pc);
> +
> +  gdb_assert (!thread->displaced_step_state.in_progress ());
> +
> +  /* Read the bytes that were overwritten by the breakpoint instruction being
> +     stepped over.  */
> +  CORE_ADDR original_pc = regcache_read_pc (get_thread_regcache (thread));
> +  gdbarch *arch = get_thread_regcache (thread)->arch ();
> +  size_t size = get_amdgpu_gdbarch_tdep (arch)->breakpoint_instruction_size;
> +  gdb::byte_vector overwritten_bytes (size);
> +
> +  read_memory (original_pc, overwritten_bytes.data (), size);
> +
> +  /* Ask dbgapi to start the displaced step.  */
> +  amd_dbgapi_wave_id_t wave_id = get_amd_dbgapi_wave_id (thread->ptid);
> +  amd_dbgapi_displaced_stepping_id_t stepping_id;
> +  amd_dbgapi_status_t status
> +    = amd_dbgapi_displaced_stepping_start (wave_id, overwritten_bytes.data (),
> +                                          &stepping_id);
> +
> +  switch (status)
> +   {
> +    case AMD_DBGAPI_STATUS_SUCCESS:
> +      break;
> +
> +    case AMD_DBGAPI_STATUS_ERROR_DISPLACED_STEPPING_BUFFER_NOT_AVAILABLE:
> +      return DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE;
> +
> +    case AMD_DBGAPI_STATUS_ERROR_ILLEGAL_INSTRUCTION:
> +      return DISPLACED_STEP_PREPARE_STATUS_CANT;
> +
> +    default:
> +      error (_("amd_dbgapi_displaced_stepping_start failed (%s)"),
> +          get_status_string (status));
> +    }
> +
> +  /* Save the displaced stepping id in the per-inferior info.  */
> +  amd_dbgapi_inferior_info *info = get_amd_dbgapi_inferior_info (thread->inf);
> +
> +  bool inserted
> +    = info->stepping_id_map.emplace (thread, stepping_id.handle).second;
> +  gdb_assert (inserted);
> +
> +  /* Get the new (displaced) PC.  */
> +  status = amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_PC,
> +                                    sizeof (displaced_pc), &displaced_pc);
> +  if (status != AMD_DBGAPI_STATUS_SUCCESS)
> +    {
> +      amd_dbgapi_displaced_stepping_complete (wave_id, stepping_id);
> +      error (_("amd_dbgapi_wave_get_info failed (%s), could not get the "
> +              "thread's displaced PC."),
> +            get_status_string (status));
> +    }
> +
> +  displaced_debug_printf ("selected buffer at %#lx", displaced_pc);
> +
> +  /* We may have written some registers, so flush the register cache.  */
> +  registers_changed_thread (thread);
> +
> +  return DISPLACED_STEP_PREPARE_STATUS_OK;
> +}
> +
> +displaced_step_finish_status
> +amd_dbgapi_target::displaced_step_finish (thread_info *thread,
> +                                         const target_waitstatus &ws)
> +{
> +  if (!ptid_is_gpu (thread->ptid))
> +    return beneath ()->displaced_step_finish (thread, ws);
> +
> +  gdb_assert (thread->displaced_step_state.in_progress ());
> +
> +  /* Find the displaced stepping id for this thread.  */
> +  amd_dbgapi_inferior_info *info = get_amd_dbgapi_inferior_info (thread->inf);
> +  auto entry = info->stepping_id_map.extract (thread);
> +
> +  gdb_assert (entry.has_value ());
> +  amd_dbgapi_displaced_stepping_id_t stepping_id {entry->second};
> +
> +  /* If the thread exited while stepping, we are done.  The code above
> +     cleared our associated resources.  We don't want to call dbgapi
> +     below: since the thread is gone, we wouldn't be able to find the
> +     necessary wave ID.  dbgapi already took care of releasing its
> +     displaced-stepping-related resources when it deleted the
> +     wave.  */
> +  if (ws.kind () == TARGET_WAITKIND_THREAD_EXITED)
> +    return DISPLACED_STEP_FINISH_STATUS_OK;
> +
> +  amd_dbgapi_wave_id_t wave_id = get_amd_dbgapi_wave_id (thread->ptid);
> +  amd_dbgapi_wave_stop_reasons_t stop_reason;
> +  amd_dbgapi_status_t status
> +    = amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_STOP_REASON,
> +                               sizeof (stop_reason), &stop_reason);
> +
> +  if (status != AMD_DBGAPI_STATUS_SUCCESS)
> +    error (_("wave_get_info for wave_%ld failed (%s)"), wave_id.handle,
> +          get_status_string (status));
> +
> +  status = amd_dbgapi_displaced_stepping_complete (wave_id, stepping_id);
> +
> +  if (status != AMD_DBGAPI_STATUS_SUCCESS)
> +    error (_("amd_dbgapi_displaced_stepping_complete failed (%s)"),
> +          get_status_string (status));
> +
> +  /* We may have written some registers, so flush the register cache.  */
> +  registers_changed_thread (thread);
> +
> +  return (stop_reason & AMD_DBGAPI_WAVE_STOP_REASON_SINGLE_STEP) != 0
> +          ? DISPLACED_STEP_FINISH_STATUS_OK
> +          : DISPLACED_STEP_FINISH_STATUS_NOT_EXECUTED;
> +}
> +
>   /* inferior_created observer.  */
> 
>   static void
> diff --git a/gdb/testsuite/gdb.rocm/displaced-stepping.exp b/gdb/testsuite/gdb.rocm/displaced-stepping.exp
> new file mode 100644
> index 000000000000..6b26e1adfd7a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.rocm/displaced-stepping.exp
> @@ -0,0 +1,54 @@
> +# Copyright 2024 Free Software Foundation, Inc.

Should probably 2025.

> +
> +# 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/>.
> +
> +# Test displaced stepping on AMD GPUs.
> +
> +load_lib rocm.exp
> +
> +standard_testfile simple.cpp

I though the usage in GDB testsuite was to not share the .cpp across 
multiple testcases.  Should there be a displaced-stepping.cpp?

> +
> +require allow_hipcc_tests
> +
> +# Since GDB doesn't yet understand DWARF expressions generated by the HIP
> +# compiler, purposefully generate the binary without debug info.
> +if {[build_executable "failed to prepare" $testfile $srcfile {hip}]} {
> +    return
> +}
> +
> +proc do_test {} {
> +    clean_restart $::binfile
> +
> +    with_rocm_gpu_lock {
> +       if ![runto_main] {
> +           return
> +       }
> +
> +       gdb_test "with breakpoint pending on -- break do_an_addition" \
> +           "Breakpoint $::decimal \\(do_an_addition\\) pending."
> +
> +       gdb_test "continue" \
> +           "Thread $::decimal hit Breakpoint $::decimal, $::hex in do_an_addition.*"
> +
> +       gdb_test_no_output "set debug displaced on"
> +
> +       gdb_test "stepi" "displaced_step_prepare_throw: prepared successfully.*$::hex in do_an_addition.*"

Just to reduce the noise in the gdb.log file when running the continue, 
this could be

gdb_test "with debug displaced on -- stepi" ...

> +
> +       gdb_test "continue" \
> +           "Inferior 1 .* exited normally.*" \
> +           "continue to end"
> +    }
> +}
> +
> +do_test
> --
> 2.47.1
> 

With those minor comments addressed, and given that it only touches 
amdgpu related code,
Approved-By: Lancelot Six <lancelot.six@amd.com> (amdgpu)

Re: [PATCH 3/3] gdb/amd-dbgapi: add displaced stepping support

[Simon Marchi]

>> diff --git a/gdb/testsuite/gdb.rocm/displaced-stepping.exp b/gdb/testsuite/gdb.rocm/displaced-stepping.exp
>> new file mode 100644
>> index 000000000000..6b26e1adfd7a
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.rocm/displaced-stepping.exp
>> @@ -0,0 +1,54 @@
>> +# Copyright 2024 Free Software Foundation, Inc.
> 
> Should probably 2025.

Fixed.

>> +
>> +# 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/>.
>> +
>> +# Test displaced stepping on AMD GPUs.
>> +
>> +load_lib rocm.exp
>> +
>> +standard_testfile simple.cpp
> 
> I though the usage in GDB testsuite was to not share the .cpp across multiple testcases.  Should there be a displaced-stepping.cpp?

For "hello world" style programs (which this one is) I don't really
mind.  But I will add displaced-stepping.cpp.

> 
>> +
>> +require allow_hipcc_tests
>> +
>> +# Since GDB doesn't yet understand DWARF expressions generated by the HIP
>> +# compiler, purposefully generate the binary without debug info.
>> +if {[build_executable "failed to prepare" $testfile $srcfile {hip}]} {
>> +    return
>> +}
>> +
>> +proc do_test {} {
>> +    clean_restart $::binfile
>> +
>> +    with_rocm_gpu_lock {
>> +       if ![runto_main] {
>> +           return
>> +       }
>> +
>> +       gdb_test "with breakpoint pending on -- break do_an_addition" \
>> +           "Breakpoint $::decimal \\(do_an_addition\\) pending."
>> +
>> +       gdb_test "continue" \
>> +           "Thread $::decimal hit Breakpoint $::decimal, $::hex in do_an_addition.*"
>> +
>> +       gdb_test_no_output "set debug displaced on"
>> +
>> +       gdb_test "stepi" "displaced_step_prepare_throw: prepared successfully.*$::hex in do_an_addition.*"
> 
> Just to reduce the noise in the gdb.log file when running the continue, this could be
> 
> gdb_test "with debug displaced on -- stepi" ...

Good idea, I never remember to use "with".  Done.

> 
>> +
>> +       gdb_test "continue" \
>> +           "Inferior 1 .* exited normally.*" \
>> +           "continue to end"
>> +    }
>> +}
>> +
>> +do_test
>> -- 
>> 2.47.1
>>
> 
> With those minor comments addressed, and given that it only touches amdgpu related code,
> Approved-By: Lancelot Six <lancelot.six@amd.com> (amdgpu)

Thanks, added.

Simon

Re: [PATCH 3/3] gdb/amd-dbgapi: add displaced stepping support

[Simon Marchi]

On 1/15/25 1:45 PM, Simon Marchi wrote:
>>> diff --git a/gdb/testsuite/gdb.rocm/displaced-stepping.exp b/gdb/testsuite/gdb.rocm/displaced-stepping.exp
>>> new file mode 100644
>>> index 000000000000..6b26e1adfd7a
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.rocm/displaced-stepping.exp
>>> @@ -0,0 +1,54 @@
>>> +# Copyright 2024 Free Software Foundation, Inc.
>>
>> Should probably 2025.
> 
> Fixed.
> 
>>> +
>>> +# 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/>.
>>> +
>>> +# Test displaced stepping on AMD GPUs.
>>> +
>>> +load_lib rocm.exp
>>> +
>>> +standard_testfile simple.cpp
>>
>> I though the usage in GDB testsuite was to not share the .cpp across multiple testcases.  Should there be a displaced-stepping.cpp?
> 
> For "hello world" style programs (which this one is) I don't really
> mind.  But I will add displaced-stepping.cpp.
> 
>>
>>> +
>>> +require allow_hipcc_tests
>>> +
>>> +# Since GDB doesn't yet understand DWARF expressions generated by the HIP
>>> +# compiler, purposefully generate the binary without debug info.
>>> +if {[build_executable "failed to prepare" $testfile $srcfile {hip}]} {
>>> +    return
>>> +}
>>> +
>>> +proc do_test {} {
>>> +    clean_restart $::binfile
>>> +
>>> +    with_rocm_gpu_lock {
>>> +       if ![runto_main] {
>>> +           return
>>> +       }
>>> +
>>> +       gdb_test "with breakpoint pending on -- break do_an_addition" \
>>> +           "Breakpoint $::decimal \\(do_an_addition\\) pending."
>>> +
>>> +       gdb_test "continue" \
>>> +           "Thread $::decimal hit Breakpoint $::decimal, $::hex in do_an_addition.*"
>>> +
>>> +       gdb_test_no_output "set debug displaced on"
>>> +
>>> +       gdb_test "stepi" "displaced_step_prepare_throw: prepared successfully.*$::hex in do_an_addition.*"
>>
>> Just to reduce the noise in the gdb.log file when running the continue, this could be
>>
>> gdb_test "with debug displaced on -- stepi" ...
> 
> Good idea, I never remember to use "with".  Done.
> 
>>
>>> +
>>> +       gdb_test "continue" \
>>> +           "Inferior 1 .* exited normally.*" \
>>> +           "continue to end"
>>> +    }
>>> +}
>>> +
>>> +do_test
>>> -- 
>>> 2.47.1
>>>
>>
>> With those minor comments addressed, and given that it only touches amdgpu related code,
>> Approved-By: Lancelot Six <lancelot.six@amd.com> (amdgpu)
> 
> Thanks, added.
> 
> Simon

I pushed the series.

Simon