[Mesa-dev] [PATCH 1/3 v2] glsl: prevent qualifiers modification of predeclared variables
andrey simiklit
asimiklit.work at gmail.com
Tue Nov 13 10:21:46 UTC 2018
Hello,
Thanks a lot for review.
Regards,
Andrii.
On Sat, Nov 10, 2018 at 5:38 AM Timothy Arceri <tarceri at itsqueeze.com>
wrote:
> Nice! Series is:
>
> Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
>
> On 10/10/18 9:07 am, Ian Romanick wrote:
> > From: Ian Romanick <ian.d.romanick at intel.com>
> >
> > Section 3.7 (Identifiers) of the GLSL spec says:
> >
> > 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
> >
> > However, this causes a regression in
> > clip-distance-out-values.shader_test. A fix for that test has been sent
> > to the piglit list for review:
> >
> > https://patchwork.freedesktop.org/patch/255201/
> >
> > 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)
> >
> > v2 (idr): Move 'earlier->data.mode != var->data.mode' test much earlier
> > in the function. Add special handling for gl_LastFragData.
> >
> > Signed-off-by: Andrii Simiklit <andrii.simiklit at globallogic.com>
> > Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> > ---
> > src/compiler/glsl/ast_to_hir.cpp | 51
> +++++++++++++++++++++-------------------
> > 1 file changed, 27 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> > index 1082d6c91cf..2e4c9ef6776 100644
> > --- a/src/compiler/glsl/ast_to_hir.cpp
> > +++ b/src/compiler/glsl/ast_to_hir.cpp
> > @@ -4238,6 +4238,29 @@ get_variable_being_redeclared(ir_variable
> **var_ptr, YYLTYPE loc,
> >
> > *is_redeclaration = true;
> >
> > + if (earlier->data.how_declared == ir_var_declared_implicitly) {
> > + /* Verify that the redeclaration of a built-in does not change the
> > + * storage qualifier. There are a couple special cases.
> > + *
> > + * 1. Some built-in variables that are defined as 'in' in the
> > + * specification are implemented as system values. Allow
> > + * ir_var_system_value -> ir_var_shader_in.
> > + *
> > + * 2. gl_LastFragData is implemented as a ir_var_shader_out, but
> the
> > + * specification requires that redeclarations omit any
> qualifier.
> > + * Allow ir_var_shader_out -> ir_var_auto for this one
> variable.
> > + */
> > + if (earlier->data.mode != var->data.mode &&
> > + !(earlier->data.mode == ir_var_system_value &&
> > + var->data.mode == ir_var_shader_in) &&
> > + !(strcmp(var->name, "gl_LastFragData") == 0 &&
> > + var->data.mode == ir_var_auto)) {
> > + _mesa_glsl_error(&loc, state,
> > + "redeclaration cannot change qualification of
> `%s'",
> > + var->name);
> > + }
> > + }
> > +
> > /* From page 24 (page 30 of the PDF) of the GLSL 1.50 spec,
> > *
> > * "It is legal to declare an array without a size and then
> > @@ -4246,11 +4269,6 @@ 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.
> > - */
> > -
> > const int size = var->type->array_size();
> > check_builtin_array_max_size(var->name, size, loc, state);
> > if ((size > 0) && (size <= earlier->data.max_array_access)) {
> > @@ -4342,28 +4360,13 @@ get_variable_being_redeclared(ir_variable
> **var_ptr, YYLTYPE loc,
> > earlier->data.precision = var->data.precision;
> > earlier->data.memory_coherent = var->data.memory_coherent;
> >
> > - } else if (earlier->data.how_declared == ir_var_declared_implicitly
> &&
> > - state->allow_builtin_variable_redeclaration) {
> > + } else if ((earlier->data.how_declared == ir_var_declared_implicitly
> &&
> > + state->allow_builtin_variable_redeclaration) ||
> > + allow_all_redeclarations) {
> > /* 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 of `%s' with incorrect
> qualifiers",
> > - var->name);
> > - } else if (earlier->type != var->type) {
> > - _mesa_glsl_error(&loc, state,
> > - "redeclaration of `%s' has incorrect type",
> > - var->name);
> > - }
> > - } else if (allow_all_redeclarations) {
> > - if (earlier->data.mode != var->data.mode) {
> > - _mesa_glsl_error(&loc, state,
> > - "redeclaration of `%s' with incorrect
> qualifiers",
> > - var->name);
> > - } else if (earlier->type != var->type) {
> > + if (earlier->type != var->type) {
> > _mesa_glsl_error(&loc, state,
> > "redeclaration of `%s' has incorrect type",
> > var->name);
> >
> _______________________________________________
> 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/20181113/f1f0f1e1/attachment.html>
More information about the mesa-dev
mailing list