Bug 106833 - glLinkProgram is expected to fail when vertex attribute aliasing happens on ES3.0 context or later
Summary: glLinkProgram is expected to fail when vertex attribute aliasing happens on E...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: mesa-dev
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-06 06:29 UTC by xinghua
Modified: 2018-10-31 13:02 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

Attachments
possible fix (6.14 KB, patch)
2018-06-06 11:23 UTC, Tapani Pälli 		Details | Splinter Review
possible fix v2 (2.90 KB, patch)
2018-08-10 10:47 UTC, Tapani Pälli 		Details | Splinter Review
attachment-12698-0.html (1.73 KB, text/html)
2018-08-10 10:48 UTC, Yang Gu 		Details
Show Obsolete (1) View All

Description xinghua 2018-06-06 06:29:58 UTC 
Steps:
1. Download chrome and install it on your Ubuntu, https://www.google.com/chrome/?platform=linux&extra=devchannel.
2. Open chrome on command line, "google-chrome-unstable --use-gl=egl".
3. Open the link, https://www.khronos.org/registry/webgl/sdk/tests/conformance2/attribs/gl-bindAttribLocation-aliasing-inactive.html?webglVersion=2&quiet=0
4. Some cases fail.

Notes:
These failed cases verify that glLinkProgram is expected to fail when binding two vertex inputs(attributes) to the same position.
You could read the spec to get more information(OpenGL ES GLSL 3.0, issue 12.46, P150).
I am not sure whether OpenGL GLSL 4.5 and OpenGL ES GLSL 2.0 allow this behavior. (It seems that OpenGL ES GLSL 2.0 allows this behavior, and does not report error when linking program)

One case may be as below,
the vertex shader：
#version 300 es
in mediump float a_1;
in mediump float a_2;
void main()
{
    gl_Position = vec4(0.0, 0.0, 0.0, 0.0);
}

API side calls:
var glProgram = gl.createProgram();
gl.bindAttribLocation(glProgram, l, 'a_1'); 
gl.bindAttribLocation(glProgram, l, 'a_2');  // bind a_2 the same location as a_1
gl.attachShader(glProgram, glVertexShader);
gl.attachShader(glProgram, glFragmentShader);
gl.linkProgram(glProgram);    // Expect failure here
Comment 1 Tapani Pälli 2018-06-06 08:30:55 UTC 
FYI I noticed this test prints out following before failing tests:

"Testing with shader that has entirely unused attributes".

IMO the issue here is that Mesa does not assign a location to entirely unused attributes (their location will be -1) and therefore checks for overlapping/aliasing locations are not done. I've proven this by skipping opt_dead_code for certain variables in the test and that makes it pass as we run the appropriate check in linker.

