[Mesa-dev] [PATCH piglit v2] Test that glShaderSource does not change compile status.

Wed Apr 27 03:20:03 UTC 2016

On Tuesday, April 26, 2016 1:08:02 PM PDT Jamey Sharp wrote:
> OpenGL 4.5 Core Profile section 7.1, in the documentation for
> CompileShader, says: "Changing the source code of a shader object with
> ShaderSource does not change its compile status or the compiled shader
> code." (I haven't checked older versions of the spec.)
> 
> This test creates a shader, compiles it, changes its source, and links
> it. The spec requires rendering done with this shader to be consistent
> with the old source, not the new source, since the shader isn't compiled
> again after the source is changed.
> 
> According to Karol Herbst, the game "Divinity: Original Sin - Enhanced
> Edition" depends on this odd quirk of the spec. See:
> https://lists.freedesktop.org/archives/mesa-dev/2016-March/109789.html
> 
> This test fails against current Mesa master, but passes with a one-line
> patch to src/mesa/main/shaderapi.c. That patch, together with
> MESA_GL_VERSION_OVERRIDE=4.2, also allowed "Divinity" to start up
> successfully on i965, though rendering bugs remain.
> 
> Based on Karol's report, I expect this test should pass on Catalyst, but
> I only have Intel hardware to test on.
> 
> Signed-off-by: Jamey Sharp <jamey at minilop.net>
> Cc: Ian Romanick <idr at freedesktop.org>
> ---
> 
> "But Ian, I learned it from watching you!" Most of the things you
> complained about, I copied verbatim from your sso-simple.c. ;-)
> 
> I did notice I apparently introduced the first use of C99 array literals
> anywhere in Piglit, and I didn't really need to, so I re-wrote that. And
> after your suggestions, piglit_init was embarrasingly tiny, so I moved
> more setup code into it. I think it's a little more clear this way.
> 
> I think this test can perhaps be simplified. Can all the work be done in
> one shader, either just a vertex shader or just a fragment shader? Or
> does OpenGL require both shaders to be present? I didn't spot the answer
> while skimming the spec.

You could probably only use one shader, but that means getting legacy
fixed-function programming for one of the two stages.  It's usually a
lot simpler to just specify both, as you have - I wouldn't change it.

