[Mesa-dev] [PATCH V3 1/2] glsl: don't attempt to link empty program

[Timothy Arceri]

On Tue, 2016-01-26 at 13:33 +1100, Timothy Arceri wrote:
> On Mon, 2016-01-25 at 16:58 -0800, Ian Romanick wrote:
> > On 01/25/2016 04:46 PM, Timothy Arceri wrote:
> > > Previously an empty program would go through the entire
> > > link_shaders() function and we would have to be careful
> > > not to cause a segfault.
> > > 
> > > In core profile also now set link_status to false by
> > > generating an error, it was previously set to true.
> > > 
> > > > From Section 7.3 (PROGRAM OBJECTS) of the OpenGL 4.5 spec:
> > > 
> > >    "Linking can fail for a variety of reasons as specified in the
> > >    OpenGL Shading Language Specification, as well as any of the
> > >    following reasons:
> > > 
> > >     - No shader objects are attached to program."
> > > 
> > > V2: Only generate an error in core profile and add spec quote
> > > (Ian)
> > > 
> > > V3: generate error in ES too, remove previous check which was
> > > only
> > > applying the rule to GL 4.5/ES 3.1 and above. My understand is
> > > that
> > > this spec change is clarifying previously undefined behaviour and
> > > therefore should be applied retrospectively. The ES CTS tests for
> > > this are in ES 2 I suspect it was passing because it would have
> > > generated an error for not having both a vertex and fragment
> > > shader.
> > > 
> > > Cc: Ian Romanick <idr at freedesktop.org>
> > > Cc: Tapani Pälli <tapani.palli at intel.com>
> > > Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> > > ---
> > >  src/glsl/linker.cpp | 43 ++++++++++++++++++++++-----------------
> > > --
> > > --
> > >  1 file changed, 22 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > > index 6657777..79039a0 100644
> > > --- a/src/glsl/linker.cpp
> > > +++ b/src/glsl/linker.cpp
> > > @@ -4105,14 +4105,34 @@
> > > disable_varying_optimizations_for_sso(struct gl_shader_program
> > > *prog)
> > >  void
> > >  link_shaders(struct gl_context *ctx, struct gl_shader_program
> > > *prog)
> > >  {
> > > +   prog->Validated = false;
> > > +   prog->_Used = false;
> > > +
> > > +   /* Section 7.3 (Program Objects) of the OpenGL 4.5 Core
> > > Profile
> > > spec says:
> > > +    *
> > > +    *     "Linking can fail for a variety of reasons as
> > > specified
> > > in the
> > > +    *     OpenGL Shading Language Specification, as well as any
> > > of
> > > the
> > > +    *     following reasons:
> > > +    *
> > > +    *     - No shader objects are attached to program."
> > > +    *
> > > +    * The Compatibility Profile specification does not list the
> > > error.  In
> > > +    * Compatibility Profile missing shader stages are replaced
> > > by
> > > +    * fixed-function.  This applies to the case where all stages
> > > are
> > > +    * missing.
> > > +    */
> > > +   if (prog->NumShaders == 0) {
> > > +      if (ctx->API != API_OPENGL_COMPAT)
> > > +         linker_error(prog, "no shaders attached to the
> > > program\n");
> > > +      return;
> > 
> > Does glsl-link-empty-prog-02 still pass?  Since prog->LinkStatus is
> > false, I think that will cause problems.  Won't glUseProgram
> > generate
> > GL_INVALID_OPERATION?
> 
> It does pass, seems something sets the link flag to true before we
> get
> here. Maybe the fallback code?
> 
> Anyway it makes sense to also move the prog->LinkStatus = true; to
> the
> top so I've done that locally.

Are you happy with the patch with this change? There are 3 piglit
changes to go with this. The last one is an updated SSO test you sent
out 2 years ago. I've reviewed and pushed all the others from that 10
patch series this is the only one outstanding. 

http://patchwork.freedesktop.org/patch/71604/
http://patchwork.freedesktop.org/patch/71605/
http://patchwork.freedesktop.org/patch/71706/




> 
> 
> > 
> > > +   }
> > > +
> > >     tfeedback_decl *tfeedback_decls = NULL;
> > >     unsigned num_tfeedback_decls = prog-
> > > > TransformFeedback.NumVarying;
> > >  
> > >     void *mem_ctx = ralloc_context(NULL); // temporary linker
> > > context
> > >  
> > >     prog->LinkStatus = true; /* All error paths will set this to
> > > false */
> > > -   prog->Validated = false;
> > > -   prog->_Used = false;
> > >  
> > >     prog->ARB_fragment_coord_conventions_enable = false;
> > >  
> > > @@ -4162,25 +4182,6 @@ link_shaders(struct gl_context *ctx,
> > > struct
> > > gl_shader_program *prog)
> > >     prog->Version = max_version;
> > >     prog->IsES = is_es_prog;
> > >  
> > > -   /* From OpenGL 4.5 Core specification (7.3 Program Objects):
> > > -    *     "Linking can fail for a variety of reasons as
> > > specified
> > > in the OpenGL
> > > -    *     Shading Language Specification, as well as any of the
> > > following
> > > -    *     reasons:
> > > -    *
> > > -    *     * No shader objects are attached to program.
> > > -    *
> > > -    *     ..."
> > > -    *
> > > -    *     Same rule applies for OpenGL ES >= 3.1.
> > > -    */
> > > -
> > > -   if (prog->NumShaders == 0 &&
> > > -       ((ctx->API == API_OPENGL_CORE && ctx->Version >= 45) ||
> > > -        (ctx->API == API_OPENGLES2 && ctx->Version >= 31))) {
> > > -      linker_error(prog, "No shader objects are attached to
> > > program.\n");
> > > -      goto done;
> > > -   }
> > > -
> > >     /* Some shaders have to be linked with some other shaders
> > > present.
> > >      */
> > >     if (num_shaders[MESA_SHADER_GEOMETRY] > 0 &&
> > > 
> > 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev