On 10/04/2018 07:08 AM, asimiklit.w...@gmail.com wrote: > From: Andrii Simiklit <andrii.simik...@globallogic.com> > > GLSL 3.7 (Identifiers): > "However, as noted in the specification, there are some cases where > previously declared variables can be redeclared to change or add some > property, and predeclared "gl_" names are allowed to be redeclared in a > shader only for these specific purposes. More generally, it is an error to > redeclare a variable, including those starting "gl_"." > > This patch should fix piglit tests: > 'clip-distance-redeclare-without-inout.frag' > 'clip-distance-redeclare-without-inout.vert' > and leads to regression in clip-distance-out-values.shader_test > but probably a fix should be made in the test.
I believe clip-distance-out-values.shader_test is incorrect. The redeclaration of gl_ClipDistance should have "out". glslang rejects it with: ERROR: 0:5: 'redeclaration' : cannot change qualification of gl_ClipDistance ERROR: 1 compilation errors. No code generated. I think Mark and Clayton would prefer it if the fix to the test landed first. That way the test never shows up as "fail" to them. I'll send the fix for this with the new test cases that I mention below. > As far as I understood following mailing thread: > https://lists.freedesktop.org/archives/piglit/2013-October/007935.html > looks like we have accepted to remove an ability to change qualifiers > but have not done it yet. Unless I missed something) > > Fixes: 8e6cb9fe51a2 "glsl: Refactor AST-to-HIR code handling variable > redeclarations" While this is technically correct, I don't think we want to add a new compile error to the stable branches. On the outside chance this breaks some application, I'd rather have it sit on master for a bit first. > Signed-off-by: Andrii Simiklit <andrii.simik...@globallogic.com> > --- > src/compiler/glsl/ast_to_hir.cpp | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/src/compiler/glsl/ast_to_hir.cpp > b/src/compiler/glsl/ast_to_hir.cpp > index 93e7c8ec33..e26ae6b92a 100644 > --- a/src/compiler/glsl/ast_to_hir.cpp > +++ b/src/compiler/glsl/ast_to_hir.cpp > @@ -4240,10 +4240,15 @@ get_variable_being_redeclared(ir_variable **var_ptr, > YYLTYPE loc, > */ > if (earlier->type->is_unsized_array() && var->type->is_array() > && (var->type->fields.array == earlier->type->fields.array)) { > - /* FINISHME: This doesn't match the qualifiers on the two > - * FINISHME: declarations. It's not 100% clear whether this is > - * FINISHME: required or not. > - */ > + > + if ((strcmp("gl_ClipDistance", var->name) == 0) && Hmm... this fixes the specific case mentioned in the old e-mail thread, but this could occur with any variable. For example, I don't think this shader should compile, but it does: #version 110 varying float x[]; uniform float x[3]; uniform int i; void main() { gl_Position = vec4(x[i]); } Much to my surprise, glslang does not reject this shader. It looks like glslang rejects anything that tries to change the storage qualifier on any built-in variable, but it seems to allow such changes on user-defined variables. I think that's a bug in glslang. Let's do this for now. 1. Change Mesa to reject any storage qualifier change on a built-in variable. This will match glslang. We can do this by bringing the code by the comment "Allow verbatim redeclarations of built-in variables. Not explicitly valid, but some applications do it." up before the big if-then-else tree. Around line 4235 add something like: if (earlier->data.how_declared == ir_var_declared_implicitly) { /* Allow verbatim redeclarations of built-in variables. Not explicitly * valid, but some applications do it. */ if (earlier->data.mode != var->data.mode && !(earlier->data.mode == ir_var_system_value && var->data.mode == ir_var_shader_in)) { _mesa_glsl_error(&loc, state, "redeclaration cannot change qualification of `%s'", var->name); } } I changed the error message to be more similar to glslang. I like that wording, but I'm flexible. We'll also want to remove the other tests for this (which will be redundant after the above change). 2. Add a bunch of piglit tests to make sure we match glslang. I've got a start on this, and I'll CC you on the patches when I send them. 3. I'll submit a GLSL spec bug to make sure cases like my example above are intended to be illegal. 4. Depending on the outcome of #3, update Mesa to match. > + earlier->data.mode != var->data.mode) { > + _mesa_glsl_error(&loc, state, > + "redeclaration of '%s %s' with incorrect > qualifiers '%s'.", > + mode_string(earlier), > + var->name, > + mode_string(var)); > + } > > const int size = var->type->array_size(); > check_builtin_array_max_size(var->name, size, loc, state); _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
