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

[Lancelot SIX <Lancelot.Six@amd.com>]

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)