On 07/30/2015 08:56 AM, Tapani Pälli wrote:
On 07/30/2015 08:50 AM, Timothy Arceri wrote:
On 30 July 2015 3:27:41 pm AEST, "Tapani Pälli"
<tapani.pa...@intel.com> wrote:
On 07/30/2015 08:25 AM, Tapani Pälli wrote:
Hi Timothy;
Would it be OK for you to wait a bit with these 3 first patches? I'm
currently going through failing PIQ tests and I have a 1 line fix to
valid_program_resource_index_name() that together with your patch
"glsl: set stage flag for structs and arrays in resource list" will
fix all of the failing "ES31-CTS.program_interface_query.uniform"
tests. Frankly I don't see the value of such a big refactor necessary
for this.
The value is in removing almost 200 lines of complicated code trying
to solve the same problem in different ways. How its this not a good
thing?
Also once this is done we can also work towards removing UniformHash
from Mesa.
Sure, I agree that removing lots of code is a good thing. I just don't
quite yet understand how some of this will work, many of the removed
checks were there for a good reason (faling tests!). Just to make
sure, did you run Piglit and conformance without regressions? Anyway,
I will read through your changes and think about this more.
Replying myself, no regressions observed :)
Yep, it seems that parse_program_resource_name() tackles the cases. I
will do some further testing, particularly on the 'gl_VertexIDMESA'
special case and review the patches during today or tomorrow.
Woops, make it "ES31-CTS.program_interface_query.uniform-types", not
"ES31-CTS.program_interface_query.uniform". It looks to me that most
tests fail just because of not figuring out the correct index for the
resource and that can be fixed by a valid_program_resource_index_name
change.
On 07/29/2015 04:56 PM, Timothy Arceri wrote:
This removes the need for multiple functions designed to validate an
array
subscript and replaces them with a call to a single function.
The change also means that validation is now only done once and the
index
is retrived at the same time, as a result the getUniformLocation
code
can
be simplified saving an extra hash table lookup (and yet another
validation call).
This chage also fixes some tests in:
ES31-CTS.program_interface_query.uniform
V3: rebase on subroutines, and move the resource index array == 0
check into _mesa_GetProgramResourceIndex() to simplify things
further
V2: Fix bounds checks for program input/output, split unrelated
comment fix
and _mesa_get_uniform_location() removal into their own patch.
Cc: Tapani Pälli <tapani.pa...@intel.com>
---
src/mesa/main/program_resource.c | 14 ++--
src/mesa/main/shader_query.cpp | 174
+++++++++++++++++++++------------------
src/mesa/main/shaderapi.c | 2 +-
src/mesa/main/shaderapi.h | 3 +-
src/mesa/main/uniforms.c | 5 +-
5 files changed, 106 insertions(+), 92 deletions(-)
diff --git a/src/mesa/main/program_resource.c
b/src/mesa/main/program_resource.c
index 641ef22..fdbd5b3 100644
--- a/src/mesa/main/program_resource.c
+++ b/src/mesa/main/program_resource.c
@@ -229,6 +229,7 @@ _mesa_GetProgramResourceIndex(GLuint program,
GLenum programInterface,
const GLchar *name)
{
GET_CURRENT_CONTEXT(ctx);
+ unsigned array_index = 0;
struct gl_program_resource *res;
struct gl_shader_program *shProg =
_mesa_lookup_shader_program_err(ctx, program,
@@ -268,13 +269,10 @@ _mesa_GetProgramResourceIndex(GLuint program,
GLenum programInterface,
case GL_PROGRAM_OUTPUT:
case GL_UNIFORM:
case GL_TRANSFORM_FEEDBACK_VARYING:
- /* Validate name syntax for array variables */
- if (!valid_program_resource_index_name(name))
- return GL_INVALID_INDEX;
- /* fall-through */
case GL_UNIFORM_BLOCK:
- res = _mesa_program_resource_find_name(shProg,
programInterface, name);
- if (!res)
+ res = _mesa_program_resource_find_name(shProg,
programInterface, name,
+ &array_index);
+ if (!res || array_index > 0)
return GL_INVALID_INDEX;
return _mesa_program_resource_index(shProg, res);
@@ -403,7 +401,7 @@ _mesa_GetProgramResourceLocation(GLuint program,
GLenum programInterface,
struct gl_shader_program *shProg =
lookup_linked_program(program,
"glGetProgramResourceLocation");
- if (!shProg || !name || invalid_array_element_syntax(name))
+ if (!shProg || !name)
return -1;
/* Validate programInterface. */
@@ -453,7 +451,7 @@ _mesa_GetProgramResourceLocationIndex(GLuint
program, GLenum programInterface,
struct gl_shader_program *shProg =
lookup_linked_program(program,
"glGetProgramResourceLocationIndex");
- if (!shProg || !name || invalid_array_element_syntax(name))
+ if (!shProg || !name)
return -1;
/* From the GL_ARB_program_interface_query spec:
diff --git a/src/mesa/main/shader_query.cpp
b/src/mesa/main/shader_query.cpp
index 3e52a09..dd11b81 100644
--- a/src/mesa/main/shader_query.cpp
+++ b/src/mesa/main/shader_query.cpp
@@ -44,7 +44,8 @@ extern "C" {
static GLint
program_resource_location(struct gl_shader_program *shProg,
- struct gl_program_resource *res, const
char *name);
+ struct gl_program_resource *res, const
char *name,
+ unsigned array_index);
/**
* Declare convenience functions to return resource data in a
given
type.
@@ -272,13 +273,15 @@ _mesa_GetAttribLocation(GLhandleARB program,
const GLcharARB * name)
if (shProg->_LinkedShaders[MESA_SHADER_VERTEX] == NULL)
return -1;
+ unsigned array_index = 0;
struct gl_program_resource *res =
- _mesa_program_resource_find_name(shProg, GL_PROGRAM_INPUT,
name);
+ _mesa_program_resource_find_name(shProg, GL_PROGRAM_INPUT,
name,
+ &array_index);
if (!res)
return -1;
- GLint loc = program_resource_location(shProg, res, name);
+ GLint loc = program_resource_location(shProg, res, name,
array_index);
/* The extra check against against 0 is made because of
builtin-attribute
* locations that have offset applied. Function
program_resource_location
@@ -456,13 +459,15 @@ _mesa_GetFragDataLocation(GLuint program,
const
GLchar *name)
if (shProg->_LinkedShaders[MESA_SHADER_FRAGMENT] == NULL)
return -1;
+ unsigned array_index = 0;
struct gl_program_resource *res =
- _mesa_program_resource_find_name(shProg, GL_PROGRAM_OUTPUT,
name);
+ _mesa_program_resource_find_name(shProg, GL_PROGRAM_OUTPUT,
name,
+ &array_index);
if (!res)
return -1;
- GLint loc = program_resource_location(shProg, res, name);
+ GLint loc = program_resource_location(shProg, res, name,
array_index);
/* The extra check against against 0 is made because of
builtin-attribute
* locations that have offset applied. Function
program_resource_location
@@ -552,39 +557,32 @@ _mesa_program_resource_array_size(struct
gl_program_resource *res)
return 0;
}
-static int
-array_index_of_resource(struct gl_program_resource *res,
- const char *name)
+/**
+ * Checks if array subscript is valid and if so sets array_index.
+ */
+static bool
+valid_array_index(const GLchar *name, unsigned *array_index)
{
- assert(res->Data);
+ unsigned idx = 0;
+ const GLchar *out_base_name_end;
- switch (res->Type) {
- case GL_PROGRAM_INPUT:
- case GL_PROGRAM_OUTPUT:
- return get_matching_index(RESOURCE_VAR(res), name);
- default:
- assert(!"support for resource type not implemented");
- return -1;
- }
+ idx = parse_program_resource_name(name, &out_base_name_end);
+ if (idx < 0)
+ return false;
+
+ if (array_index)
+ *array_index = idx;
+
+ return true;
}
/* Find a program resource with specific name in given
interface.
*/
struct gl_program_resource *
_mesa_program_resource_find_name(struct gl_shader_program *shProg,
- GLenum programInterface, const
char
*name)
+ GLenum programInterface, const
char
*name,
+ unsigned *array_index)
{
- GET_CURRENT_CONTEXT(ctx);
- const char *full_name = name;
-
- /* When context has 'VertexID_is_zero_based' set, gl_VertexID
has
been
- * lowered to gl_VertexIDMESA.
- */
- if (name && ctx->Const.VertexID_is_zero_based) {
- if (strcmp(name, "gl_VertexID") == 0)
- full_name = "gl_VertexIDMESA";
- }
-
struct gl_program_resource *res = shProg->ProgramResourceList;
for (unsigned i = 0; i < shProg->NumProgramResourceList; i++,
res++) {
if (res->Type != programInterface)
@@ -594,38 +592,46 @@ _mesa_program_resource_find_name(struct
gl_shader_program *shProg,
const char *rname = _mesa_program_resource_name(res);
unsigned baselen = strlen(rname);
- switch (programInterface) {
- case GL_TRANSFORM_FEEDBACK_VARYING:
- case GL_UNIFORM_BLOCK:
- case GL_UNIFORM:
- case GL_VERTEX_SUBROUTINE_UNIFORM:
- case GL_GEOMETRY_SUBROUTINE_UNIFORM:
- case GL_FRAGMENT_SUBROUTINE_UNIFORM:
- case GL_COMPUTE_SUBROUTINE_UNIFORM:
- case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
- case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
- case GL_VERTEX_SUBROUTINE:
- case GL_GEOMETRY_SUBROUTINE:
- case GL_FRAGMENT_SUBROUTINE:
- case GL_COMPUTE_SUBROUTINE:
- case GL_TESS_CONTROL_SUBROUTINE:
- case GL_TESS_EVALUATION_SUBROUTINE:
- if (strncmp(rname, name, baselen) == 0) {
+ if (strncmp(rname, name, baselen) == 0) {
+ switch (programInterface) {
+ case GL_UNIFORM_BLOCK:
/* Basename match, check if array or struct. */
if (name[baselen] == '\0' ||
name[baselen] == '[' ||
name[baselen] == '.') {
return res;
}
+ break;
+ case GL_TRANSFORM_FEEDBACK_VARYING:
+ case GL_UNIFORM:
+ case GL_VERTEX_SUBROUTINE_UNIFORM:
+ case GL_GEOMETRY_SUBROUTINE_UNIFORM:
+ case GL_FRAGMENT_SUBROUTINE_UNIFORM:
+ case GL_COMPUTE_SUBROUTINE_UNIFORM:
+ case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
+ case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
+ case GL_VERTEX_SUBROUTINE:
+ case GL_GEOMETRY_SUBROUTINE:
+ case GL_FRAGMENT_SUBROUTINE:
+ case GL_COMPUTE_SUBROUTINE:
+ case GL_TESS_CONTROL_SUBROUTINE:
+ case GL_TESS_EVALUATION_SUBROUTINE:
+ if (name[baselen] == '.') {
+ return res;
+ }
+ /* fall-through */
+ case GL_PROGRAM_INPUT:
+ case GL_PROGRAM_OUTPUT:
+ if (name[baselen] == '\0') {
+ return res;
+ } else if (name[baselen] == '[' &&
+ valid_array_index(name, array_index)) {
+ return res;
+ }
+ break;
+ default:
+ assert(!"not implemented for given interface");
}
- break;
- case GL_PROGRAM_INPUT:
- case GL_PROGRAM_OUTPUT:
- if (array_index_of_resource(res, full_name) >= 0)
- return res;
- break;
- default:
- assert(!"not implemented for given interface");
}
}
return NULL;
@@ -787,19 +793,9 @@ _mesa_get_program_resource_name(struct
gl_shader_program *shProg,
static GLint
program_resource_location(struct gl_shader_program *shProg,
- struct gl_program_resource *res, const
char *name)
+ struct gl_program_resource *res, const
char *name,
+ unsigned array_index)
{
- unsigned index, offset;
- int array_index = -1;
- long offset_ret;
- const GLchar *base_name_end;
-
- if (res->Type == GL_PROGRAM_INPUT || res->Type ==
GL_PROGRAM_OUTPUT) {
- array_index = array_index_of_resource(res, name);
- if (array_index < 0)
- return -1;
- }
-
/* Built-in locations should report GL_INVALID_INDEX. */
if (is_gl_identifier(name))
return GL_INVALID_INDEX;
@@ -810,13 +806,22 @@ program_resource_location(struct
gl_shader_program *shProg,
*/
switch (res->Type) {
case GL_PROGRAM_INPUT:
+ /* If the input is an array, fail if the index is out of
bounds. */
+ if (array_index > 0
+ && array_index >= RESOURCE_VAR(res)->type->length) {
+ return -1;
+ }
return RESOURCE_VAR(res)->data.location + array_index -
VERT_ATTRIB_GENERIC0;
case GL_PROGRAM_OUTPUT:
+ /* If the output is an array, fail if the index is out of
bounds. */
+ if (array_index > 0
+ && array_index >= RESOURCE_VAR(res)->type->length) {
+ return -1;
+ }
return RESOURCE_VAR(res)->data.location + array_index -
FRAG_RESULT_DATA0;
case GL_UNIFORM:
- index = _mesa_get_uniform_location(shProg, name, &offset);
-
- if (index == GL_INVALID_INDEX)
+ /* If the uniform is built-in, fail. */
+ if (RESOURCE_UNI(res)->builtin)
return -1;
/* From the GL_ARB_uniform_buffer_object spec:
@@ -830,17 +835,21 @@ program_resource_location(struct
gl_shader_program *shProg,
RESOURCE_UNI(res)->atomic_buffer_index != -1)
return -1;
- /* location in remap table + array element offset */
- return RESOURCE_UNI(res)->remap_location + offset;
-
+ /* fallthrough */
case GL_VERTEX_SUBROUTINE_UNIFORM:
case GL_GEOMETRY_SUBROUTINE_UNIFORM:
case GL_FRAGMENT_SUBROUTINE_UNIFORM:
case GL_COMPUTE_SUBROUTINE_UNIFORM:
case GL_TESS_CONTROL_SUBROUTINE_UNIFORM:
case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM:
- offset_ret = parse_program_resource_name(name,
&base_name_end);
- return RESOURCE_UNI(res)->remap_location + ((offset_ret !=
-1)
? offset_ret : 0);
+ /* If the uniform is an array, fail if the index is out of
bounds. */
+ if (array_index > 0
+ && array_index >= RESOURCE_UNI(res)->array_elements) {
+ return -1;
+ }
+
+ /* location in remap table + array element offset */
+ return RESOURCE_UNI(res)->remap_location + array_index;
default:
return -1;
}
@@ -854,14 +863,16 @@ GLint
_mesa_program_resource_location(struct gl_shader_program *shProg,
GLenum programInterface, const
char
*name)
{
+ unsigned array_index = 0;
struct gl_program_resource *res =
- _mesa_program_resource_find_name(shProg, programInterface,
name);
+ _mesa_program_resource_find_name(shProg, programInterface,
name,
+ &array_index);
/* Resource not found. */
if (!res)
return -1;
- return program_resource_location(shProg, res, name);
+ return program_resource_location(shProg, res, name,
array_index);
}
/**
@@ -873,7 +884,7 @@ _mesa_program_resource_location_index(struct
gl_shader_program *shProg,
GLenum programInterface,
const char *name)
{
struct gl_program_resource *res =
- _mesa_program_resource_find_name(shProg, programInterface,
name);
+ _mesa_program_resource_find_name(shProg, programInterface,
name, NULL);
/* Non-existent variable or resource is not referenced by
fragment stage. */
if (!res || !(res->StageReferences & (1 <<
MESA_SHADER_FRAGMENT)))
@@ -949,7 +960,8 @@ get_buffer_property(struct gl_shader_program
*shProg,
for (unsigned i = 0; i < RESOURCE_UBO(res)->NumUniforms;
i++) {
const char *iname =
RESOURCE_UBO(res)->Uniforms[i].IndexName;
struct gl_program_resource *uni =
- _mesa_program_resource_find_name(shProg, GL_UNIFORM,
iname);
+ _mesa_program_resource_find_name(shProg, GL_UNIFORM,
iname,
+ NULL);
if (!uni)
continue;
(*val)++;
@@ -959,7 +971,8 @@ get_buffer_property(struct gl_shader_program
*shProg,
for (unsigned i = 0; i < RESOURCE_UBO(res)->NumUniforms;
i++) {
const char *iname =
RESOURCE_UBO(res)->Uniforms[i].IndexName;
struct gl_program_resource *uni =
- _mesa_program_resource_find_name(shProg, GL_UNIFORM,
iname);
+ _mesa_program_resource_find_name(shProg, GL_UNIFORM,
iname,
+ NULL);
if (!uni)
continue;
*val++ =
@@ -1099,7 +1112,8 @@ _mesa_program_resource_prop(struct
gl_shader_program *shProg,
case GL_PROGRAM_INPUT:
case GL_PROGRAM_OUTPUT:
*val = program_resource_location(shProg, res,
- _mesa_program_resource_name(res));
+ _mesa_program_resource_name(res),
+ 0);
return 1;
default:
goto invalid_operation;
diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 0887c68..b702dcd 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -2229,7 +2229,7 @@ _mesa_GetSubroutineIndex(GLuint program,
GLenum
shadertype,
}
resource_type = _mesa_shader_stage_to_subroutine(stage);
- res = _mesa_program_resource_find_name(shProg, resource_type,
name);
+ res = _mesa_program_resource_find_name(shProg, resource_type,
name, NULL);
if (!res) {
_mesa_error(ctx, GL_INVALID_OPERATION, "%s", api_name);
return -1;
diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
index c081f74..0a10191 100644
--- a/src/mesa/main/shaderapi.h
+++ b/src/mesa/main/shaderapi.h
@@ -232,7 +232,8 @@ _mesa_program_resource_index(struct
gl_shader_program *shProg,
extern struct gl_program_resource *
_mesa_program_resource_find_name(struct gl_shader_program *shProg,
- GLenum programInterface, const
char
*name);
+ GLenum programInterface, const
char
*name,
+ unsigned *array_index);
extern struct gl_program_resource *
_mesa_program_resource_find_index(struct gl_shader_program
*shProg,
diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c
index 6ba746e..ff1df72 100644
--- a/src/mesa/main/uniforms.c
+++ b/src/mesa/main/uniforms.c
@@ -952,7 +952,7 @@ _mesa_GetUniformBlockIndex(GLuint program,
struct gl_program_resource *res =
_mesa_program_resource_find_name(shProg, GL_UNIFORM_BLOCK,
- uniformBlockName);
+ uniformBlockName, NULL);
if (!res)
return GL_INVALID_INDEX;
@@ -987,7 +987,8 @@ _mesa_GetUniformIndices(GLuint program,
for (i = 0; i < uniformCount; i++) {
struct gl_program_resource *res =
- _mesa_program_resource_find_name(shProg, GL_UNIFORM,
uniformNames[i]);
+ _mesa_program_resource_find_name(shProg, GL_UNIFORM,
uniformNames[i],
+ NULL);
uniformIndices[i] = _mesa_program_resource_index(shProg,
res);
}
}
_______________________________________________
mesa-dev mailing list
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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
