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

[Thomas Schwinge <thomas@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 4435 bytes --]

Hi!

On 2020-01-29T10:52:57+0100, "Harwath, Frederik" <frederik@codesourcery.com> wrote:
> On 28.01.20 16:42, Andrew Stubbs wrote:
>> On 28/01/2020 14:55, Harwath, Frederik wrote:
>> 
>> If we're going to use a fixed-size buffer then we should use snprintf and emit GCN_WARNING if the return value is greater than "sizeof(driver_version_s)", even though that is unlikely. Do the same in the testcase, but use a bigger buffer so that truncation causes a mismatch and test failure.

> --- a/libgomp/plugin/plugin-gcn.c
> +++ b/libgomp/plugin/plugin-gcn.c
> @@ -425,7 +425,10 @@ struct agent_info
>  
>    /* The instruction set architecture of the device. */
>    gcn_isa device_isa;
> -
> +  /* Name of the agent. */
> +  char name[64];
> +  /* Name of the vendor of the agent. */
> +  char vendor_name[64];
>    /* Command queues of the agent.  */
>    hsa_queue_t *sync_queue;
>    struct goacc_asyncqueue *async_queues, *omp_async_queue;
> @@ -544,6 +547,8 @@ struct hsa_context_info
>    int agent_count;
>    /* Array of agent_info structures describing the individual HSA agents.  */
>    struct agent_info *agents;
> +  /* Driver version string. */
> +  char driver_version_s[30];
>  };

> @@ -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...)  ;-)

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?

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

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


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


Grüße
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 658 bytes --]