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

[Simon Marchi <simark@simark.ca>]

On 6/6/25 7:00 AM, Lancelot SIX wrote:
> 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?

Yes, makes sense.

> 
>> +   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.

Oh, yes, that's a good idea.  Or, we could just skip the
AMD_DBGAPI_EVENT_INFO_PROCESS and AMD_DBGAPI_PROCESS_INFO_OS_ID calls
altogether, since we know for which process the event is anyway.  I will
add this as a separate patch just after this one.

Simon