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

[Samuel Iglesias Gonsálvez]

On Wednesday 18 February 2015 15:08:19 Kenneth Graunke wrote:
> 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_functio
> > > n_vertex
> > > dEQP-GLES3.functional.shaders.functions.invalid.overload_builtin_functi
> > > on_fragment
> > > dEQP-GLES3.functional.shaders.functions.invalid.redefine_builtin_functi
> > > on_vertex
> > > dEQP-GLES3.functional.shaders.functions.invalid.redefine_builtin_functi
> > > on_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!

OK, I will do it.

Thanks for your review!

Sam
-------------- 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/20150219/58305550/attachment.sig>