[Mesa-dev] [PATCH] glsl: handle unsigned int wraparound in link_shaders()

[Timothy Arceri]

On Thu, 2016-04-07 at 11:22 -0400, Lars Hamre wrote:
> NOTES:
> * Reposting due to no response last week
> * Someone with access will need to commit this patch after the review
>   process
> 
> check_explicit_uniform_locations() returns a -1 when the extension
> ARB_explicit_uniform_location is not found.
> 
> We were storing the result in num_explicit_uniform_locs as an
> unsigned int which caused it to be 4294967295 when a -1 was returned.
> 
> This in turn would cause the following error during linking:
> error: count of uniform locations > MAX_UNIFORM_LOCATIONS(4294967295
> > 98304)
> 
> Here are the results from running piglit tests/all with this patch:
> changes:     178
> fixes:       176
> regressions: 2
> 
> The two regressions are for the following tests:
> glean at glsl1-matrix column check (1)
> glean at glsl1-matrix column check (2)
> which regress from FAIL to CRASH.
> 
> I am okay with these regressions because the tests are currently
> failing due to the aforementioned linker error.
> 
> Signed-off-by: Lars Hamre <chemecse at gmail.com>
> 
> ---
>  src/compiler/glsl/linker.cpp | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/compiler/glsl/linker.cpp
> b/src/compiler/glsl/linker.cpp
> index cd35464..a0a9104 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -4171,7 +4171,7 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
> 
>     tfeedback_decl *tfeedback_decls = NULL;
>     unsigned num_tfeedback_decls = prog-
> >TransformFeedback.NumVarying;
> -   unsigned int num_explicit_uniform_locs = 0;
> +   int num_explicit_uniform_locs = 0;
> 
>     void *mem_ctx = ralloc_context(NULL); // temporary linker context
> 
> @@ -4354,6 +4354,8 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
>     }
> 
>     num_explicit_uniform_locs = check_explicit_uniform_locations(ctx,
> prog);
> +   if (num_explicit_uniform_locs < 0)
> +      num_explicit_uniform_locs = 0;

Thanks for spotting this. However the just hacks around the problem.

Since we never do anything with the -1 I would rather
see check_explicit_uniform_locations() changed to return an unsigned 0
rather than -1.


>     link_assign_subroutine_types(prog);
> 
>     if (!prog->LinkStatus)
> @@ -4585,7 +4587,7 @@ link_shaders(struct gl_context *ctx, struct
> gl_shader_program *prog)
> 
>     update_array_sizes(prog);
>     link_assign_uniform_locations(prog, ctx-
> >Const.UniformBooleanTrue,
> -                                 num_explicit_uniform_locs,
> +                                 (unsigned
> int)num_explicit_uniform_locs,
>                                   ctx-
> >Const.MaxUserAssignableUniformLocations);
>     link_assign_atomic_counter_resources(ctx, prog);
>     store_fragdepth_layout(prog);
> --
> 2.5.5
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev