[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 12:00:39 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.
> 
> > +          *
> > +          */
> > +         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>

Following Kenneth's comments, I will send an updated version of it for review. 
Also, I will modify the GLSL ES 1.0 spec citation following what Kenneth says 
in his reply.

Because of these changes, I am not going to add your R-b yet.

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/5def710b/attachment.sig>