[Mesa-dev] [PATCH] glsl: trap atomic operations on illegal image formats in compiler.

[Francisco Jerez]

Dave Airlie <airlied at gmail.com> writes:

> From: Dave Airlie <airlied at redhat.com>
>
> This fixes:
> GL43-CTS.shader_image_load_store.negative-compileErrors
>
> where shader 9 was being compiled with atomic operation on an r16i.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
>  src/compiler/glsl/ast_function.cpp      | 21 +++++++++++++++++++++
>  src/compiler/glsl/builtin_functions.cpp |  6 +++---
>  src/compiler/glsl/glsl_parser_extras.h  |  7 +++++++
>  src/compiler/glsl/ir.h                  |  2 ++
>  4 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
> index fe0df15..3ea6e05 100644
> --- a/src/compiler/glsl/ast_function.cpp
> +++ b/src/compiler/glsl/ast_function.cpp
> @@ -139,6 +139,27 @@ verify_image_parameter(YYLTYPE *loc, _mesa_glsl_parse_state *state,
>        return false;
>     }
>  
> +   if (formal->data.image_atomic) {
> +     if (actual->data.image_format != GL_R32UI &&
> +	 actual->data.image_format != GL_R32I) {
> +       _mesa_glsl_error(loc, state,
> +			"atomic operations can only happen on r32ui/r32i formats.");
> +       return false;
> +     }
> +   }
> +
> +   if (formal->data.image_atomic_exchange) {
> +     if ((actual->data.image_format != GL_R32UI &&
> +	  actual->data.image_format != GL_R32I)) {
> +       /* check for the r32f special case. */
> +       if (!(state->has_shader_image_atomic_exchange_float() &&
> +	     actual->data.image_format == GL_R32F)) {

AFAICT the state->has_shader_image_atomic_exchange_float() check (and
related changes below, including the definition of
ir_variable_data::image_atomic_exchange) shouldn't be necessary because
the floating point imageAtomicExchange overload shouldn't be introduced
into the program symbol table if the r32f variant is not supported, so
passing an r32f image would lead to a type mismatch either way
regardless of what you do here.

> +	 _mesa_glsl_error(loc, state,
> +			  "atomic exchange operations can only happen on r32ui/r32i formats (or r32f in GLES).");
> +	 return false;
> +       }
> +     }
> +   }

FTR the reason why I didn't bother to introduce any format qualifier
checks whatsoever in verify_image_parameter() is the following GLSL spec
text from section 4.10 "Memory Qualifiers":

| Layout qualifiers cannot be used on formal function parameters, but
| they are not included in parameter matching.

which sounds like it's legal to pass image variables of *any* format as
parameter for any user-defined function -- Which brings me to the
question of what should happen when you have e.g. an image atomic call
inside a function passing an image formal parameter as argument to the
atomic call -- Is that supposed to be an error?  If not how do you tell
whether the format matches to emit the error above?  You don't?  If you
don't how come it must be an error to do it at the top level?  Or is it
not necessarily an error either and the CTS test is just checking
out-of-spec behaviour?  The spec wording seems *really* sketchy
honestly...

>     return true;
>  }
>  
> diff --git a/src/compiler/glsl/builtin_functions.cpp b/src/compiler/glsl/builtin_functions.cpp
> index 25d914d..9d029bb 100644
> --- a/src/compiler/glsl/builtin_functions.cpp
> +++ b/src/compiler/glsl/builtin_functions.cpp
> @@ -478,9 +478,7 @@ shader_image_atomic(const _mesa_glsl_parse_state *state)
>  static bool
>  shader_image_atomic_exchange_float(const _mesa_glsl_parse_state *state)
>  {
> -   return (state->is_version(450, 320) ||
> -           state->ARB_ES3_1_compatibility_enable ||
> -           state->OES_shader_image_atomic_enable);
> +  return (state->has_shader_image_atomic_exchange_float());
>  }
>  
>  static bool
> @@ -5436,6 +5434,8 @@ builtin_builder::_image_prototype(const glsl_type *image_type,
>     image->data.image_coherent = true;
>     image->data.image_volatile = true;
>     image->data.image_restrict = true;
> +   image->data.image_atomic = (flags & IMAGE_FUNCTION_AVAIL_ATOMIC) != 0;
> +   image->data.image_atomic_exchange = (flags & IMAGE_FUNCTION_AVAIL_ATOMIC_EXCHANGE) != 0;
>  
>     return sig;
>  }
> diff --git a/src/compiler/glsl/glsl_parser_extras.h b/src/compiler/glsl/glsl_parser_extras.h
> index 7018347..9bf9245 100644
> --- a/src/compiler/glsl/glsl_parser_extras.h
> +++ b/src/compiler/glsl/glsl_parser_extras.h
> @@ -270,6 +270,13 @@ struct _mesa_glsl_parse_state {
>        return OES_geometry_shader_enable || is_version(150, 320);
>     }
>  
> +   bool has_shader_image_atomic_exchange_float() const
> +   {
> +      return (is_version(450, 320) ||
> +	      ARB_ES3_1_compatibility_enable ||
> +	      OES_shader_image_atomic_enable);
> +   }
> +
>     void process_version_directive(YYLTYPE *locp, int version,
>                                    const char *ident);
>  
> diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
> index 0c319ea..2bf04e68 100644
> --- a/src/compiler/glsl/ir.h
> +++ b/src/compiler/glsl/ir.h
> @@ -811,6 +811,8 @@ public:
>        unsigned image_volatile:1;
>        unsigned image_restrict:1;
>  
> +      unsigned image_atomic:1;
> +      unsigned image_atomic_exchange:1;

Because each image atomic overload can only possibly have one allowed
format qualifier (for the iimage* ones it will be r32i, for uimage* it
will be r32u and for image* it will be r32f), I don't think these two
fields are necessary either, you could just put the only allowable
format into image_format and then check in verify_image_parameter()
whether an image_format has been specified for both the formal and
actual parameter (i.e. they're both anything other than GL_NONE -- It
wouldn't for the formal parameters of user-defined functions or
imageLoad/Store), and in that case check that both formats are the same.

>        /**
>         * ARB_shader_storage_buffer_object
>         */
> -- 
> 2.5.5
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160505/dd6148e1/attachment.sig>