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

Simon Marchi via Gdb-patches
2023-09-15T14:49:10Z

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