Hi! On 2020-01-29T10:52:57+0100, "Harwath, Frederik" <frede...@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
signature.asc
Description: PGP signature