I'm not sure if this testing overlapping locations of unused attributes makes actually sense or how to fix the situation.
Comment 2 xinghua 2018-06-06 08:57:30 UTC 
(In reply to Tapani Pälli from comment #1)
> FYI I noticed this test prints out following before failing tests:
> 
> "Testing with shader that has entirely unused attributes".
> 
> IMO the issue here is that Mesa does not assign a location to entirely
> unused attributes (their location will be -1) and therefore checks for
> overlapping/aliasing locations are not done. I've proven this by skipping
> opt_dead_code for certain variables in the test and that makes it pass as we
> run the appropriate check in linker.
> 
> I'm not sure if this testing overlapping locations of unused attributes
> makes actually sense or how to fix the situation.

The  original target of these cases are surely testing with shader that has entirely unused attributes. These cases are used to verify whether GLES implementation follows the spec, and spec defines that "The existence of aliasing is determined by declarations present after preprocessing", "not depend on compiler optimizations which might be implementation-dependent".

I do also know nothing why spec defines aliasing like above. Does OpenGL 4.5 defines aliasing the same as OpenGL ES 3.0?(I had not found related information in OpenGL spec) And Do other OpenGL ES 3.0 implementations(intel or non-intel) of attribute aliasing also depend on compiler optimizations?
Thank you.
Comment 3 Tapani Pälli 2018-06-06 10:41:41 UTC 
(In reply to xinghua from comment #2)
> (In reply to Tapani Pälli from comment #1)
> > FYI I noticed this test prints out following before failing tests:
> > 
> > "Testing with shader that has entirely unused attributes".
> > 
> > IMO the issue here is that Mesa does not assign a location to entirely
> > unused attributes (their location will be -1) and therefore checks for
> > overlapping/aliasing locations are not done. I've proven this by skipping
> > opt_dead_code for certain variables in the test and that makes it pass as we
> > run the appropriate check in linker.
> > 
> > I'm not sure if this testing overlapping locations of unused attributes
> > makes actually sense or how to fix the situation.
> 
> The  original target of these cases are surely testing with shader that has
> entirely unused attributes. These cases are used to verify whether GLES
> implementation follows the spec, and spec defines that "The existence of
> aliasing is determined by declarations present after preprocessing", "not
> depend on compiler optimizations which might be implementation-dependent".

One thing making this problematic is that there are 3 ways to assign location. Either explicitly in the shader, using glBindAttribLocation or automatic assignment by the linker. This test calls glBindAttribLocation, so we don't know the location aliasing after preprocessing, we will know it only at linking phase.

However it should be possible to fix this since removing those dead variables and assigning locations both happen in the linker, however it does not look very straightforward :/ We do remove dead code before assigning locations since that makes a lot of sense.
 
> I do also know nothing why spec defines aliasing like above. Does OpenGL 4.5
> defines aliasing the same as OpenGL ES 3.0?(I had not found related
> information in OpenGL spec) And Do other OpenGL ES 3.0 implementations(intel
> or non-intel) of attribute aliasing also depend on compiler optimizations?
> Thank you.

Location aliasing is permitted by desktop GL so ES 3.0 is being different here. I don't now how other implementations do this or if they pass the test.
Comment 4 Tapani Pälli 2018-06-06 11:23:07 UTC 
Created attachment 140052 [details] [review]
possible fix

This should fix the issue but will investigate if there is a cleaner way.
Comment 5 Tapani Pälli 2018-06-06 15:02:26 UTC 
(In reply to Tapani Pälli from comment #4)
> Created attachment 140052 [details] [review] [review]
> possible fix
> 
> This should fix the issue but will investigate if there is a cleaner way.

*Warning, this fixes the issue but introduces problems.*
Comment 6 Tapani Pälli 2018-08-10 10:47:14 UTC 
Created attachment 141034 [details] [review]
possible fix v2

Here's a fixed version .. it's not nice though but we should do something along these lines, perhaps have a separate lighter weight pass.
Comment 7 Yang Gu 2018-08-10 10:48:14 UTC 
Created attachment 141035 [details]
attachment-12698-0.html

Yang is OOO from Aug 10 to 19. Please expect slow response.
Comment 8 Mark Janes 2018-09-21 23:01:56 UTC 
Tapani, is there a piglit test for this?
Comment 9 Tapani Pälli 2018-09-25 04:37:42 UTC 
(In reply to Mark Janes from comment #8)
> Tapani, is there a piglit test for this?

Nope but I can make a test, will send also the fix to mesa-dev unless I come up with something better.
Comment 10 Tapani Pälli 2018-10-31 13:02:17 UTC 
Fixed by following commit. Note that since the rule applies to ES only, this fixes the conformance when running chrome with '--use-gl=egl'. Let me know if this is not sufficient from Chrome or WebGL perspective. It could be the Angle needs to be fixed when running on desktop GL.

--- 8< ---
commit 27f1298b9d95899c4061294e384fadfd1c0a1b44
Author: Tapani Pälli <tapani.palli@intel.com>
Date:   Wed Jun 6 14:19:16 2018 +0300

    glsl/linker: validate attribute aliasing before optimizations
    
    Patch does a 'dry run' of assign_attribute_or_color_locations before
    optimizations to catch cases where we have aliasing of unused attributes
    which is forbidden by the GLSL ES 3.x specifications.
    
    We need to run this pass before unused attributes may be removed and with
    attribute binding information from program, therefore we re-use existing
    pass in linker rather than attempt to write another one.
    
    This fixes WebGL2 test 'gl-bindAttribLocation-aliasing-inactive' and
    Piglit test 'gles-3.0-attribute-aliasing'.
    
    Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106833
    Reviewed-by: Timothy Arceri <tarceri@itsqueeze.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.