[Mesa-dev] [PATCH] glsl: handle unsigned int wraparound in link_shaders()
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@glsl1-matrix column check (1) glean@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 --- 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; 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glsl: handle unsigned int wraparound in link_shaders()
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@glsl1-matrix column check (1) glean@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 --- 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; 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: handle unsigned int wraparound in link_shaders()
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@glsl1-matrix column check (1) > glean@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 > > --- > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: handle unsigned int wraparound in link_shaders()
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.arc...@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@glsl1-matrix column check (1) > > glean@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 > > > > --- > > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
