[5/8] Fix thread target ID of exited waves

  Currently, if you step over kernel exit, you see:

 stepi
 [AMDGPU Wave ?:?:?:1 (?,?,?)/? exited]
 Command aborted, thread exited.
 (gdb)

Those '?' are because the thread/wave is already gone by the time GDB
prints the "exited" notification, we can't ask dbgapi for any info
about the wave anymore.

This commit fixes it by caching the wave's coordinates as soon as GDB
sees the wave for the first time, and making
amd_dbgapi_target::pid_to_str use the cached info.

At first I thought of clearing the wave_info object from a
thread_exited observer.  However, that is too soon, resulting in this:

 (gdb) si
 [AMDGPU Wave 1:4:1:1 (0,0,0)/0 exited]
 Command aborted, thread exited.
 (gdb) thread
 [Current thread is 6 (AMDGPU Wave ?:?:?:0 (?,?,?)/?) (exited)]

We need instead to clear the wave info when the thread is ultimately
deleted, so we get:

 (gdb) si
 [AMDGPU Wave 1:4:1:1 (0,0,0)/0 exited]
 Command aborted, thread exited.
 (gdb) thread
 [Current thread is 6 (AMDGPU Wave 1:4:1:1 (0,0,0)/0) (exited)]

And for that, we need a new thread_deleted observable.

Change-Id: I6c3e22541f051e1205f75eb657b04dc15e547580
---
 gdb/amd-dbgapi-target.c | 168 +++++++++++++++++++++++++++++++---------
 gdb/observable.c        |   1 +
 gdb/observable.h        |   5 ++
 gdb/thread.c            |   2 +
 4 files changed, 138 insertions(+), 38 deletions(-)
  

Hi Pedro,

Thanks for doing this.

I have minor suggestions below, but I am happy if you prefer what you
have here.

