[Mesa-dev] [PATCH 4/4] glsl: In later GLSL versions, sequence operator is cannot be a constant expression

[Ian Romanick]

On 10/08/2015 03:31 AM, Iago Toral wrote:
> On Wed, 2015-10-07 at 14:34 -0700, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> Fixes:
>>     ES3-CTS.shaders.negative.constant_sequence
>>
>>     spec/glsl-es-3.00/compiler/global-initializer/from-sequence.vert
>>     spec/glsl-es-3.00/compiler/global-initializer/from-sequence.frag
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>  src/glsl/ast_to_hir.cpp | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> index 6af0f80..c7444ed 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -3311,8 +3311,49 @@ process_initializer(ir_variable *var, ast_declaration *decl,
>>        if (new_rhs != NULL) {
>>           rhs = new_rhs;
>>  
>> +         /* Section 4.3.3 (Constant Expressions) of the GLSL ES 3.00.4 spec
>> +          * says:
>> +          *
>> +          *     "A constant expression is one of
>> +          *
>> +          *        ...
>> +          *
>> +          *        - an expression formed by an operator on operands that are
>> +          *          all constant expressions, including getting an element of
>> +          *          a constant array, or a field of a constant structure, or
>> +          *          components of a constant vector.  However, the sequence
>> +          *          operator ( , ) and the assignment operators ( =, +=, ...)
>> +          *          are not included in the operators that can create a
>> +          *          constant expression."
>> +          *
>> +          * Section 12.43 (Sequence operator and constant expressions) says:
>> +          *
>> +          *     "Should the following construct be allowed?
>> +          *
>> +          *         float a[2,3];
>> +          *
>> +          *     The expression within the brackets uses the sequence operator
>> +          *     (',') and returns the integer 3 so the construct is decl aring
>> +          *     a single-dimensional array of size 3.  In some languages, the
>> +          *     construct declares a two-dimensional array.  It would be
>> +          *     preferable to make this construct illegal to avoid confusion.
>> +          *
>> +          *     One possibility is to change the definition of the sequence
>> +          *     operator so that it does not return a constant- expression and
>> +          *     hence cannot be used to declare an array size.
>> +          *
>> +          *     RESOLUTION: The result of a sequence operator is not a
>> +          *     constant-expression."
>> +          *
>> +          * Section 4.3.3 (Constant Expressions) of the GLSL 4.30.9 spec
>> +          * contains language almost identical to the section 4.3.3 fomr the
>> +          * GLSL ES 3.00.4 spec.  This is a new limitation for these GLSL
>> +          * versions.
>> +          */
>>           ir_constant *constant_value = rhs->constant_expression_value();
>> -         if (!constant_value) {
>> +         if (!constant_value ||
>> +             (state->is_version(430, 300) &&
>> +              decl->initializer->has_sequence_subexpression())) {
>>              const char *const variable_mode =
>>                 (type->qualifier.flags.q.constant)
>>                 ? "const"
> 
> This only seems to handle the case of sequence operators used in
> declaration initializers. Maybe this is all we really care about in
> practice, but the spec says that in code such as:
> 
> float b = ... ;
> ...
> b = (a++, a > 0.0 ? 2.0 : 0.0);
> 
> the RHS in the assignment is not a constant expression, and this patch
> would not address that, right? I wonder if this distinction has any
> actual implications in practice though.

That's fine, but there is another case that is not caught.  There are
only a few places where a constant expression is required.  Some
initializers (global variables in GLSL ES, uniforms, const-decorated
global variabes, and const-decorated local variables before GLSL 4.20),
array sizes, and maybe one other thing... that I can't think of right now.

I sent a test,
spec/glsl-es-3.00/compiler/array-sized-by-sequence-in-parenthesis.vert,
for the array size case.  This patch does not cover that, but I have one
on the way.

> Iago