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 <el...@igalia.com> wrote: > > > From: Samuel Iglesias Gonsalvez <sigles...@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 <sigles...@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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev