[PATCH 2/4] Validate acc_device_t uses

[Thomas Schwinge]

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 <thomas@codesourcery.com>" in the
commit log, see <https://gcc.gnu.org/wiki/Reviewed-by>.

On 2019-11-13T16:32:13+0100, Frederik Harwath <frederik@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