On 22 October 2013 13:00, Ian Romanick <i...@freedesktop.org> wrote: > On 10/18/2013 01:14 PM, Paul Berry wrote: > > On 11 October 2013 11:18, Ian Romanick <i...@freedesktop.org > > <mailto:i...@freedesktop.org>> wrote: > > > > From: Ian Romanick <ian.d.roman...@intel.com > > <mailto:ian.d.roman...@intel.com>> > > > > 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 <ian.d.roman...@intel.com > > <mailto:ian.d.roman...@intel.com>> > > --- > > 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. > > Yes... and I forgot to release the memory when I was done. Strong work. > :( I'll fix both of those issues. > > > + > > + 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. > > 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. >
That sounds reasonable. Thanks. > > > With those two things fixed, this patch is: > > > > Reviewed-by: Paul Berry <stereotype...@gmail.com > > <mailto:stereotype...@gmail.com>> > > > > > > + > > + /* 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 > > mesa-dev@lists.freedesktop.org <mailto: > mesa-dev@lists.freedesktop.org> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
