Re: [PATCH v2 3/3] gdb/amdgpu: add precise-memory support

[Lancelot SIX <lsix@lancelotsix.com>]

Hi Simon,

> diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
> index 22c269b7992c..d38b790f53f7 100644
> --- a/gdb/amd-dbgapi-target.c
> +++ b/gdb/amd-dbgapi-target.c
> @@ -1326,6 +1338,29 @@ amd_dbgapi_target::stopped_by_hw_breakpoint ()
>    return false;
>  }
>  
> +/* Try to set the process's memory access reporting precision mode.
> +
> +   Warn if the requested mode is not supported on at least one agent in the
> +   process.
> +
> +   Error out if setting the requested mode failed for some other reason.  */
> +
> +static void
> +try_set_process_memory_precision (amd_dbgapi_inferior_info &info)
> +{
> +  auto mode = (info.precise_memory.requested
> +	       ? AMD_DBGAPI_MEMORY_PRECISION_PRECISE
> +	       : AMD_DBGAPI_MEMORY_PRECISION_NONE);
> +  amd_dbgapi_status_t status
> +    = amd_dbgapi_set_memory_precision (info.process_id, mode);
> +
> +  if (status == AMD_DBGAPI_STATUS_ERROR_NOT_SUPPORTED)
> +    warning (_("AMDGPU precise memory access reporting could not be enabled."));
> +  else if (status != AMD_DBGAPI_STATUS_SUCCESS)
> +    error (_("amd_dbgapi_set_memory_precision failed (%s)"),
> +	   get_status_string (status));

I think you forgot something like

     else
       info.precise_memory.enabled = info.precise_memory.requested;

here.

> +}
> +
>  /* Make the amd-dbgapi library attach to the process behind INF.
>  
>     Note that this is unrelated to the "attach" GDB concept / command.
> @@ -1399,6 +1434,8 @@ attach_amd_dbgapi (inferior *inf)
>    amd_dbgapi_debug_printf ("process_id = %" PRIu64 ", notifier fd = %d",
>  			   info->process_id.handle, info->notifier);
>  
> +  try_set_process_memory_precision (*info);
> +
>    /* If GDB is attaching to a process that has the runtime loaded, there will
>       already be a "runtime loaded" event available.  Consume it and push the
>       target.  */
> @@ -1443,8 +1480,10 @@ detach_amd_dbgapi (inferior *inf)
>    for (auto &&value : info->breakpoint_map)
>      delete_breakpoint (value.second);
>  
> -  /* Reset the amd_dbgapi_inferior_info.  */
> +  /* Reset the amd_dbgapi_inferior_info, except for precise_memory_mode.  */
> +  bool precise_memory_requested = info->precise_memory.requested;
>    *info = amd_dbgapi_inferior_info (inf);

Not really significant, but I am wonedring if adding a
request_precise_memory bool parameter to the amd_dbgapi_inferior_info
ctor (maybe with default value to false) would make sense.

That being said, I'm happy either way.

> +  info->precise_memory.requested = precise_memory_requested;
>  
>    maybe_reset_amd_dbgapi ();
>  }

I have tested this patch (with the fix suggested above) on gfx900 (where
precise memory is not supported) and gfx90a (where precise memory is
supported).

With the correction, the patch looks good to me.

Best,
Lancelot.