mesa: Refactor parameter validate for GetUniform, Uniform, and UniformMatrix

v2: Update a comment block about the different treatment of
location=-1 based on feedback from Ken.

Signed-off-by: Ian Romanick <[email protected]>
Tested-by: Tom Stellard <[email protected]>

1 files changed, 99 insertions, 52 deletions

diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c
index 041e0db2203..b68d63d7853 100644
--- a/src/mesa/main/uniforms.c
+++ b/src/mesa/main/uniforms.c
@@ -35,7 +35,7 @@
  * 2. Insert FLUSH_VERTICES calls in various places
  */
 
-
+#include <stdbool.h>
 #include "main/glheader.h"
 #include "main/context.h"
 #include "main/dispatch.h"
@@ -318,6 +318,90 @@ get_uniform_rows_cols(const struct gl_program_parameter *p,
    }
 }
 
+static bool
+validate_uniform_parameters(struct gl_context *ctx,
+			    struct gl_shader_program *shProg,
+			    GLint location, GLsizei count,
+			    unsigned *loc,
+			    unsigned *array_index,
+			    const char *caller,
+			    bool negative_one_is_not_valid)
+{
+   if (!shProg || !shProg->LinkStatus) {
+      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(program not linked)", caller);
+      return false;
+   }
+
+   if (location == -1) {
+      /* For glGetUniform, page 264 (page 278 of the PDF) of the OpenGL 2.1
+       * spec says:
+       *
+       *     "The error INVALID_OPERATION is generated if program has not been
+       *     linked successfully, or if location is not a valid location for
+       *     program."
+       *
+       * For glUniform, page 82 (page 96 of the PDF) of the OpenGL 2.1 spec
+       * says:
+       *
+       *     "If the value of location is -1, the Uniform* commands will
+       *     silently ignore the data passed in, and the current uniform
+       *     values will not be changed."
+       *
+       * Allowing -1 for the location parameter of glUniform allows
+       * applications to avoid error paths in the case that, for example, some
+       * uniform variable is removed by the compiler / linker after
+       * optimization.  In this case, the new value of the uniform is dropped
+       * on the floor.  For the case of glGetUniform, there is nothing
+       * sensible to do for a location of -1.
+       *
+       * The negative_one_is_not_valid flag selects between the two behaviors.
+       */
+      if (negative_one_is_not_valid) {
+	 _mesa_error(ctx, GL_INVALID_OPERATION, "%s(location=%d)",
+		     caller, location);
+      }
+
+      return false;
+   }
+
+   /* From page 12 (page 26 of the PDF) of the OpenGL 2.1 spec:
+    *
+    *     "If a negative number is provided where an argument of type sizei or
+    *     sizeiptr is specified, the error INVALID_VALUE is generated."
+    */
+   if (count < 0) {
+      _mesa_error(ctx, GL_INVALID_VALUE, "%s(count < 0)", caller);
+      return false;
+   }
+
+   /* Page 82 (page 96 of the PDF) of the OpenGL 2.1 spec says:
+    *
+    *     "If any of the following conditions occur, an INVALID_OPERATION
+    *     error is generated by the Uniform* commands, and no uniform values
+    *     are changed:
+    *
+    *     ...
+    *
+    *         - if no variable with a location of location exists in the
+    *           program object currently in use and location is not -1,
+    */
+   if (location < -1) {
+      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(location=%d)",
+                  caller, location);
+      return false;
+   }
+
+   _mesa_uniform_split_location_offset(location, loc, array_index);
+
+   if (*loc >= shProg->Uniforms->NumUniforms) {
+      _mesa_error(ctx, GL_INVALID_OPERATION, "%s(location=%d)",
+		  caller, location);
+      return false;
+   }
+
+   return true;
+}
+
 /**
  * Called via glGetUniform[fiui]v() to get the current value of a uniform.
  */
@@ -328,14 +412,14 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, GLint location,
    struct gl_shader_program *shProg =
       _mesa_lookup_shader_program_err(ctx, program, "glGetUniformfv");
    struct gl_program *prog;
-   GLint paramPos, offset;
+   GLint paramPos;
+   unsigned loc, offset;
 
