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

[Lars Hamre]

Agreed, I didn't see that check_explicit_uniform_locations() was
only used in link_shaders(). I will submit a v2 with those changes.

On Thu, Apr 7, 2016 at 11:24 PM, Timothy Arceri <
timothy.arceri at collabora.com> wrote:

> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160408/27c460f4/attachment.html>