Re: [PATCH 6/8] Fix handling of vanishing threads that were stepping/stopping

[Lancelot SIX <lancelot.six@amd.com>]

Hi Pedro,

I have a minor comment below.

On Thu, Dec 14, 2023 at 08:22:36PM +0000, Pedro Alves wrote:
> Downstream, AMD is carrying a testcase
> (gdb.rocm/continue-over-kernel-exit.exp) that exposes a couple issues
> with the amd-dbgapi target's handling of exited threads.  The test
> can't be added upstream yet, unfortunately, due to dependency on DWARF
> extensions that can't be upstreamed yet.  However, it can be found on
> the mailing list on the same series as this patch.
> 
> The test spawns a kernel with a number of waves.  The waves do nothing
> but exit.  There is a breakpoint on the s_endpgm instruction.  Once
> that breakpoint is hit, the test issues a "continue" command.  We
> should see one breakpoint hit per wave, and then the whole program
> exiting.  We do see that, however we also see this:
> 
>  [New AMDGPU Wave ?:?:?:1 (?,?,?)/?]
>  [AMDGPU Wave ?:?:?:1 (?,?,?)/? exited]
>  *repeat for other waves*
>  ...
>  [Thread 0x7ffff626f640 (LWP 3048491) exited]
>  [Thread 0x7fffeb7ff640 (LWP 3048488) exited]
>  [Inferior 1 (process 3048475) exited normally]
> 
> That "New AMDGPU Wave" output comes from infrun.c itself adding the
> thread to the GDB thread list, because it got an event for a thread
> not on the thread list yet.  The output shows "?"s instead of proper
> coordinates, because the event was a TARGET_WAITKIND_THREAD_EXITED,
> i.e., the wave was already gone when infrun.c added the thread to the
> thread list.
> 
> That shouldn't ever happen for the amd-dbgapi target, threads should
> only ever be added by the backend.
> 
> Note "New AMDGPU Wave ?:?:?:1" is for wave 1.  What happened was that
> wave 1 terminated previously, and a previous call to
> amd_dbgapi_target::update_thread_list() noticed the wave had vanished
> and removed it from the GDB thread list.  However, because the wave
> was stepping when it terminated (due to the displaced step over the
> s_endpgm) instruction, it is guaranteed that the amd-dbgapi library
> queues a WAVE_COMMAND_TERMINATED event for the exit.
> 
> When we process that WAVE_COMMAND_TERMINATED event, in
> amd-dbgapi-target.c:process_one_event, we return it to the core as a
> TARGET_WAITKIND_THREAD_EXITED event:
> 
>  static void
>  process_one_event (amd_dbgapi_event_id_t event_id,
> 		    amd_dbgapi_event_kind_t event_kind)
>  {
>  ...
> 	 if (status == AMD_DBGAPI_STATUS_ERROR_INVALID_WAVE_ID
> 	     && event_kind == AMD_DBGAPI_EVENT_KIND_WAVE_COMMAND_TERMINATED)
> 	   ws.set_thread_exited (0);
>  ...
>  }
> 
> Recall the wave is already gone from the GDB thread list.  So when GDB
> sees that TARGET_WAITKIND_THREAD_EXITED event for a thread it doesn't
> know about, it adds the thread to the thread list, resulting in that:
> 
>  [New AMDGPU Wave ?:?:?:1 (?,?,?)/?]
> 
> and then, because it was a TARGET_WAITKIND_THREAD_EXITED event, GDB
> marks the thread exited right afterwards:
> 
>  [AMDGPU Wave ?:?:?:1 (?,?,?)/? exited]
> 
> The fix is to make amd_dbgapi_target::update_thread_list() _not_
> delete vanishing waves iff they were stepping or in progress of being
> stopped.  These two cases are the ones dbgapi guarantees will result
> in a WAVE_COMMAND_TERMINATED event if the wave terminates:
> 
>   /**
>    * A command for a wave was not able to complete because the wave has
>    * terminated.
>    *
>    * Commands that can result in this event are ::amd_dbgapi_wave_stop and
>    * ::amd_dbgapi_wave_resume in single step mode.  Since the wave terminated
>    * before stopping, this event will be reported instead of
>    * ::AMD_DBGAPI_EVENT_KIND_WAVE_STOP.
>    *
>    * The wave that terminated is available by the ::AMD_DBGAPI_EVENT_INFO_WAVE
>    * query.  However, the wave will be invalid since it has already terminated.
>    * It is the client's responsibility to know what command was being performed
>    * and was unable to complete due to the wave terminating.
>    */
>   AMD_DBGAPI_EVENT_KIND_WAVE_COMMAND_TERMINATED = 2,
> 
> As the comment says, it's GDB's responsability to know whether the
> wave was stepping or being stopped.  Since we now have a wave_info map
> with one entry for each wave, that seems like the place to store that
> information.  However, I still decided to put all the coordinate
> information in its own structure.  I.e., basically renamed the
> existing wave_info to wave_coordinates, and then added a new wave_info
> structure that holds the new state, plus a wave_coordinates object.
> This seemer cleaner as there are places where we only need to

