[Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

[Timothy Arceri]

On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:
> 
> On 10/26/2016 08:15 AM, Timothy Arceri wrote:
> > 
> > On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:
> > > 
> > > SSO shader programs can be later modified by attaching/detaching
> > > shaders and relinked, this requires IR.
> > 
> > Doesn't relinking recreate the IR? We can relink exiting shaders
> > into
> > new programs. The IR is cloned from gl_shader (the compiled IR)
> > before
> > this happens.
> > 
> > Where exactly are things falling over for SSO?
> 
> I went this way as I haven't seen this happening elsewhere but when 
> relinking program created by glCreateShaderProgram. TBH I'm not sure
> if 
> this could happen with regular programs, I would assume we had
> already 
> bugs if it would be so (?)
> 
> In practice things go bad in brw_link_shader where process_glsl_ir() 
> gets called, like this:
> 
> --- 8< ---
> #0  do_expression_flattening (instructions=instructions at entry=0x0, 
> predicate=predicate at entry=0x7ffff0f808a0 
> <mat_op_to_vec_predicate(ir_instruction*)>) at 
> glsl/ir_expression_flattening.cpp:60
> #1  0x00007ffff0f816b9 in do_mat_op_to_vec (instructions=0x0) at 
> glsl/lower_mat_op_to_vec.cpp:96
> #2  0x00007ffff10385bd in process_glsl_ir (shader_prog=0x9b74d8, 
> shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
> #3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
> brw_link.cpp:234
> #4  0x00007ffff0ec1f31 in _mesa_glsl_link_shader (ctx=0x7d2478, 
> prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
> #5  0x00007ffff0ddc99b in _mesa_link_program (ctx=0x7d2478, 
> shProg=0x9b74d8) at main/shaderapi.c:1098
> #6  0x00007ffff7ac8d15 in stub_glLinkProgram (program=2) at 
> /home/tpalli/source/fdo/piglit/tests/util/piglit-dispatch-gen.c:33005
> #7  0x00000000004019ab in 
> relink_program_created_by_glCreateShaderProgram () at 
> /home/tpalli/source/fdo/piglit/tests/spec/arb_separate_shader_objects
> /api-errors.c:78

There is a comment above that line in the test.

/* Issue #14 of the GL_ARB_separate_shader_objects spec says:
*
 *     "14. Should glLinkProgram work to re-link a shader created with
 *          glCreateShaderProgram?
 *
 *          RESOLVED: NO because the shader created by
 *          glCreateShaderProgram is detached and deleted as part of
 *          the glCreateShaderProgram sequence.  This means if you
 *          call glLinkProgram on a program returned from
 *          glCreateShaderProgram, you'll find the re-link fails
 *          because no shader object is attached.
 *
 *          An application is free to attach one or more new shader
 *          objects to the program and then relink would work.
 *
 *          This is fine because re-linking isn't necessary/expected."
 */

Which means we shouldn't able to relink this shader. We shouldn't be
able to get as far along as we do when we get the error message.

> 
> 
> > 
> > > 
> > >  This patch fixes regression
> > > caused by 4542c7ed5fc6d8cb2495d322b4f06d802d7292cc.
> > > 
> > > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97715
> > > Cc: "12.0 13.0" <mesa-stable at lists.freedesktop.org>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_link.cpp | 17 ++++++++++-------
> > >  1 file changed, 10 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
> > > b/src/mesa/drivers/dri/i965/brw_link.cpp
> > > index 5ea9773..ffb66a9 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_link.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
> > > @@ -290,14 +290,17 @@ brw_link_shader(struct gl_context *ctx,
> > > struct
> > > gl_shader_program *shProg)
> > > 
> > >     build_program_resource_list(ctx, shProg);
> > > 
> > > -   for (stage = 0; stage < ARRAY_SIZE(shProg->_LinkedShaders);
> > > stage++) {
> > > -      struct gl_linked_shader *shader = shProg-
> > > > 
> > > > _LinkedShaders[stage];
> > > -      if (!shader)
> > > -         continue;
> > > +   /* We can't free IR for SSO programs since those may need
> > > relinking. */
> > > +   if (!shProg->SeparateShader) {
> > > +      for (stage = 0; stage < ARRAY_SIZE(shProg-
> > > >_LinkedShaders);
> > > stage++) {
> > > +         struct gl_linked_shader *shader = shProg-
> > > > 
> > > > _LinkedShaders[stage];
> > > +         if (!shader)
> > > +            continue;
> > > 
> > > -      /* The GLSL IR won't be needed anymore. */
> > > -      ralloc_free(shader->ir);
> > > -      shader->ir = NULL;
> > > +         /* The GLSL IR won't be needed anymore. */
> > > +         ralloc_free(shader->ir);
> > > +         shader->ir = NULL;
> > > +      }
> > >     }
> > > 
> > >     return true;
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev