Re: [PATCH 4/5] gdb/amd-dbgapi: factor out require_forward_progress overload to target one inferior

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

On Thu, Jun 05, 2025 at 04:16:27PM -0400, Simon Marchi wrote:
> A following patch will want to call require_forward_progress for a given
> inferior.  Extract a new require_forward_progress overload from the
> existing require_forward_progress function that targets a specific
> inferior.

Hi,

This LGTM, thanks.

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

Best,
Lancelot.

> 
> Change-Id: I54f42b83eb8443d4d91747ffbc86eaeb017f1e49
> ---
>  gdb/amd-dbgapi-target.c | 49 +++++++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
> index 1ec20d1874ec..e86d7f34d0f6 100644
> --- a/gdb/amd-dbgapi-target.c
> +++ b/gdb/amd-dbgapi-target.c
> @@ -443,6 +443,32 @@ async_event_handler_mark ()
>    mark_async_event_handler (amd_dbgapi_async_event_handler);
>  }
>  
> +/* Set forward progress requirement to REQUIRE for inferior INFO.  */
> +
> +static void
> +require_forward_progress (amd_dbgapi_inferior_info &info, bool require)
> +{
> +  /* If we try to disable forward progress requirement but the target expects
> +     resumed threads to be committed to the target, we could wait for events
> +     that will never arrive.  */
> +  if (!require)
> +    gdb_assert (!info.inf->process_target ()->commit_resumed_state);
> +
> +  gdb_assert (info.process_id != AMD_DBGAPI_PROCESS_NONE);
> +
> +  /* Don't do unnecessary calls to amd-dbgapi to avoid polluting the logs.  */
> +  if (info.forward_progress_required == require)
> +    return;
> +
> +  const auto progress
> +    = require ? AMD_DBGAPI_PROGRESS_NORMAL : AMD_DBGAPI_PROGRESS_NO_FORWARD;
> +  const auto status
> +    = amd_dbgapi_process_set_progress (info.process_id, progress);
> +  gdb_assert (status == AMD_DBGAPI_STATUS_SUCCESS);
> +
> +  info.forward_progress_required = require;
> +}
> +
>  /* Set forward progress requirement to REQUIRE for all processes of PROC_TARGET
>     matching PTID.  */
>  
> @@ -450,12 +476,6 @@ static void
>  require_forward_progress (ptid_t ptid, process_stratum_target *proc_target,
>  			  bool require)
>  {
> -  /* If we try to disable forward progress requirement but the target expects
> -     resumed threads to be committed to the target, we could wait for events
> -     that will never arrive.  */
> -  if (!require)
> -    gdb_assert (!proc_target->commit_resumed_state);
> -
>    for (inferior *inf : all_inferiors (proc_target))
>      {
>        if (ptid != minus_one_ptid && inf->pid != ptid.pid ())
> @@ -463,21 +483,8 @@ require_forward_progress (ptid_t ptid, process_stratum_target *proc_target,
>  
>        amd_dbgapi_inferior_info *info = get_amd_dbgapi_inferior_info (inf);
>  
> -      if (info->process_id == AMD_DBGAPI_PROCESS_NONE)
> -	continue;
> -
> -      /* Don't do unnecessary calls to amd-dbgapi to avoid polluting the logs.  */
> -      if (info->forward_progress_required == require)
> -	continue;
> -
> -      amd_dbgapi_status_t status
> -	= amd_dbgapi_process_set_progress
> -	    (info->process_id, (require
> -				? AMD_DBGAPI_PROGRESS_NORMAL
> -				: AMD_DBGAPI_PROGRESS_NO_FORWARD));
> -      gdb_assert (status == AMD_DBGAPI_STATUS_SUCCESS);
> -
> -      info->forward_progress_required = require;
> +      if (info->process_id != AMD_DBGAPI_PROCESS_NONE)
> +	require_forward_progress (*info, require);
>  
>        /* If ptid targets a single inferior and we have found it, no need to
>  	 continue.  */
> -- 
> 2.49.0