Hi Frederik! You once had this patch separate, but then merged into the upstream submission of 'acc_get_property'; let's please keep this separate.
With changes as indicated below, please commit this to trunk (without the three hunks related to 'acc_get_property', of course; these will then go into the 'acc_get_property' changes, don't need to re-post 'acc_get_property' because of that). To record the review effort, please include "Reviewed-by: Thomas Schwinge <tho...@codesourcery.com>" in the commit log, see <https://gcc.gnu.org/wiki/Reviewed-by>. On 2019-11-13T16:32:13+0100, Frederik Harwath <frede...@codesourcery.com> wrote: > Check that function arguments of type acc_device_t > are valid enumeration values in all publicly visible > functions. > > libgomp/ > * oacc-init.c (acc_known_device_type): Add function. > (unknown_device_type_error): Add function. > (name_of_acc_device_t): Change to call unknown_device_type_error > on unknown type. > (resolve_device): Use acc_known_device_type. > (acc_init): Fail if acc_device_t argument is not valid. > (acc_shutdown): Likewise. > (acc_get_num_devices): Likewise. > (acc_set_device_type): Likewise. > (acc_get_device_num): Likewise. > (acc_set_device_num): Likewise. > (get_property_any): Likewise. > (acc_get_property): Likewise. > (acc_get_property_string): Likewise. > (acc_on_device): Likewise. > (goacc_save_and_set_bind): Likewise. Proper indenting. > --- a/libgomp/oacc-init.c > +++ b/libgomp/oacc-init.c > @@ -93,6 +93,21 @@ get_openacc_name (const char *name) > return name; > } > > + > +static int > +acc_known_device_type (acc_device_t __arg) No leading underscores ('__arg') in implementation files. (Or, just us 'd' as often/always done elsewhere in this file.) Often, such inquiry functions are post-fixed with '_p' (but not always). Can drop the 'acc_' prefix, as this is an internal ('static') function. Return type should be 'bool'. > +{ > + return __arg >= 0 && __arg < _ACC_device_hwm; > +} > + > + > +static void > +unknown_device_type_error (unsigned invalid_type) I suggest this should take an 'acc_device_t, and then cast to '(unsigned)' when used here: > +{ > + gomp_fatal ("unknown device type %u", invalid_type); > +} > + > + > static const char * > name_of_acc_device_t (enum acc_device_t type) > { Also, move the two new functions before 'get_openacc_name'. Generally, does usage of these functions obsolete some existing usage of 'acc_dev_num_out_of_range'? (OK to address later.) > @@ -103,8 +118,9 @@ name_of_acc_device_t (enum acc_device_t type) > case acc_device_host: return "host"; > case acc_device_not_host: return "not_host"; > case acc_device_nvidia: return "nvidia"; > - default: gomp_fatal ("unknown device type %u", (unsigned) type); > + default: unknown_device_type_error(type); Missing space before '('. Not just here, but generally. > } > + return 0; /** Never reached **/ > } As done elsewhere in libgomp, instead of that 'return' plus comment, just call '__builtin_unreachable ()'. > @@ -123,7 +139,7 @@ resolve_device (acc_device_t d, bool fail_is_error) > if (goacc_device_type) > { > /* Lookup the named device. */ > - while (++d != _ACC_device_hwm) > + while (acc_known_device_type (++d)) > if (dispatchers[d] > && !strcasecmp (goacc_device_type, > get_openacc_name (dispatchers[d]->name)) > @@ -147,7 +163,7 @@ resolve_device (acc_device_t d, bool fail_is_error) > > case acc_device_not_host: > /* Find the first available device after acc_device_not_host. */ > - while (++d != _ACC_device_hwm) > + while (acc_known_device_type (++d)) > if (dispatchers[d] && dispatchers[d]->get_num_devices_func () > 0) > goto found; > if (d_arg == acc_device_default) > @@ -168,7 +184,7 @@ resolve_device (acc_device_t d, bool fail_is_error) > break; > > default: > - if (d > _ACC_device_hwm) > + if (!acc_known_device_type (d)) > { > if (fail_is_error) > goto unsupported_device; Note that this had 'd > _ACC_device_hwm', not '>=' as it now does, that is, previously didn't reject 'd == _ACC_device_hwm' but now does -- but I suppose this was an (minor) bug that existed before, so OK to change as you did? > @@ -328,7 +344,6 @@ acc_shutdown_1 (acc_device_t d) > gomp_unload_device (acc_dev); > gomp_mutex_unlock (&acc_dev->lock); > } > - > gomp_mutex_lock (&goacc_thread_lock); > > /* Free target-specific TLS data and close all devices. */ > @@ -494,7 +509,6 @@ goacc_attach_host_thread_to_device (int ord) > thr->api_info = NULL; > /* Initially, all callbacks for all events are enabled. */ > thr->prof_callbacks_enabled = true; > - > thr->target_tls > = acc_dev->openacc.create_thread_data_func (ord); > } Please avoid doing such unrelated changes. > @@ -505,12 +519,15 @@ goacc_attach_host_thread_to_device (int ord) > void > acc_init (acc_device_t d) > { > + if (!acc_known_device_type (d)) > + unknown_device_type_error(d); > + > gomp_init_targets_once (); > > gomp_mutex_lock (&acc_device_lock); > cached_base_dev = acc_init_1 (d, acc_construct_runtime_api, 0); > gomp_mutex_unlock (&acc_device_lock); > - > + > goacc_attach_host_thread_to_device (-1); > } > > @@ -519,6 +536,9 @@ ialias (acc_init) > void > acc_shutdown (acc_device_t d) > { > + if (!acc_known_device_type (d)) > + unknown_device_type_error(d); > + > gomp_init_targets_once (); > > gomp_mutex_lock (&acc_device_lock); > @@ -533,6 +553,9 @@ ialias (acc_shutdown) > int > acc_get_num_devices (acc_device_t d) > { > + if (!acc_known_device_type (d)) > + unknown_device_type_error(d); > + > int n = 0; > struct gomp_device_descr *acc_dev; > > @@ -564,6 +587,9 @@ ialias (acc_get_num_devices) > void > acc_set_device_type (acc_device_t d) > { > + if (!acc_known_device_type (d)) > + unknown_device_type_error(d); > + > struct gomp_device_descr *base_dev, *acc_dev; > struct goacc_thread *thr = goacc_thread (); > > @@ -648,12 +674,12 @@ ialias (acc_get_device_type) > int > acc_get_device_num (acc_device_t d) > { > + if (!acc_known_device_type (d)) > + unknown_device_type_error(d); > + > const struct gomp_device_descr *dev; > struct goacc_thread *thr = goacc_thread (); > > - if (d >= _ACC_device_hwm) > - gomp_fatal ("unknown device type %u", (unsigned) d); > - > acc_prof_info prof_info; > acc_api_info api_info; > bool profiling_p = GOACC_PROFILING_SETUP_P (thr, &prof_info, &api_info); > @@ -683,6 +709,9 @@ ialias (acc_get_device_num) > void > acc_set_device_num (int ord, acc_device_t d) > { > + if (!acc_known_device_type (d)) > + unknown_device_type_error(d); > + > struct gomp_device_descr *base_dev, *acc_dev; > int num_devices; > > @@ -806,6 +844,9 @@ ialias (acc_get_property_string) > int __attribute__ ((__optimize__ ("O2"))) > acc_on_device (acc_device_t dev) > { > + if (!acc_known_device_type (dev)) > + unknown_device_type_error(dev); > + > return __builtin_acc_on_device (dev); > } Please drop this last one; maybe mention this in the function comment. This function is meant to just forward to '__builtin_acc_on_device'. Any 'acc_known_device_type' checking should thus be done as part of the builtin expansion -- if that's feasible at all. This will need further thought, can be addressed later. > @@ -836,6 +877,9 @@ goacc_runtime_initialize (void) > attribute_hidden void > goacc_save_and_set_bind (acc_device_t d) > { > + if (!acc_known_device_type (d)) > + unknown_device_type_error (d); > + > struct goacc_thread *thr = goacc_thread (); > > assert (!thr->saved_bound_dev); This is an internal-only function, guaranteed to always be called with a valid 'acc_device_t', so if anything at all, this should 'assert', or just drop this one. Grüße Thomas
