From: Iago Toral Quiroga Date: Tue, 18 Oct 2016 12:15:36 +0000 (+0200) Subject: glsl: Indirect array indexing on non-last SSBO member must fail compilation X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=3da08e166415a745139c1127040a24e8a45dc553;p=mesa.git glsl: Indirect array indexing on non-last SSBO member must fail compilation After the changes in comit 5b2675093e863a52, we moved this check to the linker, but the spec expects this to be checked at compile-time. There are dEQP tests that expect an error at compile time and the spec seems to confirm that expectation: "Except for the last declared member of a shader storage block (section 4.3.9 “Interface Blocks”), the size of an array must be declared (explicitly sized) before it is indexed with anything other than an integral constant expression. The size of any array must be declared before passing it as an argument to a function. Violation of any of these rules result in compile-time errors. It is legal to declare an array without a size (unsized) and then later redeclare the same name as an array of the same type and specify a size, or index it only with integral constant expressions (implicitly sized)." Commit 5b2675093e863a52 tries to take care of the case where we have implicitly sized arrays in SSBOs and it does so by checking the max_array_access field in ir_variable during linking. In this patch we change the approach: we look for indirect access on SSBO arrays, and when we find one, we emit a compile-time error if the accessed member is not the last in the SSBO definition. There is a corner case that the specs do not address directly though and that dEQP checks for: the case of an unsized array in an SSBO definition that is not defined last but is never used in the shader code either. The following dEQP tests expect a compile-time error in this scenario: dEQP-GLES31.functional.debug.negative_coverage.callbacks.shader.compile_compute_shader dEQP-GLES31.functional.debug.negative_coverage.get_error.shader.compile_compute_shader dEQP-GLES31.functional.debug.negative_coverage.log.shader.compile_compute_shader However, since the unsized array is never used it is never indexed with a non-constant expression, so by the spec quotation above, it should be valid and the tests are probably incorrect. Reviewed-by: Nicolai Hähnle --- diff --git a/src/compiler/glsl/ast_array_index.cpp b/src/compiler/glsl/ast_array_index.cpp index e29dafb7907..dfa44b703d4 100644 --- a/src/compiler/glsl/ast_array_index.cpp +++ b/src/compiler/glsl/ast_array_index.cpp @@ -233,6 +233,20 @@ _mesa_ast_array_index_to_hir(void *mem_ctx, else if (array->variable_referenced()->data.mode != ir_var_shader_storage) { _mesa_glsl_error(&loc, state, "unsized array index must be constant"); + } else { + /* Unsized array non-constant indexing on SSBO is allowed only for + * the last member of the SSBO definition. + */ + ir_variable *var = array->variable_referenced(); + const glsl_type *iface_type = var->get_interface_type(); + int field_index = iface_type->field_index(var->name); + /* Field index can be < 0 for instance arrays */ + if (field_index >= 0 && + field_index != (int) iface_type->length - 1) { + _mesa_glsl_error(&loc, state, "Indirect access on unsized " + "array is limited to the last member of " + "SSBO."); + } } } else if (array->type->without_array()->is_interface() && ((array->variable_referenced()->data.mode == ir_var_uniform diff --git a/src/compiler/glsl/link_uniform_blocks.cpp b/src/compiler/glsl/link_uniform_blocks.cpp index 5b0dff6aa19..bb423c55410 100644 --- a/src/compiler/glsl/link_uniform_blocks.cpp +++ b/src/compiler/glsl/link_uniform_blocks.cpp @@ -150,13 +150,7 @@ private: */ const glsl_type *type_for_size = type; if (type->is_unsized_array()) { - if (!last_field) { - linker_error(prog, "unsized array `%s' definition: " - "only last member of a shader storage block " - "can be defined as unsized array", - name); - } - + assert(last_field); type_for_size = type->without_array(); }