[Mesa-dev] [PATCH 3/7] glsl: Forbid opaque variables as operands of the ternary operator.

[Ian Romanick]

On 02/01/2015 01:15 PM, Francisco Jerez wrote:
> Ian Romanick <idr at freedesktop.org> writes:
> 
>> On 01/31/2015 09:54 PM, Francisco Jerez wrote:
>>> ---
>>>  src/glsl/ast_to_hir.cpp | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>>> index 1ba29f7..783384e 100644
>>> --- a/src/glsl/ast_to_hir.cpp
>>> +++ b/src/glsl/ast_to_hir.cpp
>>> @@ -1596,6 +1596,18 @@ ast_expression::do_hir(exec_list *instructions,
>>>           error_emitted = true;
>>>        }
>>>  
>>> +      /* From section 4.1.7 of the GLSL 4.50 spec (Opaque Types):
>>> +       *
>>> +       *  "Except for array indexing, structure member selection, and
>>> +       *   parentheses, opaque variables are not allowed to be operands in
>>> +       *   expressions; such use results in a compile-time error."
>>> +       */
>>> +      if (type->contains_opaque()) {
>>> +         _mesa_glsl_error(&loc, state, "opaque variables cannot be operands "
>>> +                          "of the ?: operator");
>>> +         error_emitted = true;
>>> +      }
>>> +
>>
>> This is what the spec says, but I'm suspicious that other vendors may
>> also allow it.  Can we poke at AMD and NVIDIA close-source drivers to
>> see if they allow things like
>>
>>     x = texture(b ? sampler1 : sampler2, t);
>>
>> If they allow it, I'll submit a spec bug.  If they do not allow it, we
>> shouldn't either.
>>
> Yes, they do [1].  But I don't think it would be a good idea to change
> the spec.  For images it would be fine actually (as long as the
> condition is dynamically uniform) because they're implemented as a
> simple 32-bit integer index.  But for samplers the hell would break
> loose because most back-ends expect to be able to determine the sampler
> binding statically (up to a dynamically uniform array index if they do
> gpu_shader5), so the fact is that it's already broken and will lead to a
> crash even if we pretend to allow it.

Well...

    x = texture(b ? sampler1 : sampler2, t);

is really the same as

    x = (b) ? texture(sampler1, t) : texture(sampler2, t);

so it seems reasonable for a compiler to "just make it work."  The
problem with having everyone follow the spec is there's probably some
application that expects the non-conformant behavior.  We get enough of
those bug reports. :)

Although, if we currently crash on this sort of input, we're already
worse than generating an unexpected error for the application.

> [1] http://people.freedesktop.org/~currojerez/piglit-summary-image_load_store-kepler/results-all/spec/ARB_shader_image_load_store/compiler/expression-selection.frag.html
> 
>>>        ir_constant *cond_val = op[0]->constant_expression_value();
>>>        ir_constant *then_val = op[1]->constant_expression_value();
>>>        ir_constant *else_val = op[2]->constant_expression_value();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150201/b86f7d4f/attachment-0001.sig>