s/seemer/seemed/ maybe?

> instantiate a wave_coordinates object.
> 
> There's an extra twist.  The testcase also exercises stopping at a new
> kernel right after the first kernel fully exits.  In that scenario, we
> were hitting this assertion after the first kernel fully exits and the
> hit of the breakpoint at the second kernel is handled:
> 
>  [amd-dbgapi] process_event_queue: Pulled event from dbgapi: event_id.handle = 26, event_kind = WAVE_STOP
>  [amd-dbgapi-lib] suspending queue_3, queue_2, queue_1 (refresh wave list)
>  ../../src/gdb/amd-dbgapi-target.c:1625: internal-error: amd_dbgapi_thread_deleted: Assertion `it != info->wave_info_map.end ()' failed.
>  A problem internal to GDB has been detected,
>  further debugging may prove unreliable.
> 
> This is the exact same problem as above, just a different
> manifestation.  In this scenario, we end up in update_thread_list
> successfully deleting the exited thread (because it was no longer the
> current thread) that was incorrectly added by infrun.c.  Because it
> was added by infrun.c and not by amd-dbgapi-target.c:add_gpu_thread,
> it doesn't have an entry in the wave_info map, so
> amd_dbgapi_thread_deleted trips on this assertion:
> 
>       gdb_assert (it != info->wave_info_map.end ());
> 
> here:
> 
>   ...
>   -> stop_all_threads
>    -> update_thread_list
>     -> target_update_thread_list
>      -> amd_dbgapi_target::update_thread_list
>       -> thread_db_target::update_thread_list
>        -> linux_nat_target::update_thread_list
> 	-> delete_exited_threads
> 	 -> delete_thread
> 	  -> delete_thread_1
> 	   -> gdb::observers::observable<thread_info*>::notify
> 	    -> amd_dbgapi_thread_deleted
> 	     -> internal_error_loc
> 
> The testcase thus tries both running to exit after the first kernel
> exits, and running to a breakpoint in a second kernel after the first
> kernel exits.
> 
> Change-Id: I43a66f060c35aad1fe0d9ff022ce2afd0537f028
> ---
>  gdb/amd-dbgapi-target.c | 197 ++++++++++++++++++++++++++++++----------
>  1 file changed, 149 insertions(+), 48 deletions(-)
> 
> diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
> index 86102b7fb03..06f9e8c5f9c 100644
> --- a/gdb/amd-dbgapi-target.c
> +++ b/gdb/amd-dbgapi-target.c
> @@ -109,10 +109,9 @@ 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.  */
> +/* A type holding coordinates, etc. info for a given wave.  */
>  
> -struct wave_info
> +struct wave_coordinates
>  {
>    /* The wave.  Set by the ctor.  */
>    amd_dbgapi_wave_id_t wave_id;
> @@ -125,11 +124,44 @@ struct wave_info
>    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);
> +  explicit wave_coordinates (amd_dbgapi_wave_id_t wave_id)
> +    : wave_id (wave_id)
> +  {}
>  
> -  /* Return the target ID string for the wave this wave_info is
> +  /* Return the target ID string for the wave this wave_coordinates is
>       for.  */
>    std::string to_string () const;
> +
> +  /* Pull out coordinates info from the amd-dbgapi library.  */
> +  void fetch ();
> +};
> +
> +/* A type holding info about a given wave.  */
> +
> +struct wave_info
> +{
> +  /* We cache the coordinates info because we need it after a wave
> +     exits.  The wave's ID is here.  */
> +  wave_coordinates coords;
> +
> +  /* The last resume_mode passed to amd_dbgapi_wave_resume for this
> +     wave.  We track this because we are guaranteed to see a
> +     WAVE_COMMAND_TERMINATED event if a stepping wave terminates, and
> +     we need to know to not delete such a wave until we process that
> +     event.  */
> +  amd_dbgapi_resume_mode_t last_resume_mode = AMD_DBGAPI_RESUME_MODE_NORMAL;
> +
> +  /* Whether we've called amd_dbgapi_wave_stop for this wave and are
> +     waiting for its stop event.  Similarly, we track this because
> +     we're guaranteed to get a WAVE_COMMAND_TERMINATED event if the
> +     wave terminates while being stopped.  */
> +  bool stopping = false;
> +
> +  explicit wave_info (amd_dbgapi_wave_id_t wave_id)
> +    : coords (wave_id)
> +  {
> +    coords.fetch ();
> +  }
>  };
>  
>  /* Big enough to hold the size of the largest register in bytes.  */
> @@ -275,6 +307,19 @@ static struct amd_dbgapi_target the_amd_dbgapi_target;
>  static const registry<inferior>::key<amd_dbgapi_inferior_info>
>    amd_dbgapi_inferior_data;
>  
> +/* Fetch the amd_dbgapi_inferior_info data for the given inferior.  */
> +
> +static struct amd_dbgapi_inferior_info *
> +get_amd_dbgapi_inferior_info (struct inferior *inferior)
> +{
> +  amd_dbgapi_inferior_info *info = amd_dbgapi_inferior_data.get (inferior);
> +
> +  if (info == nullptr)
> +    info = amd_dbgapi_inferior_data.emplace (inferior, inferior);
> +
> +  return info;
> +}
> +
>  /* The async event handler registered with the event loop, indicating that we
>     might have events to report to the core and that we'd like our wait method
>     to be called.
> @@ -289,7 +334,7 @@ static const registry<inferior>::key<amd_dbgapi_inferior_info>
>  static async_event_handler *amd_dbgapi_async_event_handler = nullptr;
>  
>  std::string
> -wave_info::to_string () const
> +wave_coordinates::to_string () const
>  {
>    std::string str = "AMDGPU Wave";
>  
> @@ -319,37 +364,41 @@ wave_info::to_string () const
>    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)
> +void
> +wave_coordinates::fetch ()
>  {
> -  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);
> +			    sizeof (agent_id), &agent_id);
>    amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_QUEUE,
> -			    sizeof (res.queue_id), &res.queue_id);
> +			    sizeof (queue_id), &queue_id);
>    amd_dbgapi_wave_get_info (wave_id, AMD_DBGAPI_WAVE_INFO_DISPATCH,
> -			    sizeof (res.dispatch_id), &res.dispatch_id);
> +			    sizeof (dispatch_id), &dispatch_id);
>  
>    amd_dbgapi_wave_get_info (wave_id,
>  			    AMD_DBGAPI_WAVE_INFO_WORKGROUP_COORD,
> -			    sizeof (res.group_ids), &res.group_ids);
> +			    sizeof (group_ids), &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);
> +			    sizeof (wave_in_group), &wave_in_group);
> +}
> +
> +/* Get the wave_info object for TP, from the wave_info map.  It is
> +   assumed that the wave is in the map.  */
> +
> +static wave_info &
> +get_thread_wave_info (thread_info *tp)
> +{
> +  amd_dbgapi_inferior_info *info = get_amd_dbgapi_inferior_info (tp->inf);
> +  amd_dbgapi_wave_id_t 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 ());
>  
> -  return res;
> +  return it->second;
>  }
>  
>  /* Clear our async event handler.  */
> @@ -370,19 +419,6 @@ async_event_handler_mark ()
>    mark_async_event_handler (amd_dbgapi_async_event_handler);
>  }
>  
> -/* Fetch the amd_dbgapi_inferior_info data for the given inferior.  */
> -
> -static struct amd_dbgapi_inferior_info *
> -get_amd_dbgapi_inferior_info (struct inferior *inferior)
> -{
> -  amd_dbgapi_inferior_info *info = amd_dbgapi_inferior_data.get (inferior);
> -
> -  if (info == nullptr)
> -    info = amd_dbgapi_inferior_data.emplace (inferior, inferior);
> -
> -  return info;
> -}
> -
>  /* Set forward progress requirement to REQUIRE for all processes of PROC_TARGET
>     matching PTID.  */
>  
> @@ -565,12 +601,12 @@ amd_dbgapi_target::pid_to_str (ptid_t ptid)
>  
>    auto it = info->wave_info_map.find (wave_id.handle);
>    if (it != info->wave_info_map.end ())
> -    return it->second.to_string ();
> +    return it->second.coords.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 ();
> +  return wave_coordinates (wave_id).to_string ();
>  }
>  
>  const char *
> @@ -694,16 +730,24 @@ amd_dbgapi_target::resume (ptid_t scope_ptid, int step, enum gdb_signal signo)
>  
>        amd_dbgapi_wave_id_t wave_id = get_amd_dbgapi_wave_id (thread->ptid);
>        amd_dbgapi_status_t status;
> +
> +      wave_info &wi = get_thread_wave_info (thread);
> +      amd_dbgapi_resume_mode_t &resume_mode = wi.last_resume_mode;
> +      amd_dbgapi_exceptions_t wave_exception;
>        if (thread->ptid == inferior_ptid)
> -	status = amd_dbgapi_wave_resume (wave_id,
> -					 (step
> -					  ? AMD_DBGAPI_RESUME_MODE_SINGLE_STEP
> -					  : AMD_DBGAPI_RESUME_MODE_NORMAL),
> -					 exception);
> +	{
> +	  resume_mode = (step
> +			 ? AMD_DBGAPI_RESUME_MODE_SINGLE_STEP
> +			 : AMD_DBGAPI_RESUME_MODE_NORMAL);
> +	  wave_exception = exception;
> +	}
>        else
> -	status = amd_dbgapi_wave_resume (wave_id, AMD_DBGAPI_RESUME_MODE_NORMAL,
> -					 AMD_DBGAPI_EXCEPTION_NONE);
> +	{
> +	  resume_mode = AMD_DBGAPI_RESUME_MODE_NORMAL;
> +	  wave_exception = AMD_DBGAPI_EXCEPTION_NONE;
> +	}
>  
> +      status = amd_dbgapi_wave_resume (wave_id, resume_mode, wave_exception);
>        if (status != AMD_DBGAPI_STATUS_SUCCESS
>  	  /* Ignore the error that wave is no longer valid as that could
>  	     indicate that the process has exited.  GDB treats resuming a
> @@ -711,6 +755,8 @@ amd_dbgapi_target::resume (ptid_t scope_ptid, int step, enum gdb_signal signo)
>  	  && status != AMD_DBGAPI_STATUS_ERROR_INVALID_WAVE_ID)
>  	error (_("wave_resume for wave_%ld failed (%s)"), wave_id.handle,
>  	       get_status_string (status));
> +
> +      wi.stopping = false;
>      }
>  }
>  
> @@ -725,6 +771,15 @@ amd_dbgapi_target::commit_resumed ()
>    require_forward_progress (minus_one_ptid, proc_target, true);
>  }
>  
> +/* Return a string version of RESUME_MODE, for debug log purposes.  */
> +static const char *
> +resume_mode_to_string (amd_dbgapi_resume_mode_t resume_mode)
> +{
> +  return (resume_mode == AMD_DBGAPI_RESUME_MODE_SINGLE_STEP
> +	  ? "step"
> +	  : "normal");

Pedantically, amd_dbgapi_resume_mode_t is an enum and could in theory
get more enumerators.  I don't expect any, but the following construct
would make the compiler complain if that ever happens:

   static const char *
   resume_mode_to_string (amd_dbgapi_resume_mode_t resume_mode)
   {
     switch (resume_mode)
       {
       case AMD_DBGAPI_RESUME_MODE_NORMAL:
         return "normal";
       case AMD_DBGAPI_RESUME_MODE_SINGLE_STEP:
         return "step";
       }
     gdb_assert_not_reached ("invalid amd_dbgapi_resume_mode_t");
   }

> +}
> +
>  void
>  amd_dbgapi_target::stop (ptid_t ptid)
>  {
> @@ -758,7 +813,11 @@ amd_dbgapi_target::stop (ptid_t ptid)
>  
>  	  status = amd_dbgapi_wave_stop (wave_id);
>  	  if (status == AMD_DBGAPI_STATUS_SUCCESS)
> -	    return;
> +	    {
> +	      wave_info &wi = get_thread_wave_info (thread);
> +	      wi.stopping = true;
> +	      return;
> +	    }
>  
>  	  if (status != AMD_DBGAPI_STATUS_ERROR_INVALID_WAVE_ID)
>  	    error (_("wave_stop for wave_%ld failed (%s)"), wave_id.handle,
> @@ -772,6 +831,23 @@ amd_dbgapi_target::stop (ptid_t ptid)
>  	 could have terminated since the last time the wave list was
>  	 refreshed.  */
>  
> +      wave_info &wi = get_thread_wave_info (thread);
> +      wi.stopping = true;
> +
> +      amd_dbgapi_debug_printf ("got AMD_DBGAPI_STATUS_ERROR_INVALID_WAVE_ID "
> +			       "for wave_%ld, last_resume_mode=%s, "
> +			       "report_thread_events=%d",
> +			       wave_id.handle,
> +			       resume_mode_to_string (wi.last_resume_mode),
> +			       m_report_thread_events);
> +
> +      /* If the wave was stepping when it terminated, then it is
> +	 guaranteed that we will see a WAVE_COMMAND_TERMINATED event
> +	 for it.  Don't report a thread exit event or delete the
> +	 thread yet, until we see such event.  */
> +      if (wi.last_resume_mode == AMD_DBGAPI_RESUME_MODE_SINGLE_STEP)
> +	return;
> +
>        if (m_report_thread_events)
>  	{
>  	  get_amd_dbgapi_inferior_info (thread->inf)->wave_events.emplace_back
> @@ -1018,7 +1094,7 @@ add_gpu_thread (inferior *inf, ptid_t wave_ptid)
>    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)
> +					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
> @@ -1770,7 +1846,32 @@ amd_dbgapi_target::update_thread_list ()
>  	    auto it = threads.find (tp->ptid.tid ());
>  
>  	    if (it == threads.end ())
> -	      delete_thread_silent (tp);
> +	      {
> +		auto wave_id = get_amd_dbgapi_wave_id (tp->ptid);
> +		wave_info &wi = get_thread_wave_info (tp);
> +
> +		/* Waves that were stepping or in progress of being
> +		   stopped are guaranteed to report a
> +		   WAVE_COMMAND_TERMINATED event if they terminate.
> +		   Don't delete such threads until we see the
> +		   event.  */
> +		if (wi.last_resume_mode == AMD_DBGAPI_RESUME_MODE_SINGLE_STEP
> +		    || wi.stopping)
> +		  {
> +		    amd_dbgapi_debug_printf
> +		      ("wave_%ld disappeared, keeping it"
> +		       " (last_resume_mode=%s, stopping=%d)",
> +		       wave_id.handle,
> +		       resume_mode_to_string (wi.last_resume_mode),
> +		       wi.stopping);
> +		  }
> +		else
> +		  {
> +		    amd_dbgapi_debug_printf ("wave_%ld disappeared, deleting it",
> +					     wave_id.handle);
> +		    delete_thread_silent (tp);
> +		  }
> +	      }
>  	    else
>  	      threads.erase (it);
>  	  }
> 
> -- 
> 2.43.0
> 

I happy with the patch as it is if you prefer to not change
resume_mode_to_string.  Either way

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

Best,
Lancelot.