[Mesa-dev] [PATCH] glsl: prevent qualifiers modification of predeclared variables
andrey simiklit
asimiklit.work at gmail.com
Tue Oct 9 13:52:17 UTC 2018
Hello,
Please find my comments below:
On Sat, Oct 6, 2018 at 1:07 AM Ian Romanick <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 wrote:
> >> From: Andrii Simiklit <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>
> >> ---
> >> 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 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?
>
> > 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
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
>
Regards,
Andrii.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181009/23583208/attachment.html>
More information about the mesa-dev
mailing list