On Thu, Dec 14, 2023 at 08:22:35PM +0000, Pedro Alves wrote:
> Currently, if you step over kernel exit, you see:
> 
>  stepi
>  [AMDGPU Wave ?:?:?:1 (?,?,?)/? exited]
>  Command aborted, thread exited.
>  (gdb)
> 
> Those '?' are because the thread/wave is already gone by the time GDB
> prints the "exited" notification, we can't ask dbgapi for any info
> about the wave anymore.
> 
> This commit fixes it by caching the wave's coordinates as soon as GDB
> sees the wave for the first time, and making
> amd_dbgapi_target::pid_to_str use the cached info.
> 
> At first I thought of clearing the wave_info object from a
> thread_exited observer.  However, that is too soon, resulting in this:
> 
>  (gdb) si
>  [AMDGPU Wave 1:4:1:1 (0,0,0)/0 exited]
>  Command aborted, thread exited.
>  (gdb) thread
>  [Current thread is 6 (AMDGPU Wave ?:?:?:0 (?,?,?)/?) (exited)]
> 
> We need instead to clear the wave info when the thread is ultimately
> deleted, so we get:
> 
>  (gdb) si
>  [AMDGPU Wave 1:4:1:1 (0,0,0)/0 exited]
>  Command aborted, thread exited.
>  (gdb) thread
>  [Current thread is 6 (AMDGPU Wave 1:4:1:1 (0,0,0)/0) (exited)]
> 
> And for that, we need a new thread_deleted observable.
> 
> Change-Id: I6c3e22541f051e1205f75eb657b04dc15e547580
> ---
>  gdb/amd-dbgapi-target.c | 168 +++++++++++++++++++++++++++++++---------
>  gdb/observable.c        |   1 +
>  gdb/observable.h        |   5 ++
>  gdb/thread.c            |   2 +
>  4 files changed, 138 insertions(+), 38 deletions(-)
> 
> diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
> index 18c0543c40e..86102b7fb03 100644
> --- a/gdb/amd-dbgapi-target.c
> +++ b/gdb/amd-dbgapi-target.c
> @@ -109,6 +109,28 @@ get_amd_dbgapi_target_inferior_created_observer_token ()
>    return amd_dbgapi_target_inferior_created_observer_token;
>  }
>  
> +/* A type holding coordinate, etc. info for a given wave.  We cache
> +   this because we need this information after a wave exits.  */
> +
> +struct wave_info
> +{
> +  /* The wave.  Set by the ctor.  */
> +  amd_dbgapi_wave_id_t wave_id;
> +
> +  /* All these fields are initialized here to a value that is printed
> +     as "?".  */
> +  amd_dbgapi_dispatch_id_t dispatch_id {};
> +  amd_dbgapi_queue_id_t queue_id {};
> +  amd_dbgapi_agent_id_t agent_id {};

We could be more explicit here and use the semantically equivalent
notation:

  amd_dbgapi_dispatch_id_t dispatch_id = AMD_DBGAPI_DISPATCH_NONE;
  amd_dbgapi_queue_id_t queue_id = AMD_DBGAPI_QUEUE_NONE;
  amd_dbgapi_agent_id_t agent_id = AMD_DBGAPI_AGENT_NONE;


> +  uint32_t group_ids[3] {UINT32_MAX, UINT32_MAX, UINT32_MAX};
> +  uint32_t wave_in_group = UINT32_MAX;
> +
> +  explicit wave_info (amd_dbgapi_wave_id_t wave_id);
> +
> +  /* Return the target ID string for the wave this wave_info is
> +     for.  */
> +  std::string to_string () const;
> +};
>  
>  /* Big enough to hold the size of the largest register in bytes.  */
>  #define AMDGPU_MAX_REGISTER_SIZE 256
> @@ -160,6 +182,16 @@ 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 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.:
> +	[AMDGPU Wave 1:4:1:1 (0,0,0)/0 exited]
> +
> +     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>
> +    wave_info_map;
>  };
>  
>  static amd_dbgapi_event_id_t process_event_queue
> @@ -256,56 +288,70 @@ static const registry<inferior>::key<amd_dbgapi_inferior_info>
>  
>  static async_event_handler *amd_dbgapi_async_event_handler = nullptr;
>  
> -/* Return the target id string for a given wave.  */
> -
> -static std::string
> -wave_target_id_string (amd_dbgapi_wave_id_t wave_id)
> +std::string
> +wave_info::to_string () const
>  {
> -  amd_dbgapi_dispatch_id_t dispatch_id;
> -  amd_dbgapi_queue_id_t queue_id;
> -  amd_dbgapi_agent_id_t agent_id;
> -  uint32_t group_ids[3], wave_in_group;
>    std::string str = "AMDGPU Wave";
>  
> -  amd_dbgapi_status_t status
> -    = amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_AGENT,
> -				sizeof (agent_id), &agent_id);
> -  str += (status == AMD_DBGAPI_STATUS_SUCCESS
> +  str += (agent_id.handle != 0

We could use `agent_id != AMD_DBGAPI_AGENT_NONE` here.

>  	  ? string_printf (" %ld", agent_id.handle)
>  	  : " ?");
>  
> -  status = amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_QUEUE,
> -				     sizeof (queue_id), &queue_id);
> -  str += (status == AMD_DBGAPI_STATUS_SUCCESS
> +  str += (queue_id.handle != 0

Similarly, `queue_id != AMD_DBGAPI_QUEUE_NONE`.

>  	  ? string_printf (":%ld", queue_id.handle)
>  	  : ":?");
>  
> -  status = amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_DISPATCH,
> -				     sizeof (dispatch_id), &dispatch_id);
> -  str += (status == AMD_DBGAPI_STATUS_SUCCESS
> +  str += (dispatch_id.handle != 0

Similarly, `dispatch_id != AMD_DBGAPI_DISPATCH_NONE`.

>  	  ? string_printf (":%ld", dispatch_id.handle)
>  	  : ":?");
>  
>    str += string_printf (":%ld", wave_id.handle);
>  
> -  status = amd_dbgapi_wave_get_info (wave_id,
> -				     AMD_DBGAPI_WAVE_INFO_WORKGROUP_COORD,
> -				     sizeof (group_ids), &group_ids);
> -  str += (status == AMD_DBGAPI_STATUS_SUCCESS
> +  str += (group_ids[0] != UINT32_MAX
>  	  ? string_printf (" (%d,%d,%d)", group_ids[0], group_ids[1],
>  			   group_ids[2])
>  	  : " (?,?,?)");
>  
> -  status = amd_dbgapi_wave_get_info
> -    (wave_id, AMD_DBGAPI_WAVE_INFO_WAVE_NUMBER_IN_WORKGROUP,
> -     sizeof (wave_in_group), &wave_in_group);
> -  str += (status == AMD_DBGAPI_STATUS_SUCCESS
> +  str += (wave_in_group != UINT32_MAX
>  	  ? string_printf ("/%d", wave_in_group)
>  	  : "/?");
>  
>    return str;
>  }
>  
> +wave_info::wave_info (amd_dbgapi_wave_id_t wave_id)
> +  : wave_id (wave_id)
> +{
> +}
> +
> +/* Read in wave_info for WAVE_ID.  */
> +
> +static wave_info
> +get_wave_info (amd_dbgapi_wave_id_t wave_id)
> +{
> +  wave_info res (wave_id);
> +
> +  /* Any field that fails to be read is left with its in-class
> +     initialized value, which is printed as "?".  */
> +
> +  amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_AGENT,
> +			    sizeof (res.agent_id), &res.agent_id);
> +  amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_QUEUE,
> +			    sizeof (res.queue_id), &res.queue_id);
> +  amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_DISPATCH,
> +			    sizeof (res.dispatch_id), &res.dispatch_id);
> +
> +  amd_dbgapi_wave_get_info (wave_id,
> +			    AMD_DBGAPI_WAVE_INFO_WORKGROUP_COORD,
> +			    sizeof (res.group_ids), &res.group_ids);
> +
> +  amd_dbgapi_wave_get_info (wave_id,
> +			    AMD_DBGAPI_WAVE_INFO_WAVE_NUMBER_IN_WORKGROUP,
> +			    sizeof (res.wave_in_group), &res.wave_in_group);
> +
> +  return res;
> +}
> +
>  /* Clear our async event handler.  */
>  
>  static void
> @@ -510,7 +556,21 @@ amd_dbgapi_target::pid_to_str (ptid_t ptid)
>    if (!ptid_is_gpu (ptid))
>      return beneath ()->pid_to_str (ptid);
>  
> -  return wave_target_id_string (get_amd_dbgapi_wave_id (ptid));
> +  process_stratum_target *proc_target = current_inferior ()->process_target ();
> +  inferior *inf = find_inferior_pid (proc_target, ptid.pid ());
> +  gdb_assert (inf != nullptr);
> +  amd_dbgapi_inferior_info *info = get_amd_dbgapi_inferior_info (inf);
> +
> +  auto wave_id = get_amd_dbgapi_wave_id (ptid);
> +
> +  auto it = info->wave_info_map.find (wave_id.handle);
> +  if (it != info->wave_info_map.end ())
> +    return it->second.to_string ();
> +
> +  /* A wave we don't know about.  Shouldn't usually happen, but
> +     asserting and bringing down the session is a bit too harsh.  Just
> +     print all unknown info as "?"s.  */
> +  return wave_info (wave_id).to_string ();
>  }
>  
>  const char *
> @@ -929,6 +989,46 @@ make_gpu_ptid (ptid_t::pid_type pid, amd_dbgapi_wave_id_t wave_id)
>   return ptid_t (pid, 1, wave_id.handle);
>  }
>  
> +/* When a thread is deleted, remove its wave_info from the inferior's
> +   wave_info map.  */
> +
> +static void
> +amd_dbgapi_thread_deleted (thread_info *tp)
> +{
> +  if (tp->inf->target_at (arch_stratum) == &the_amd_dbgapi_target
> +      && ptid_is_gpu (tp->ptid))
> +    {
> +      amd_dbgapi_inferior_info *info = amd_dbgapi_inferior_data.get (tp->inf);
> +      auto wave_id = get_amd_dbgapi_wave_id (tp->ptid);
> +      auto it = info->wave_info_map.find (wave_id.handle);
> +      gdb_assert (it != info->wave_info_map.end ());
> +      info->wave_info_map.erase (it);
> +    }
> +}
> +
> +/* Register WAVE_PTID as a new thread in INF's thread list, and record
> +   its wave_info in the inferior's wave_info map.  */
> +
> +static thread_info *
> +add_gpu_thread (inferior *inf, ptid_t wave_ptid)
> +{
> +  process_stratum_target *proc_target = inf->process_target ();
> +  amd_dbgapi_inferior_info *info = get_amd_dbgapi_inferior_info (inf);
> +
> +  auto wave_id = get_amd_dbgapi_wave_id (wave_ptid);
> +
> +  if (!info->wave_info_map.try_emplace (wave_id.handle,
> +					get_wave_info (wave_id)).second)
> +    internal_error ("wave ID %ld already in map", wave_id.handle);
> +
> +  /* Create new GPU threads silently to avoid spamming the terminal
> +     with thousands of "[New Thread ...]" messages.  */
> +  thread_info *thread = add_thread_silent (proc_target, wave_ptid);
> +  set_running (proc_target, wave_ptid, true);
> +  set_executing (proc_target, wave_ptid, true);
> +  return thread;
> +}
> +
>  /* Process an event that was just pulled out of the amd-dbgapi library.  */
>  
>  static void
> @@ -1015,13 +1115,7 @@ process_one_event (amd_dbgapi_event_id_t event_id,
>  
>  	    thread_info *thread = proc_target->find_thread (event_ptid);
>  	    if (thread == nullptr)
> -	      {
> -		/* Silently create new GPU threads to avoid spamming the
> -		   terminal with thousands of "[New Thread ...]" messages.  */
> -		thread = add_thread_silent (proc_target, event_ptid);
> -		set_running (proc_target, event_ptid, true);
> -		set_executing (proc_target, event_ptid, true);
> -	      }
> +	      thread = add_gpu_thread (inf, event_ptid);
>  
>  	    /* If the wave is stopped because of a software breakpoint, the
>  	       program counter needs to be adjusted so that it points to the
> @@ -1686,10 +1780,7 @@ amd_dbgapi_target::update_thread_list ()
>  	{
>  	  ptid_t wave_ptid
>  	    = make_gpu_ptid (inf->pid, amd_dbgapi_wave_id_t {tid});
> -
> -	  add_thread_silent (inf->process_target (), wave_ptid);
> -	  set_running (inf->process_target (), wave_ptid, true);
> -	  set_executing (inf->process_target (), wave_ptid, true);
> +	  add_gpu_thread (inf, wave_ptid);
>  	}
>      }
>  
> @@ -2115,6 +2206,7 @@ _initialize_amd_dbgapi_target ()
>    gdb::observers::inferior_forked.attach (amd_dbgapi_inferior_forked, "amd-dbgapi");
>    gdb::observers::inferior_exit.attach (amd_dbgapi_inferior_exited, "amd-dbgapi");
>    gdb::observers::inferior_pre_detach.attach (amd_dbgapi_inferior_pre_detach, "amd-dbgapi");
> +  gdb::observers::thread_deleted.attach (amd_dbgapi_thread_deleted, "amd-dbgapi");
>  
>    add_basic_prefix_cmd ("amdgpu", no_class,
>  			_("Generic command for setting amdgpu flags."),
> diff --git a/gdb/observable.c b/gdb/observable.c
> index f2e65b11604..29675f3abf3 100644
> --- a/gdb/observable.c
> +++ b/gdb/observable.c
> @@ -46,6 +46,7 @@ DEFINE_OBSERVABLE (all_objfiles_removed);
>  DEFINE_OBSERVABLE (free_objfile);
>  DEFINE_OBSERVABLE (new_thread);
>  DEFINE_OBSERVABLE (thread_exit);
> +DEFINE_OBSERVABLE (thread_deleted);
>  DEFINE_OBSERVABLE (thread_stop_requested);
>  DEFINE_OBSERVABLE (target_resumed);
>  DEFINE_OBSERVABLE (about_to_proceed);
> diff --git a/gdb/observable.h b/gdb/observable.h
> index 32ef65435cc..91a2c871524 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -126,6 +126,11 @@ extern observable<thread_info */* t */,
>  		  std::optional<ULONGEST> /* exit_code */,
>  		  bool /* silent */> thread_exit;
>  
> +/* The thread specified by T has been deleted, with delete_thread.
> +   This is called just before the thread_info object is destroyed with
> +   operator delete.  */
> +extern observable<thread_info */* t */> thread_deleted;
> +
>  /* An explicit stop request was issued to PTID.  If PTID equals
>     minus_one_ptid, the request applied to all threads.  If
>     ptid_is_pid(PTID) returns true, the request applied to all
> diff --git a/gdb/thread.c b/gdb/thread.c
> index 85bdbaa6cd8..bd3fe85f3b9 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -527,6 +527,8 @@ delete_thread_1 (thread_info *thr, std::optional<ULONGEST> exit_code,
>    auto it = thr->inf->thread_list.iterator_to (*thr);
>    thr->inf->thread_list.erase (it);
>  
> +  gdb::observers::thread_deleted.notify (thr);
> +
>    delete thr;
>  }
>  
> 
> -- 
> 2.43.0
> 

I am happy whether you do the suggested changes or not. Either way

Approved-By: Lancelot Six <lancelot.six@amd.com> (amdgpu)

Best,
Lancelot.