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

[Timothy Arceri]

On Sat, 2016-04-30 at 12:22 -0700, 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.htm
> l
> 
> 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>
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> ---
> 
> v3: Simplify the shaders per Ken's suggestions.
> 
> I think at this point, this patch has reviewed-by from both Ian and
> Ken
> and I'm happy with it. Could one of you merge it, and the
> corresponding
> Mesa patch? Thanks!

I've pushed both patches thanks for working on this.

Note in future you only need to send piglit tests to piglit at lists.freed
esktop.org and mesa patches to mesa-dev at lists.freedesktop.org there is
no need to send everything to both lists :)

> 
> Oh, and as Ian noticed, I forgot to mention that the Mesa patch
> fixes:
> https://bugs.freedesktop.org/show_bug.cgi?id=93551
> 
>  tests/all.py                            |  1 +
>  tests/shaders/CMakeLists.gl.txt         |  1 +
>  tests/shaders/shadersource-no-compile.c | 98
> +++++++++++++++++++++++++++++++++
>  3 files changed, 100 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..8a2264f
> --- /dev/null
> +++ b/tests/shaders/shadersource-no-compile.c
> @@ -0,0 +1,98 @@
> +/*
> + * 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; }";
> +
> +/* good_fs_text uses a constant green color, while bad_fs_text uses
> a
> + * constant red color, so that we can tell which version of the
> fragment
> + * shader got executed. Both are distinct from the clear-color so we
> can
> + * tell if the shader ran at all.
> + */
> +
> +static const char good_fs_text[] =
> +	"void main() { gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0); }";
> +
> +static const char bad_fs_text[] =
> +	"void main() { gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0); }";
> +
> +enum piglit_result
> +piglit_display(void)
> +{
> +	static const float green[3] = { 0.0, 1.0, 0.0 };
> +
> +	glClear(GL_COLOR_BUFFER_BIT);
> +
> +	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);
> +}