[Mesa-dev] [PATCH 2/6] glsl/tests: Verify vertex shader built-ins generated by _mesa_glsl_initialize_variables
Paul Berry
stereotype441 at gmail.com
Tue Oct 22 22:09:43 CEST 2013
On 22 October 2013 13:00, Ian Romanick <idr at freedesktop.org> wrote:
> On 10/18/2013 01:14 PM, Paul Berry wrote:
> > On 11 October 2013 11:18, Ian Romanick <idr at freedesktop.org
> > <mailto:idr at freedesktop.org>> wrote:
> >
> > From: Ian Romanick <ian.d.romanick at intel.com
> > <mailto:ian.d.romanick at 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.romanick at intel.com
> > <mailto:ian.d.romanick at 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 <stereotype441 at gmail.com
> > <mailto:stereotype441 at 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 at lists.freedesktop.org <mailto:
> mesa-dev at lists.freedesktop.org>
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131022/7f72d59f/attachment-0001.html>
More information about the mesa-dev
mailing list