|Summary:||Linking fails when not writing gl_Position.|
|Component:||glsl-compiler||Assignee:||Tapani Pälli <lemody>|
|Status:||RESOLVED FIXED||QA Contact:||Intel 3D Bugs Mailing List <intel-3d-bugs>|
|i915 platform:||i915 features:|
Description kalyank 2014-09-02 03:13:46 UTC
Created attachment 105583 [details] Fix. From GLSL3.0 Spec(section 7.1) "The variable gl_Position is intended for writing the homogeneous vertex position. It can be written at any time during shader execution. This value will be used by primitive assembly, clipping, culling, and other fixed functionality operations, if present, that operate on primitives after vertex processing has occurred. Its value is undefined after the vertex processing stage if the vertex shader executable does not write gl_Position." Failing to write to a gl_position is not an error and atleast the linking of shaders should pass. Mesa throws an linker error in case we dont write to a gl_position. GL & GLSL version: OpenGL ES 3.0 Mesa 10.3.0-devel (git-a1853ea) OpenGL ES GLSL ES 3.0 Use Case: The following WebGL conformance tests fail when running Chromium Web Browser with Wayland(https://github.com/01org/ozone-wayland) 1)https://github.com/KhronosGroup/WebGL/blob/master/sdk/tests/conformance/glsl/misc/empty_main.vert.html 2)https://github.com/KhronosGroup/WebGL/blob/master/sdk/tests/conformance/glsl/misc/gl_position_unset.vert.html Related discussion on khronos webgl mailing list: http://www.khronos.org/webgl/public-mailing-list/archives/1306/msg00042.html Possible fix: I have attached the patch which changes the error to warning.
Comment 1 Tapani Pälli 2014-09-02 07:52:16 UTC
Because test does not specify GLSL version, I believe lowest possible one is used and that is why the error condition gets hit, currently test could pass if it specified #version 140 at the beginning. But it looks like Mesa is indeed too strict here, GLSL specification 1.10 states: "Compilers may generate a diagnostic message if they detect gl_Position is not written, or read before being written, but not all such cases are detectable. Results are undefined if a vertex shader is executed and does not write gl_Position." So for me it looks like Kalyan's fix is proper, I can send your patch to the list with a change to the comment section as well.
Comment 2 Tapani Pälli 2014-09-02 08:06:15 UTC
I might have to take back comment #1 though. In the discussion section of GLSL 1.10 spec there is still mention that writing to gl_Position is mandatory. --- 8< --- 82) Is it really necessary to require writing of gl_Position, gl_FragColor or gl_FragDepth? DISCUSSION: This can be difficult to handle error cases for when the writes are conditional. There was also discussion that either kill should be called, or a gl_ output be written in a fragment shader. However, that is irrelevant, as it’s okay to neither kill nor write any outputs in a fragment shader. For the vertex shader, it still makes no sense to not write a position, so this should still be required. RESOLUTION: For the fragment shader, there are no rules; either kill can be called or not, and if not, nothing need be written to, existing values are picked up from the pipeline. For the vertex shader, gl_Position should still be written, with the compiler giving a diagnostic when possible. --- 8< --- If the test uses ES 3.0, why does it not use GLSL ES 3.0 in the shaders? Test should have line "#version 300 es" to utilize correct version.
Comment 3 Tapani Pälli 2014-09-02 11:40:03 UTC
This shader fails to link also with Nvidia binary driver, maybe the correct solution would be to fix this test instead.
Comment 4 Ian Romanick 2014-09-02 19:52:48 UTC
I don't have the spec handy, but ES 1.00 shaders require that gl_Position be written. ES 3.00 lifts this restriction due to the addition of transform feedback.
Comment 5 Tapani Pälli 2014-09-03 12:10:58 UTC
Huh I did not realize that desktop and ES specs treat this subject in a slightly different way. What I can tell from GLSL ES spec is that it is not mandatory to write gl_Position (?) At least by these paragraphs: --- 8< --- GLSL ES spec 1.0.17 says in 7.1 Vertex Shader Special Variables section: "The value of gl_Position is undefined if a vertex shader is executed and does not write gl_Position." AND 10.13 gl_Position "Is it an error if the vertex shader doesn't write to gl_Position? Whether a shader writes to gl_Position cannot always be determined e.g. if there is dependence on an attribute. Option 1: No it is not an error. The behavior is undefined in this case. Development systems should issue a warning in this case but the on-target compiler should not have to detect this. RESOLUTION: No error (option 1). The nature of the undefined behavior must be specified." --- 8< ---
Comment 6 kalyank 2014-09-03 13:52:12 UTC
Thanks for the feedback Ian and Tapani. Ya, Spec ES 1.0 spec (1, 2) Section 7.1 says "The variable gl_Position is available only in the vertex language and is intended for writing the homogeneous vertex position. All executions of a well-formed vertex shader should write a value into this variable. It can be written at any time during shader execution. It may also be read back by the shader after being written. This value will be used by primitive assembly, clipping, culling, and other fixed functionality operations that operate on primitives after vertex processing has occurred. Compilers may generate a diagnostic message if they detect gl_Position is not written, or read before being written, but not all such cases are detectable. The value of gl_Position is undefined if a vertex shader is executed and does not write gl_Position. The variable gl_PointSize is available only in the vertex language and is intended for a vertex shader to write the size of the point to be rasterized. It is measured in pixels. " 1) http://www.khronos.org/files/opengles_shading_language.pdf 2)https://www.khronos.org/registry/gles/specs/2.0/GLSL_ES_Specification_1.0.14.pdf
Comment 7 kalyank 2014-09-03 15:21:39 UTC
Created attachment 105695 [details] [review] Patch v2 Take into account that GLSL 1.0 mandates writing to gl_Position.
Comment 8 Tapani Pälli 2014-09-09 07:42:00 UTC
Kalyan's fix was pushed to Mesa master, thanks!