Re: [PATCH] Add OpenACC acc_get_property support for AMD GCN

[Andrew Stubbs <ams@codesourcery.com>]

On 29/01/2020 17:44, Thomas Schwinge wrote:
>> @@ -1513,6 +1518,23 @@ init_hsa_context (void)
> 
>> +  size_t len = sizeof hsa_context.driver_version_s;
>> +  int printed = snprintf (hsa_context.driver_version_s, len,
>> +			  "HSA Runtime %hu.%hu", (unsigned short int)major,
>> +			  (unsigned short int)minor);
>> +  if (printed >= len)
>> +    GCN_WARNING ("HSA runtime version string was truncated."
>> +		 "Version %hu.%hu is too long.", (unsigned short int)major,
>> +		 (unsigned short int)minor);
> 
> (Can it actually happen that 'snprintf' returns 'printed > len' --
> meaning that it's written into random memory?  I thought 'snprintf' has a
> hard stop at 'len'?  Or does this indicate the amount of memory it
> would've written?  I should re-read the manpage at some point...)  ;-)

snprintf returns the length it would have been, if not truncated.

> For 'printed = len' does or doesn't 'snprintf' store the terminating
> 'NUL' character, or do we manually have to set:
> 
>      hsa_context.driver_version_s[len - 1] = '\0';
> 
> ... in that case?

It does the right thing already, hence why printed == len is an 
overflow: the last character will be missing.

> 
>> @@ -3410,15 +3432,19 @@ GOMP_OFFLOAD_init_device (int n)
> 
>> -  char buf[64];
>>     status = hsa_fns.hsa_agent_get_info_fn (agent->id, HSA_AGENT_INFO_NAME,
>> -					  &buf);
>> +					  &agent->name);
>>     if (status != HSA_STATUS_SUCCESS)
>>       return hsa_error ("Error querying the name of the agent", status);
> 
> (That's of course pre-existing, but) this looks like a dangerous API,
> given that 'hsa_agent_get_info_fn' doesn't know 'sizeof agent->name' (or
> 'sizeof buf' before)...

Those ones are written into the HSA documentation. They are fixed-sized 
fields, so there must be 64 bytes regardless of the actual name of the 
agent.

>> +  status = hsa_fns.hsa_agent_get_info_fn (agent->id, HSA_AGENT_INFO_VENDOR_NAME,
>> +					  &agent->vendor_name);
>> +  if (status != HSA_STATUS_SUCCESS)
>> +    return hsa_error ("Error querying the vendor name of the agent", status);
> 
> Similar here.

Same reason.

>> --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property.c
>> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_get_property.c
>> @@ -3,8 +3,7 @@
>>      of all device types mentioned in the OpenACC standard.
>>   
>>      See also acc_get_property.f90. */
>> -/* { dg-do run { target { { ! { openacc_host_selected } } && { ! { openacc_amdgcn_accel_selected } } } } } */
>> -/* FIXME: This test does not work with the GCN implementation stub yet.  */
> 
> (I had wondered why 'host' was disabled here; now I don't need to wonder
> any longer.)  ;-)
> 
>> +/* { dg-do run } */
> 
> This one is not actually needed, is the default.  (But no reason to
> remove it.)
> 
>> --- a/libgomp/testsuite/libgomp.oacc-fortran/acc_get_property.f90
>> +++ b/libgomp/testsuite/libgomp.oacc-fortran/acc_get_property.f90
>> @@ -3,8 +3,6 @@
>>   ! of all device types mentioned in the OpenACC standard.
>>   !
>>   ! See also acc_get_property.c
>> -! { dg-do run { target { { ! { openacc_host_selected } } && { ! { openacc_amdgcn_accel_selected } } } } }
>> -! FIXME: This test does not work with the GCN implementation stub yet.
> 
> ..., but here, with 'dg-do run' removed, this is no longer doing "torture
> testing" (cycle through optimization levels/flags).  (See the rationale
> in 'libgomp/testsuite/libgomp.oacc-fortran/fortran.exp'.)
> 
> ... but that should actually be OK given that we're really only testing
> the 'acc_get_property'/'acc_get_property_string' API calls, no other
> Fortran extravaganza.

Thanks for the additional review, Thomas.

Andrew