> 
>  tests/all.py                            |   1 +
>  tests/shaders/CMakeLists.gl.txt         |   1 +
>  tests/shaders/shadersource-no-compile.c | 102 +++++++++++++++++++++++++++++
+++
>  3 files changed, 104 insertions(+)
>  create mode 100644 tests/shaders/shadersource-no-compile.c
> 
> diff --git a/tests/all.py b/tests/all.py
> index 93d64e6..9f5c019 100644
> --- a/tests/all.py
> +++ b/tests/all.py
> @@ -572,6 +572,7 @@ with profile.group_manager(PiglitGLTest, 'shaders') as 
g:
>      g(['glsl-kwin-blur-2'])
>      g(['gpu_shader4_attribs'])
>      g(['link-unresolved-function'])
> +    g(['shadersource-no-compile'])
>      g(['sso-simple'])
>      g(['sso-uniforms-01'])
>      g(['sso-uniforms-02'])
> diff --git a/tests/shaders/CMakeLists.gl.txt b/tests/shaders/
CMakeLists.gl.txt
> index afbcc4b..2db2ded 100644
> --- a/tests/shaders/CMakeLists.gl.txt
> +++ b/tests/shaders/CMakeLists.gl.txt
> @@ -150,6 +150,7 @@ ENDIF (UNIX)
>  piglit_add_executable (glsl-kwin-blur-1 glsl-kwin-blur-1.c)
>  piglit_add_executable (glsl-kwin-blur-2 glsl-kwin-blur-2.c)
>  piglit_add_executable (link-unresolved-function link-unresolved-function.c)
> +piglit_add_executable (shadersource-no-compile shadersource-no-compile.c)
>  piglit_add_executable (sso-simple sso-simple.c)
>  piglit_add_executable (sso-uniforms-01 sso-uniforms-01.c)
>  piglit_add_executable (sso-uniforms-02 sso-uniforms-02.c)
> diff --git a/tests/shaders/shadersource-no-compile.c b/tests/shaders/
shadersource-no-compile.c
> new file mode 100644
> index 0000000..b5db55d
> --- /dev/null
> +++ b/tests/shaders/shadersource-no-compile.c
> @@ -0,0 +1,102 @@
> +/*
> + * Copyright © 2016 Jamey Sharp
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the 
"Software"),
> + * to deal in the Software without restriction, including without 
limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the 
next
> + * paragraph) shall be included in all copies or substantial portions of 
the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +/**
> + * \file shadersource-no-compile.c
> + * OpenGL 4.5 Core Profile section 7.1, in the documentation for 
CompileShader,
> + * says: "Changing the source code of a shader object with ShaderSource 
does not
> + * change its compile status or the compiled shader code."
> + *
> + * This test creates a shader, compiles it, changes its source, and links 
it.
> + * The spec requires rendering done with this shader to be consistent with 
the
> + * old source, not the new source, since the shader isn't compiled again 
after
> + * the source is changed.
> + *
> + * According to Karol Herbst, the game "Divinity: Original Sin - Enhanced
> + * Edition" depends on this odd quirk of the spec. See:
> + * https://lists.freedesktop.org/archives/mesa-dev/2016-March/109789.html
> + */
> +#include "piglit-util-gl.h"
> +
> +PIGLIT_GL_TEST_CONFIG_BEGIN
> +
> +	config.supports_gl_compat_version = 20;
> +
> +	config.window_visual = PIGLIT_GL_VISUAL_RGB | PIGLIT_GL_VISUAL_DOUBLE;
> +
> +PIGLIT_GL_TEST_CONFIG_END
> +
> +static const char vs_text[] =
> +	"void main() { gl_Position = gl_Vertex; "
> +	"gl_FrontColor = vec4(0.0, 1.0, 0.0, 1.0); }";
> +
> +static const char good_fs_text[] =
> +	"void main() { gl_FragColor = gl_Color; }";
> +
> +/* It is important that this shader *not* use gl_Color.
> + */
> +static const char bad_fs_text[] =
> +	"void main() { gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0); }";

Why not just have both shaders draw constant colors - one blue, the
other green - and skip using gl_Color / glColor3fv?  It seems like that
would reproduce the issue, too, and is a bit simpler.

Either way, this looks good to me.  Thanks for testing and fixing this!

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

> +
> +enum piglit_result
> +piglit_display(void)
> +{
> +	static const float green[3] = { 0.0, 1.0, 0.0 };
> +	static const float blue[3]  = { 0.0, 0.0, 1.0 };
> +
> +	glClear(GL_COLOR_BUFFER_BIT);
> +
> +	/* good_fs_text uses the color from the vertex shader, which is green.
> +	 * bad_fs_text uses a constant red color. Both are distinct from the
> +	 * color set here, and from the clear-color.
> +	 */
> +	glColor3fv(blue);
> +
> +	piglit_draw_rect(-1, -1, 2, 2);
> +	if (!piglit_probe_pixel_rgb(15, 15, green))
> +		return PIGLIT_FAIL;
> +
> +	return PIGLIT_PASS;
> +}
> +
> +void
> +piglit_init(int argc, char **argv)
> +{
> +	GLuint vs = piglit_compile_shader_text(GL_VERTEX_SHADER, vs_text);
> +	GLuint fs = piglit_compile_shader_text(GL_FRAGMENT_SHADER, good_fs_text);
> +	const GLchar *bad_fs_texts[] = { bad_fs_text };
> +	GLuint prog;
> +
> +	/* Change the shader source, but don't recompile it before linking. */
> +	glShaderSource(fs, 1, bad_fs_texts, NULL);
> +	prog = piglit_link_simple_program(vs, fs);
> +	if (!prog)
> +		piglit_report_result(PIGLIT_FAIL);
> +
> +	glDeleteShader(vs);
> +	glDeleteShader(fs);
> +
> +	glUseProgram(prog);
> +
> +	glClearColor(0.3, 0.3, 0.3, 0.0);
> +}
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160426/7a5db308/attachment.sig>