[Mesa-dev] [PATCH v2 23/28] glsl: validate output types for shader stages

[Tapani Pälli]

On 02/08/2015 06:54 AM, Ilia Mirkin wrote:
> On Fri, Feb 6, 2015 at 4:15 AM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 02/06/2015 06:56 AM, Ilia Mirkin wrote:
>>> From: Tapani Pälli <tapani.palli at intel.com>
>>>
>>> Patch fixes Piglit test:
>>>     arb_gpu_shader_fp64/preprocessor/fs-output-double.frag
>>>
>>> and adds additional validation for shader outputs.
>>>
>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>>> ---
>>>   src/glsl/ast_to_hir.cpp | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 47 insertions(+)
>>>
>>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>>> index 3ea3a13..2a74a51 100644
>>> --- a/src/glsl/ast_to_hir.cpp
>>> +++ b/src/glsl/ast_to_hir.cpp
>>> @@ -3625,6 +3625,53 @@ ast_declarator_list::hir(exec_list *instructions,
>>>
>>>               handle_geometry_shader_input_decl(state, loc, var);
>>>            }
>>> +      } else if (var->data.mode == ir_var_shader_out) {
>>> +         const glsl_type *check_type = var->type;
>>> +         while (check_type->is_array())
>>> +            check_type = check_type->element_type();
>>
>>        const glsl_type *const = var->type->without_array();
>
> Right.
>
>>
>>> +
>>> +         /* From section 4.3.6 (Output variables) of the GLSL 4.40 spec:
>>> +          *
>>> +          *     It is a compile-time error to declare a vertex, tessellation
>>> +          *     evaluation, tessellation control, or geometry shader output
>>> +          *     that contains any of the following:
>>> +          *
>>> +          *     * A Boolean type (bool, bvec2 ...)
>>> +          *     * An opaque type
>>> +          */
>>> +         if (check_type->is_boolean() || check_type->contains_opaque())
>>> +            _mesa_glsl_error(&loc, state,
>>> +                             "%s shader output cannot have type %s",
>>> +                             _mesa_shader_stage_to_string(state->stage),
>>> +                             check_type->name);
>>
>> Aren't we already enforcing this somewhere?  Do we have any test cases?
>
> http://cgit.freedesktop.org/~tpohjola/mesa/commit/?h=fp64_common&id=2556b37be981156dc91d51849e96f69e52467a5e
>
> Tapani?

Yep, I see now that there's output type validation done in 
'apply_type_qualifier_to_variable' and we have a test case called 
varying-bool-prohibited.vert|frag for testing with booleans that passes. 
New test 'fs-output-double.frag' did not pass though so that's why I 
wrote output validation in 'ast_declarator_list::hir' (there'a also 
input type validation there). Will need to investigate why test would 
not pass, maybe better to take away this patch from the series for now.


>>
>>> +
>>> +         /* From section 4.3.6 (Output variables) of the GLSL 4.40 spec:
>>> +          *
>>> +          *     It is a compile-time error to declare a fragment shader output
>>> +          *     that contains any of the following:
>>> +          *
>>> +          *     * A Boolean type (bool, bvec2 ...)
>>> +          *     * A double-precision scalar or vector (double, dvec2 ...)
>>> +          *     * An opaque type
>>> +          *     * Any matrix type
>>> +          *     * A structure
>>> +          */
>>> +         if (state->stage == MESA_SHADER_FRAGMENT) {
>>> +            if (check_type->is_record() || check_type->is_matrix())
>>> +               _mesa_glsl_error(&loc, state,
>>> +                                "fragment shader output "
>>> +                                "cannot have struct or array type");
>>> +            switch (check_type->base_type) {
>>> +               case GLSL_TYPE_UINT:
>>> +               case GLSL_TYPE_INT:
>>> +               case GLSL_TYPE_FLOAT:
>>> +               break;
>>> +               default:
>>> +                  _mesa_glsl_error(&loc, state,
>>> +                                "fragment shader output cannot have "
>>> +                                "type %s", check_type->name);
>>> +            }
>>> +         }
>>>         }
>>>
>>>         /* Integer fragment inputs must be qualified with 'flat'.  In GLSL ES,
>>>
>>