[Mesa-dev] [PATCH 3/3] glsl: validate memory qualifiers inside structures/interfaces

[Samuel Pitoiset]

On 05/03/2017 05:31 PM, Nicolai Hähnle wrote:
> On 01.05.2017 12:34, Samuel Pitoiset wrote:
>> This relaxes using memory qualifiers for buffer variables
>> because they are allowed.
>>
>> Fixes the following ARB_shader_image_load_store test:
>>
>> memory-qualifier-with-non-image-type.frag
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>  src/compiler/glsl/ast_to_hir.cpp | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp 
>> b/src/compiler/glsl/ast_to_hir.cpp
>> index a3f8a5a8c9..20a0f11755 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -3297,14 +3297,21 @@ validate_memory_qualifier_for_type(struct 
>> _mesa_glsl_parse_state *state,
>>                                     const struct ast_type_qualifier 
>> *qual,
>>                                     const glsl_type *type)
>>  {
>> -   if (!type->is_image()) {
>> +   /* From Section 4.10 (Memory Qualifiers) of the GLSL 4.50 spec:
>> +    *
>> +    * "Memory qualifiers are only supported in the declarations of image
>> +    *  variables, buffer variables, and shader storage blocks; it is 
>> an error
>> +    *  to use such qualifiers in any other declarations.
>> +    */
> 
> I know I R-b'ed the piglit change, but now I'm having second thoughts 
> about this, because the test actually uses a uniform block and not a 
> shader storage block.
> 
> Did you double-check the test with another implementation?

The commit message is a little confusing. This patch just checks that 
memory qualifiers are only used with image types, buffer variables and 
shader storage blocks. I will rewrite and send a v2.

No, I didn't check other drivers.

The Intel CI system is happy with this series, btw.

> 
> Cheers,
> Nicolai
> 
> 
>> +   if (!type->is_image() && !qual->flags.q.buffer) {
>>        if (qual->flags.q.read_only ||
>>            qual->flags.q.write_only ||
>>            qual->flags.q.coherent ||
>>            qual->flags.q._volatile ||
>>            qual->flags.q.restrict_flag) {
>>           _mesa_glsl_error(loc, state, "memory qualifiers may only be 
>> applied "
>> -                          "to images");
>> +                          "in the declarations of image variables, 
>> buffer "
>> +                          "variables, and shader storage blocks");
>>           return false;
>>        }
>>     }
>> @@ -6893,6 +6900,7 @@ 
>> ast_process_struct_or_iface_block_members(exec_list *instructions,
>>                            "to struct or interface block members");
>>        }
>>
>> +      validate_memory_qualifier_for_type(state, &loc, qual, decl_type);
>>        validate_image_format_qualifier_for_type(state, &loc, qual, 
>> decl_type);
>>
>>        /* From Section 4.4.2.3 (Geometry Outputs) of the GLSL 4.50 spec:
>>
> 
>