-   if (!shProg)
+   if (!validate_uniform_parameters(ctx, shProg, location, 1,
+				    &loc, &offset, "glGetUniform", true))
       return;
 
-   _mesa_uniform_split_location_offset(location, &location, &offset);
-
-   if (!find_uniform_parameter_pos(shProg, location, &prog, &paramPos)) {
+   if (!find_uniform_parameter_pos(shProg, loc, &prog, &paramPos)) {
       _mesa_error(ctx, GL_INVALID_OPERATION,  "glGetUniformfv(location)");
    }
    else {
@@ -744,41 +828,20 @@ _mesa_uniform(struct gl_context *ctx, struct gl_shader_program *shProg,
               const GLvoid *values, GLenum type)
 {
    struct gl_uniform *uniform;
-   GLint elems, offset;
+   GLint elems;
+   unsigned loc, offset;
 
    ASSERT_OUTSIDE_BEGIN_END(ctx);
 
-   if (!shProg || !shProg->LinkStatus) {
-      _mesa_error(ctx, GL_INVALID_OPERATION, "glUniform(program not linked)");
-      return;
-   }
-
-   if (location == -1)
-      return;   /* The standard specifies this as a no-op */
-
-   if (location < -1) {
-      _mesa_error(ctx, GL_INVALID_OPERATION, "glUniform(location=%d)",
-                  location);
-      return;
-   }
-
-   _mesa_uniform_split_location_offset(location, &location, &offset);
-
-   if (location < 0 || location >= (GLint) shProg->Uniforms->NumUniforms) {
-      _mesa_error(ctx, GL_INVALID_VALUE, "glUniform(location=%d)", location);
+   if (!validate_uniform_parameters(ctx, shProg, location, count,
+				    &loc, &offset, "glUniform", false))
       return;
-   }
-
-   if (count < 0) {
-      _mesa_error(ctx, GL_INVALID_VALUE, "glUniform(count < 0)");
-      return;
-   }
 
    elems = _mesa_sizeof_glsl_type(type);
 
    FLUSH_VERTICES(ctx, _NEW_PROGRAM_CONSTANTS);
 
-   uniform = &shProg->Uniforms->Uniforms[location];
+   uniform = &shProg->Uniforms->Uniforms[loc];
 
    if (ctx->Shader.Flags & GLSL_UNIFORMS) {
       const GLenum basicType = base_uniform_type(type);
@@ -925,30 +988,14 @@ _mesa_uniform_matrix(struct gl_context *ctx, struct gl_shader_program *shProg,
                      GLboolean transpose, const GLfloat *values)
 {
    struct gl_uniform *uniform;
-   GLint offset;
+   unsigned loc, offset;
 
    ASSERT_OUTSIDE_BEGIN_END(ctx);
 
-   if (!shProg || !shProg->LinkStatus) {
-      _mesa_error(ctx, GL_INVALID_OPERATION,
-         "glUniformMatrix(program not linked)");
+   if (!validate_uniform_parameters(ctx, shProg, location, count,
+				    &loc, &offset, "glUniformMatrix", false))
       return;
-   }
-
-   if (location == -1)
-      return;   /* The standard specifies this as a no-op */
 
-   if (location < -1) {
-      _mesa_error(ctx, GL_INVALID_OPERATION, "glUniformMatrix(location)");
-      return;
-   }
-
-   _mesa_uniform_split_location_offset(location, &location, &offset);
-
-   if (location < 0 || location >= (GLint) shProg->Uniforms->NumUniforms) {
-      _mesa_error(ctx, GL_INVALID_VALUE, "glUniformMatrix(location)");
-      return;
-   }
    if (values == NULL) {
       _mesa_error(ctx, GL_INVALID_VALUE, "glUniformMatrix");
       return;
@@ -956,7 +1003,7 @@ _mesa_uniform_matrix(struct gl_context *ctx, struct gl_shader_program *shProg,
 
    FLUSH_VERTICES(ctx, _NEW_PROGRAM_CONSTANTS);
 
-   uniform = &shProg->Uniforms->Uniforms[location];
+   uniform = &shProg->Uniforms->Uniforms[loc];
 
    if (shProg->_LinkedShaders[MESA_SHADER_VERTEX]) {
       /* convert uniform location to program parameter index */