[Mesa-dev] [PATCH] glsl: prevent qualifiers modification of predeclared variables
Ian Romanick
idr at freedesktop.org
Tue Oct 9 22:05:30 UTC 2018
On 10/09/2018 06:52 AM, andrey simiklit wrote:
> Hello,
>
> Please find my comments below:
>
> On Sat, Oct 6, 2018 at 1:07 AM Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
>
> On 10/05/2018 03:02 PM, Ian Romanick wrote:
> > On 10/04/2018 07:08 AM, asimiklit.work at gmail.com
> <mailto:asimiklit.work at gmail.com> wrote:
> >> From: Andrii Simiklit <andrii.simiklit at globallogic.com
> <mailto:andrii.simiklit at 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.simiklit at globallogic.com
> <mailto:andrii.simiklit at 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);
>
> Oops...
>
> } else if (earlier->type != var->type) {
> _mesa_glsl_error(&loc, state,
> "redeclaration of `%s' has incorrect type",
> var->name);
>
> > }
> > }
>
>
> I have tried to test this version on Intel CI.
>
> https://gitlab.freedesktop.org/asimiklit/mesa/commit/9d4f6a97568e7e2720e43601697eaf90908b7aab
>
> but it caused many errors:
>
> https://mesa-ci.01.org/global_logic/builds/16/group/63a9f0ea7bb98050796b649e85481845
>
> so I modified it in the following way:
>
> - } else if (earlier->type != var->type) {
> + } else if (earlier->type->without_array() != var->type->without_array()) {
>
> https://gitlab.freedesktop.org/asimiklit/mesa/commit/7f313c10640a72e37069f54baadaae1bc3d5a0a3
>
> and it helped a bit:
>
> https://mesa-ci.01.org/global_logic/builds/17/group/63a9f0ea7bb98050796b649e85481845
I poked at this a little bit too. The earlier->type vs. var->type check
can't be moved before the if-statements. The problem occurs when
earlier->type is an implicitly sized array (like gl_ClipDistance or
gl_TexCoord) and var->type explicitly sizes the array. The types won't
(and should not) match.
I believe your modified test would allow shaders that resize built-in
arrays that have a fixed size (e.g., gl_FragData).
> I have been investigating the remaining errors.
> Some part of the errors refer to 'error: redeclaration of
> `gl_LastFragData' with incorrect qualifiers'
> (ex. https://mesa-ci.01.org/global_logic/builds/17/results/23218604 )
> so probably we have to do some workaround for this variable
> because it is added as 'ir_var_shader_out' but then
> it is expected as 'ir_var_auto' in 'get_variable_being_redeclared' function.
>
> I think about something like:
> + const bool qualifiersChangeIsRequired =
> + (state->has_framebuffer_fetch() &&
> + !state->is_version(130, 300) &&
> + var->type->is_array() &&
> + strcmp(var->name, "gl_LastFragData") == 0);
> 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)) {
> + var->data.mode == ir_var_shader_in) &&
> + !qualifiersChangeIsRequired) {
> _mesa_glsl_error(&loc, state,
> "redeclaration of `%s' with incorrect
> qualifiers",
> var->name);
>
> What do you think about this workaround?
This basically moves an existing check earlier in the function. I think
this is starting to turn into the old woman who swallowed a fly. We're
adding a little more fix on to each fix until all that's left is chaos.
The fundamental problem is that my suggestion to move the type check
earlier was just wrong.
I simplified the fix a bit, and I added a couple refactors on top. I
ran this through the CI, and it looks good.
https://cgit.freedesktop.org/~idr/mesa/log/?h=ast_to_hir-work
> > 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 at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
>
> Regards,
> Andrii.
More information about the mesa-dev
mailing list