<div dir="ltr">On 22 October 2013 13:00, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On 10/18/2013 01:14 PM, Paul Berry wrote: > On 11 October 2013 11:18, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a> </div><div class="im">> <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>>> wrote: > > From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a> </div>> <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>>> <div class="im">> > Checks that the variables generated meet certain criteria. > > - Vertex shader inputs have an explicit location. > > - Vertex shader outputs have an explicit location. > > - Fragment shader-only varying locations are not used. > > - Vertex shader uniforms and system values don't have an explicit > location. > > - Vertex shader constants don't have an explicit location and are > read-only. > > - No other kinds of vertex variables exist. > > It does not verify that an specific variables exist. > > Signed-off-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a> </div>> <mailto:<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>>> <div><div class="h5">> --- > src/glsl/Makefile.am | 1 + > src/glsl/tests/builtin_variable_test.cpp | 235 > +++++++++++++++++++++++++++++++ > 2 files changed, 236 insertions(+) > create mode 100644 src/glsl/tests/builtin_variable_test.cpp > > diff --git a/src/glsl/Makefile.am b/src/glsl/Makefile.am > index f43f49d..80949fb 100644 > --- a/src/glsl/Makefile.am > +++ b/src/glsl/Makefile.am > @@ -59,6 +59,7 @@ tests_general_ir_test_SOURCES = \ > $(top_srcdir)/src/mesa/program/prog_hash_table.c\ > $(top_srcdir)/src/mesa/program/symbol_table.c \ > $(GLSL_SRCDIR)/standalone_scaffolding.cpp \ > + tests/builtin_variable_test.cpp \ > tests/general_ir_test.cpp > tests_general_ir_test_CFLAGS = \ > $(PTHREAD_CFLAGS) > diff --git a/src/glsl/tests/builtin_variable_test.cpp > b/src/glsl/tests/builtin_variable_test.cpp > new file mode 100644 > index 0000000..c4711ab > --- /dev/null > +++ b/src/glsl/tests/builtin_variable_test.cpp > @@ -0,0 +1,235 @@ > +/* > + * Copyright © 2013 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person > obtaining a > + * copy of this software and associated documentation files (the > "Software"), > + * to deal in the Software without restriction, including without > limitation > + * the rights to use, copy, modify, merge, publish, distribute, > sublicense, > + * and/or sell copies of the Software, and to permit persons to > whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > the next > + * paragraph) shall be included in all copies or substantial > portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO > EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, > DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + */ > +#include <gtest/gtest.h> > +#include "standalone_scaffolding.h" > +#include "main/compiler.h" > +#include "main/mtypes.h" > +#include "main/macros.h" > +#include "ralloc.h" > +#include "ir.h" > +#include "glsl_parser_extras.h" > +#include "glsl_symbol_table.h" > + > +class common_builtin : public ::testing::Test { > +public: > + common_builtin(GLenum shader_type) > + : shader_type(shader_type) > + { > + /* empty */ > + } > + > + virtual void SetUp(); > + virtual void TearDown(); > + > + void string_starts_with_prefix(const char *str, const char *prefix); > + void names_start_with_gl(); > + void uniforms_and_system_values_dont_have_explicit_location(); > + void constants_are_constant(); > + void no_invalid_variable_modes(); > + > + GLenum shader_type; > + struct _mesa_glsl_parse_state *state; > + struct gl_shader *shader; > + void *mem_ctx; > + gl_context ctx; > + exec_list ir; > +}; > + > +void > +common_builtin::SetUp() > +{ > + this->mem_ctx = ralloc_context(NULL); > + this->ir.make_empty(); > + > + initialize_context_to_defaults(&this->ctx, API_OPENGL_COMPAT); > + > + this->shader = rzalloc(this->mem_ctx, gl_shader); > + this->shader->Type = this->shader_type; > + > + this->state = > + new(mem_ctx) _mesa_glsl_parse_state(&this->ctx, > this->shader->Type, > + this->shader); > + > + _mesa_glsl_initialize_types(this->state); > + _mesa_glsl_initialize_variables(&this->ir, this->state); > +} > + > +void > +common_builtin::TearDown() > +{ > + ralloc_free(this->mem_ctx); > + this->mem_ctx = NULL; > +} > + > +void > +common_builtin::string_starts_with_prefix(const char *str, const > char *prefix) > +{ > + const size_t len = strlen(prefix); > + char *const name_prefix = new char[len]; > > > This needs to be new char[len + 1], since you assign to its len'th > element below. </div></div>Yes... and I forgot to release the memory when I was done. Strong work. :( I'll fix both of those issues. <div><div class="h5"> > + > + strncpy(name_prefix, str, len); > + name_prefix[len] = '\0'; > + EXPECT_STREQ(prefix, name_prefix) << "Bad name " << str; > +} > + > +void > +common_builtin::names_start_with_gl() > +{ > + foreach_list(node, &this->ir) { > + ir_variable *const var = ((ir_instruction *) > node)->as_variable(); > + > + string_starts_with_prefix(var->name, "gl_"); > + } > +} > + > +void > +common_builtin::uniforms_and_system_values_dont_have_explicit_location() > +{ > + foreach_list(node, &this->ir) { > + ir_variable *const var = ((ir_instruction *) > node)->as_variable(); > + > + if (var->mode != ir_var_uniform && var->mode != > ir_var_system_value) > + continue; > + > + EXPECT_FALSE(var->explicit_location); > + EXPECT_EQ(-1, var->location); > + } > +} > + > +void > +common_builtin::constants_are_constant() > +{ > + foreach_list(node, &this->ir) { > + ir_variable *const var = ((ir_instruction *) > node)->as_variable(); > + > + if (var->mode != ir_var_auto) > + continue; > + > + EXPECT_FALSE(var->explicit_location); > + EXPECT_EQ(-1, var->location); > + EXPECT_TRUE(var->read_only); > + } > +} > + > +void > +common_builtin::no_invalid_variable_modes() > +{ > + foreach_list(node, &this->ir) { > + ir_variable *const var = ((ir_instruction *) > node)->as_variable(); > + > + switch (var->mode) { > + case ir_var_auto: > + case ir_var_uniform: > + case ir_var_shader_in: > + case ir_var_shader_out: > + case ir_var_system_value: > + break; > + > + /* Invalid modes that we know. */ > + case ir_var_function_in: > + case ir_var_function_out: > + case ir_var_function_inout: > + case ir_var_const_in: > + case ir_var_temporary: > + EXPECT_NE(ir_var_function_in, var->mode); > + EXPECT_NE(ir_var_function_out, var->mode); > + EXPECT_NE(ir_var_function_inout, var->mode); > + EXPECT_NE(ir_var_const_in, var->mode); > + EXPECT_NE(ir_var_temporary, var->mode); > + break; > > > This block seems really strange to me. All of these invalid modes are > adequately covered by the default case, which is easier to read. </div></div>I did that so that when a failure was encountered a more reasonable message would be logged. However, it occurs to me that isn't very useful because you still don't know which variable failed. I've deleted this block and changed the default case to: default: ADD_FAILURE() << "Built-in variable " << var->name << " has an invalid mode " << int(var->mode); break; That should make it easier to track down failures. </blockquote><div> </div><div>That sounds reasonable. Thanks. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> > With those two things fixed, this patch is: > > Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a> </div>> <mailto:<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>> <div><div class="h5">> > > + > + /* Invalid modes that we don't know. */ > + default: > + EXPECT_TRUE(false) << "invalid mode " << int(var->mode); > + break; > + } > + } > +} > + > +/************************************************************/ > + > +class vertex_builtin : public common_builtin { > +public: > + vertex_builtin() > + : common_builtin(GL_VERTEX_SHADER) > + { > + /* empty */ > + } > +}; > + > +TEST_F(vertex_builtin, names_start_with_gl) > +{ > + common_builtin::names_start_with_gl(); > +} > + > +TEST_F(vertex_builtin, inputs_have_explicit_location) > +{ > + foreach_list(node, &this->ir) { > + ir_variable *const var = ((ir_instruction *) > node)->as_variable(); > + > + if (var->mode != ir_var_shader_in) > + continue; > + > + EXPECT_TRUE(var->explicit_location); > + EXPECT_NE(-1, var->location); > + EXPECT_GT(VERT_ATTRIB_GENERIC0, var->location); > + EXPECT_EQ(0u, var->location_frac); > + } > +} > + > +TEST_F(vertex_builtin, outputs_have_explicit_location) > +{ > + foreach_list(node, &this->ir) { > + ir_variable *const var = ((ir_instruction *) > node)->as_variable(); > + > + if (var->mode != ir_var_shader_out) > + continue; > + > + EXPECT_TRUE(var->explicit_location); > + EXPECT_NE(-1, var->location); > + EXPECT_GT(VARYING_SLOT_VAR0, var->location); > + EXPECT_EQ(0u, var->location_frac); > + > + /* Several varyings only exist in the fragment shader. Be > sure that no > + * outputs with these locations exist. > + */ > + EXPECT_NE(VARYING_SLOT_PNTC, var->location); > + EXPECT_NE(VARYING_SLOT_FACE, var->location); > + EXPECT_NE(VARYING_SLOT_PRIMITIVE_ID, var->location); > + } > +} > + > +TEST_F(vertex_builtin, > uniforms_and_system_values_dont_have_explicit_location) > +{ > + > common_builtin::uniforms_and_system_values_dont_have_explicit_location(); > +} > + > +TEST_F(vertex_builtin, constants_are_constant) > +{ > + common_builtin::constants_are_constant(); > +} > + > +TEST_F(vertex_builtin, no_invalid_variable_modes) > +{ > + common_builtin::no_invalid_variable_modes(); > +} > -- > 1.8.1.4 > > _______________________________________________ > mesa-dev mailing list </div></div>> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a>> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>