Created attachment 40723 [details] new test case for piglit Bug detailed description: -------------------------- This is a negative test. The attached shader is expected to fail to compile, but now it gets segfault. This is a recent regression. GLSL spec 1.20 section 5.7 says: The equality operators and assignment operator are only allowed if the two operands are same size and type... Both array operands must be explicitly sized. Reproduce steps: ---------------- glslparsertest glsl-array-compare-unsized.vert fail Current result: ---------------- Segmentation fault #0 ast_selection_statement::hir (this=0x968180, instructions=0xe94f70, state=<value optimized out>) at ast_to_hir.cpp:2816 #1 0x00007ffff64d17df in ast_compound_statement::hir (this=0x968220, instructions=0xe94f70, state=0x9625d0) at ast_to_hir.cpp:1668 #2 0x00007ffff64d1912 in ast_function_definition::hir (this=0x9682c0, instructions=<value optimized out>, state=0x9625d0) at ast_to_hir.cpp:2665 #3 0x00007ffff64d2807 in _mesa_ast_to_hir (instructions=0x962b10, state=0x9625d0) at ast_to_hir.cpp:85 #4 0x00007ffff64c9bef in _mesa_glsl_compile_shader (ctx=0x6264a0, shader=0x9623d0) at program/ir_to_mesa.cpp:2959 #5 0x000000000040353e in test () at /home/gordon/piglit/tests/glslparsertest/glslparsertest.c:108 #6 0x00000000004039cd in main (argc=3, argv=0x7fffffffe2b8) at /home/gordon/piglit/tests/glslparsertest/glslparsertest.c:224 Expected result: ---------------- Failed to compile vertex shader glsl-array-compare-unsized.vert: 0:8(10): error: operands of `==' must have the same type PIGLIT: {'result': 'pass' } Rootcause: ---------------- It's caused by below mesa patch. Array size needs checking before comparison. Branch: master Commit: ff79633d9f930e396933a0ad9564824ec73ea4dc URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=ff79633d9f930e396933a0ad9564824ec73ea4dc Author: Eric Anholt <eric@anholt.net> Date: Tue Nov 30 11:23:28 2010 -0800 glsl: Fix structure and array comparisions. We were trying to emit a single ir_expression to compare the whole thing. The backends (ir_to_mesa.cpp and brw_fs.cpp so far) expected ir_binop_any_nequal or ir_binop_all_equal to apply to at most a vector (with matrices broken down by the lowering pass). Break them down to a bunch of ORed or ANDed any_nequals/all_equals. Fixes: glsl-array-compare glsl-array-compare-02 glsl-fs-struct-equal glsl-fs-struct-notequal Bug #31909 --- src/glsl/ast_to_hir.cpp | 72 +++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp index fd9ed55..0978100 100644 --- a/src/glsl/ast_to_hir.cpp +++ b/src/glsl/ast_to_hir.cpp @@ -745,6 +745,75 @@ ast_node::hir(exec_list *instructions, return NULL; } +static ir_rvalue * +do_comparison(void *mem_ctx, int operation, ir_rvalue *op0, ir_rvalue *op1) +{ + int join_op; + + if (operation == ir_binop_all_equal) + join_op = ir_binop_logic_and; + else + join_op = ir_binop_logic_or; + + switch (op0->type->base_type) { + case GLSL_TYPE_FLOAT: + case GLSL_TYPE_UINT: + case GLSL_TYPE_INT: + case GLSL_TYPE_BOOL: + return new(mem_ctx) ir_expression(operation, op0, op1); + + case GLSL_TYPE_ARRAY: { + ir_rvalue *last = NULL; + + for (unsigned int i = 0; i < op0->type->length; i++) { + ir_rvalue *e0, *e1, *result; + + e0 = new(mem_ctx) ir_dereference_array(op0->clone(mem_ctx, NULL), + new(mem_ctx) ir_constant(i)); + e1 = new(mem_ctx) ir_dereference_array(op1->clone(mem_ctx, NULL), + new(mem_ctx) ir_constant(i)); + result = do_comparison(mem_ctx, operation, e0, e1); + + if (last) { + last = new(mem_ctx) ir_expression(join_op, last, result); + } else { + last = result; + } + } + return last; + }
the test case also shows another issue: the driver considers "b[i] = i" (while b is unsized array) illegal: 0:7(5): error: unsized array index must be constant. I don't see this defined in spec. I can file a separate bug for this if you want.
(In reply to comment #1) > the test case also shows another issue: the driver considers "b[i] = i" (while > b is unsized array) illegal: > 0:7(5): error: unsized array index must be constant. > > I don't see this defined in spec. > > I can file a separate bug for this if you want. That is illegal: Section 4.1.9 (page 21 of the spec, page 27 of the PDF) says: "If an array is indexed with an expression that is not an integral constant expression or passed as an argument to a function, then its size must be declared before any such use."
I have added several piglit tests, array-22.vert and array-compare-0[1234].vert, to reproduce these issues.
(In reply to comment #2) > (In reply to comment #1) > > the test case also shows another issue: the driver considers "b[i] = i" (while > > b is unsized array) illegal: > > 0:7(5): error: unsized array index must be constant. > > > > I don't see this defined in spec. > > > > I can file a separate bug for this if you want. > > That is illegal: > > Section 4.1.9 (page 21 of the spec, page 27 of the PDF) says: > > "If an array is indexed with an expression that is not an integral constant > expression or passed as an argument to a function, then its size must be > declared before any such use." Ian, you are right. I'm attached an updated version.
Created attachment 40817 [details] new test case for piglit
(In reply to comment #3) > I have added several piglit tests, array-22.vert and > array-compare-0[1234].vert, to reproduce these issues. I don't see that in piglit.
(In reply to comment #6) > (In reply to comment #3) > > I have added several piglit tests, array-22.vert and > > array-compare-0[1234].vert, to reproduce these issues. > > I don't see that in piglit. I forgot to push my piglit changes. Sorry about that. These tests should be fixed by: commit 6d36be508ff0765beb6cf6bb95a323ff01e458dd Author: Ian Romanick <ian.d.romanick@intel.com> Date: Thu Dec 2 12:17:36 2010 -0800 glsl: Ensure that equality comparisons don't return a NULL IR tree This fixes bugzilla #32035 and piglit test case array-compare-01 and array-compare-02. NOTE: This is a candidate for the 7.9 branch.
catch a typo in array-22.vert: s/b/a in "These assignments will implicitly size b."
not marking 7.9 bugs as P1
It looks like this bug was left open until the fix was cherry-picked to 7.9. Ian's commit listed here is now on the 7.9 and 7.10 branches. Does it fix the issue? If so, this can be closed.
agree this can be fixed.
verified on master and 7.10.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.