Bug 102685

Summary: piglit.spec.glsl-1_50.compiler.vs-redeclares-pervertex-out-before-global-redeclaration
Product: Mesa Reporter: Mark Janes <mark.a.janes>
Component: Drivers/DRI/i965Assignee: Iago Toral <itoral>
Status: RESOLVED FIXED QA Contact: Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity: normal    
Priority: medium    
Version: git   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Mark Janes 2017-09-12 17:42:38 UTC 
The following parser tests regressed:
piglit.spec.glsl-1_50.compiler.vs-redeclares-pervertex-out-before-global-redeclaration_vert
piglit.spec.glsl-1_50.compiler.gs-redeclares-pervertex-out-before-global-redeclaration_geom

Beginning with:
51bf007d2c27fbad6dac590c72f0fc4860e3fa02
Author:     Iago Toral Quiroga <itoral@igalia.com>

glsl: Disallow unsized array of atomic_uint

This was a bugfix to the spec addressed in OpenGL 4.5 (revision
7 of the spec) and there is a CTS test to check this.

Fixes:
KHR-GL45.shader_atomic_counters.negative-unsized-array

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>


Based on the commit message, the piglit test may be incorrect.  If so, please change this bug to be against piglit and fix the associated tests.
Comment 1 Iago Toral 2017-09-13 08:04:16 UTC 
Hi Mark, sorry about this, I am not sure why I didn't see this before,  I was sure that I had sent this patch to CI before sending it for review :-/

Anyway, the problem is not with piglit, there is some rather unsafe code right prior to the place where this commit operated that can invalidate the memory of 'var' in one specific scenario (triggered by these parser tests) that makes any access to that variable from that point forward potentially unsafe: the variable ends up pointing to freed memory so you get undefined behavior, so the crash is not even guaranteed to happen. I think this is quite unfortunate, so I'll be sending a couple of patches, one to fix this regression and one or two more to make that implementation safer so we don't see more instances of this type of problems in the future.
Comment 2 Iago Toral 2017-09-13 12:22:40 UTC 
Sent a 3 patch series for this, the first patch fixes the regressions:
https://lists.freedesktop.org/archives/mesa-dev/2017-September/169441.html
Comment 3 Kenneth Graunke 2017-09-14 22:51:16 UTC 
Fixed by:

commit 4af156224ea8d5c21464f02aecc25f64de681170
Author: Iago Toral Quiroga <itoral@igalia.com>
Date:   Wed Sep 13 08:59:18 2017 +0200

    glsl: use 'declared_var' instead of 'var' after checking redeclarations
    
    Since the original 'var' might have been deleted from this point forward.
    
    Bugzila: https://bugs.freedesktop.org/show_bug.cgi?id=102685
    Fixes: 51bf007d2c27fba (glsl: Disallow unsized array of atomic_uint)
    
    Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>

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.