[Mesa-dev] [PATCH] glShaderSource must not change compile status.

[Timothy Arceri]

On Tue, 2016-04-26 at 13:49 +0200, Ian Romanick wrote:
> On 04/26/2016 12:57 PM, Timothy Arceri wrote:
> > 
> > On Tue, 2016-04-26 at 09:29 +0200, Ian Romanick wrote:
> > > 
> > > On 04/26/2016 07:06 AM, 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.)
> > > > 
> > > > 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/1097
> > > > 89.h
> > > > tml
> > > > 
> > > > This patch, together with MESA_GL_VERSION_OVERRIDE=4.2, allows
> > > > "Divinity" to start up successfully on i965, though rendering
> > > > bugs
> > > > remain.
> > > > 
> > > > Signed-off-by: Jamey Sharp <jamey at minilop.net>
> > > > Cc: Karol Herbst <nouveau at karolherbst.de>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93551
> > > 
> > > I think this is correct, but this might have some interactions
> > > with
> > > Timothy's shader cache work.
> > It should be fine I don't think I'm using this flag for anything.
> I was thinking that there might be an interaction between the flag
> being
> GL_TRUE but the gl_shader::Source pointing to a different source
> string.

We use the source string to create a sha but that is done when the
shader is compiled so this change shouldn't impact anything.

However now that I'm thinking about it. I really should be doing
something to guard against the source changing as it is used when
falling back to compile after a cache miss and the source could well
have changed but not have been compiled by the user. In this case we
want to compile the old code not the new code.

> 
> > 
> > > 
> > > Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> > > 
> > > > 
> > > > 
> > > > ---
> > > >  src/mesa/main/shaderapi.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/src/mesa/main/shaderapi.c
> > > > b/src/mesa/main/shaderapi.c
> > > > index b28b5ce..fc2e885 100644
> > > > --- a/src/mesa/main/shaderapi.c
> > > > +++ b/src/mesa/main/shaderapi.c
> > > > @@ -949,7 +949,6 @@ shader_source(struct gl_shader *sh, const
> > > > GLchar *source)
> > > >     /* free old shader source string and install new one */
> > > >     free((void *)sh->Source);
> > > >     sh->Source = source;
> > > > -   sh->CompileStatus = GL_FALSE;
> > > >  #ifdef DEBUG
> > > >     sh->SourceChecksum = _mesa_str_checksum(sh->Source);
> > > >  #endif
> > > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev