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
