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

[Kenneth Graunke]

On Wednesday, February 18, 2015 12:00:39 PM Matt Turner wrote:
> 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.

The GLSL ES 1.0 specification actually very explicitly allows
overloading of built-in functions:

From the GLSL ES 1.0 specification, section 4.2.6:
"User defined functions may overload built-in functions."
and chapter 8:
"User code can overload the built-in functions but cannot redefine
them."

This is indeed different than GLSL ES 3.00, which prohibits both,
and different than desktop GLSL, which has entirely different rules.

> 
> > +          *
> > +          */
> > +         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);
> > +            }
> > +         }

This code looks good, but there's one subtle issue with where you've
placed it.  Namely, it allows this to compile:

#version 300 es
precision highp float;
out vec4 color;

void main()
{
   color = vec4(sin(0.5));
}

float sin(float x);

When handling that prototype, it will take the existing
exact_matching_signature != NULL path (since your code is the "else" to
that block), which sees it as a harmless redundant prototype, and allows
it.

I would advise simply moving this code out a level and up:

if (state->es_shader && state->language_version >= 300) {
    /* Do your code here */
    return NULL;
}

if (state->es_shader || f->has_user_signature()) {
    /* Do the old code here */
}

Please submit an updated patch and CC me.  Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150218/8741fcf8/attachment.sig>