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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 9/15/23 06:13, Lancelot SIX via Gdb-patches wrote:
> 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;

Huh that's true, that got lost in my refactoring.  How come the test
didn't catch it?  Oh, oh my machine I have multiple AMD GPUs, not all of
which support this memory precision feature.  So it would just stay
disabled.  I spoke to Lancelot offline, there is apparently no way right
now to tell the whole stack to just consider a single card.

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

Yeah I can do that, it allows this to be a one-liner:

  *info = amd_dbgapi_inferior_info (inf, info->precise_memory.requested);

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

Thanks, I will post a v3 just to be sure.

Simon