[Mesa-dev] [PATCH 08/10] glsl: A shader cannot redefine or overload built-in functions in GLSL ES 3.00

[Matt Turner]

On Mon, Dec 1, 2014 at 5:04 AM, Eduardo Lima Mitev <elima at igalia.com> wrote:
> From: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>
> Create a new search function to look for matching built-in functions by name
> and use it for built-in function redefinition or overload in GLSL ES 3.00.
>
> GLSL ES 3.0 spec, chapter 6.1 "Function Definitions", page 71
>
> "A shader cannot redefine or overload built-in functions."
>
> In case of GLSL ES 1.0 spec, chapter 6.1 "Function Definitions", page 54
>
> "Function names can be overloaded. This allows the same function name to be
> used for multiple functions, as long as the argument list types differ. If
> functions’ names and argument types match, then their return type and
> parameter qualifiers must also match. Function signature matching is based on
> parameter type only, no qualifiers are used. Overloading is used heavily in the
> built-in functions. When overloaded functions (or indeed any functions) are
> resolved, an exact match for the function's signature is sought. This includes
> exact match of array size as well. No promotion or demotion of the return type
> or input argument types is done. All expected combination of inputs and
> outputs must be defined as separate functions."
>
> So this check is specific to GLSL ES 3.00.
>
> This patch fixes the following dEQP tests:
>
> dEQP-GLES3.functional.shaders.functions.invalid.overload_builtin_function_vertex
> dEQP-GLES3.functional.shaders.functions.invalid.overload_builtin_function_fragment
> dEQP-GLES3.functional.shaders.functions.invalid.redefine_builtin_function_vertex
> dEQP-GLES3.functional.shaders.functions.invalid.redefine_builtin_function_fragment
>
> No piglit regressions.
>
> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
> ---
>  src/glsl/ast_to_hir.cpp        | 22 ++++++++++++++++++++++
>  src/glsl/builtin_functions.cpp | 11 +++++++++++
>  src/glsl/ir.h                  |  4 ++++
>  3 files changed, 37 insertions(+)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index fe1e129..b7074bc 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -4167,6 +4167,28 @@ ast_function::hir(exec_list *instructions,
>                 return NULL;
>              }
>           }
> +      } else {
> +         /* From GLSL ES 3.0 spec, chapter 6.1 "Function Definitions", page 71:
> +          *
> +          * "A shader cannot redefine or overload built-in functions."
> +          *
> +          * While in GLSL ES 1.0 spec, chapter 6.1 "Function Definitions", page
> +          * 54, this is allowed:
> +          *
> +          * "Function names can be overloaded. [...] Overloading is used heavily
> +          * in the built-in functions."

I don't think that quote is really explicitly saying that you can
overload built-in functions. It's just saying that built-in functions
contain many overloads.

It doesn't, however, prohibit the user from adding overloads of their own.

I'd probably replace the spec citation with just a statement that the
GLSL ES 1.0 spec doesn't prohibit overloading built-in functions,
since it really doesn't say anything explicitly about the topic.

> +          *
> +          */
> +         if (state->es_shader && state->language_version >= 300) {
> +            /* Local shader has no exact candidates; check the built-ins. */
> +            _mesa_glsl_initialize_builtin_functions();
> +            if (_mesa_glsl_find_builtin_function_by_name(state, name)) {
> +               YYLTYPE loc = this->get_location();
> +               _mesa_glsl_error(& loc, state,
> +                                "A shader cannot redefine or overload built-in "
> +                                "function `%s' in GLSL ES 3.00", name);
> +            }
> +         }
>        }
>     }
>
> diff --git a/src/glsl/builtin_functions.cpp b/src/glsl/builtin_functions.cpp
> index bb7fbcd..f5052d3 100644
> --- a/src/glsl/builtin_functions.cpp
> +++ b/src/glsl/builtin_functions.cpp
> @@ -4618,6 +4618,17 @@ _mesa_glsl_find_builtin_function(_mesa_glsl_parse_state *state,
>     return s;
>  }
>
> +ir_function *
> +_mesa_glsl_find_builtin_function_by_name(_mesa_glsl_parse_state *state,
> +                                         const char *name)
> +{
> +   ir_function * f;

Remove the space after the *.

As far as I can tell, this patch looks correct. Confirmation from Ken
would be nice as well.

(This behavior should have been the default in all versions of GLSL as
far as I'm concerned)

Reviewed-by: Matt Turner <mattst88 at gmail.com>