Re: [PATCH 3/5] gdb/amd-dbgapi: pass amd_dbgapi_inferior_info to process_event_queue

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

On Thu, Jun 05, 2025 at 04:16:26PM -0400, Simon Marchi wrote:
> A following patch will make process_event_queue access a field of
> amd_dbgapi_inferior_info.  Prepare for this by making
> process_event_queue accept an amd_dbgapi_inferior_info object, instead
> of a process id.
> 
> Change-Id: I9adc491dd1ff64ff74c40aa7662fffb11bd8332b
> ---
>  gdb/amd-dbgapi-target.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
> index 8fd8bcf5ae49..1ec20d1874ec 100644
> --- a/gdb/amd-dbgapi-target.c
> +++ b/gdb/amd-dbgapi-target.c
> @@ -234,7 +234,7 @@ struct amd_dbgapi_inferior_info
>  };
>  
>  static amd_dbgapi_event_id_t process_event_queue
> -  (amd_dbgapi_process_id_t process_id,
> +  (amd_dbgapi_inferior_info &info,
>     amd_dbgapi_event_kind_t until_event_kind = AMD_DBGAPI_EVENT_KIND_NONE);
>  
>  static const target_info amd_dbgapi_target_info = {
> @@ -564,8 +564,7 @@ amd_dbgapi_target_breakpoint::check_status (struct bpstat *bs)
>    /* If the action is AMD_DBGAPI_BREAKPOINT_ACTION_HALT, we need to wait until
>       a breakpoint resume event for this breakpoint_id is seen.  */
>    amd_dbgapi_event_id_t resume_event_id
> -    = process_event_queue (info->process_id,
> -			   AMD_DBGAPI_EVENT_KIND_BREAKPOINT_RESUME);
> +    = process_event_queue (*info, AMD_DBGAPI_EVENT_KIND_BREAKPOINT_RESUME);
>  
>    /* We should always get a breakpoint_resume event after processing all
>       events generated by reporting the breakpoint hit.  */
> @@ -1337,28 +1336,22 @@ event_kind_str (amd_dbgapi_event_kind_t kind)
>    gdb_assert_not_reached ("unhandled amd_dbgapi_event_kind_t value");
>  }
>  
> -/* Drain the dbgapi event queue of a given process_id, or of all processes if
> -   process_id is AMD_DBGAPI_PROCESS_NONE.  Stop processing the events if an
> -   event of a given kind is requested and `process_id` is not
> -   AMD_DBGAPI_PROCESS_NONE.  Wave stop events that are not returned are queued
> -   into their inferior's amd_dbgapi_inferior_info pending wave events. */
> +/* Drain the dbgapi event queue of a given process_id.  Stop processing the

It reads odd to have process_id here, as we don't pass it as an explicit
parameter anymore.  Could it say "a given inferior" instead?

> +   events if an event of a given kind is requested (not AMD_DBGAPI_EVENT_NONE).
> +   Wave stop events that are not returned are queued into their inferior's
> +   amd_dbgapi_inferior_info pending wave events. */

My first intuition when reading this patch was "hey, the INFO param is
never modified, why not use a const ref".  But eventually ,under
process_one_event, we'll grab a reference to the INFO and modify it to
stash stop events.

Should this patch go one step further, and have the INFO passed to
process_one_event (this one can ensure the event's process_id matches
the INFO's process_id)?  This would also save iterating over all
inferiors to find the one we are looking for.

Best,
Lancelot.

>  
>  static amd_dbgapi_event_id_t
> -process_event_queue (amd_dbgapi_process_id_t process_id,
> +process_event_queue (amd_dbgapi_inferior_info &info,
>  		     amd_dbgapi_event_kind_t until_event_kind)
>  {
> -  /* An event of a given type can only be requested from a single
> -     process_id.  */
> -  gdb_assert (until_event_kind == AMD_DBGAPI_EVENT_KIND_NONE
> -	      || process_id != AMD_DBGAPI_PROCESS_NONE);
> -
>    while (true)
>      {
>        amd_dbgapi_event_id_t event_id;
>        amd_dbgapi_event_kind_t event_kind;
>  
>        amd_dbgapi_status_t status
> -	= amd_dbgapi_process_next_pending_event (process_id, &event_id,
> +	= amd_dbgapi_process_next_pending_event (info.process_id, &event_id,
>  						 &event_kind);
>  
>        if (status != AMD_DBGAPI_STATUS_SUCCESS)
> @@ -1479,7 +1472,7 @@ amd_dbgapi_target::wait (ptid_t ptid, struct target_waitstatus *ws,
>        /* Drain the events for the current inferior from the amd_dbgapi and
>  	 preserve the ordering.  */
>        auto info = get_amd_dbgapi_inferior_info (current_inferior ());
> -      process_event_queue (info->process_id);
> +      process_event_queue (*info);
>  
>        std::tie (event_ptid, gpu_waitstatus) = consume_one_event (ptid.pid ());
>        if (event_ptid == minus_one_ptid)
> -- 
> 2.49.0