[Mesa-dev] [PATCH v2 12/23] glsl: Validate vertex emission in geometry shaders.

[Iago Toral]

On Wed, 2014-06-18 at 13:38 -0700, Ian Romanick wrote:
> On 06/18/2014 02:51 AM, Iago Toral Quiroga wrote:
> > Check if non-zero streams are used. Fail to link if emitting to unsupported
> > streams or emitting to non-zero streams with output type other than GL_POINTS.
> > ---
> >  src/glsl/linker.cpp | 148 +++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 134 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > index 0b6a716..f8ff138 100644
> > --- a/src/glsl/linker.cpp
> > +++ b/src/glsl/linker.cpp
> > @@ -250,31 +250,100 @@ public:
> >     }
> >  };
> >  
> > -
> >  /**
> > - * Visitor that determines whether or not a shader uses ir_end_primitive.
> > + * Visitor that determines the highest stream id to which a (geometry) shader
> > + * emits vertices. It also checks whether End{Stream}Primitive is ever called.
> >   */
> > -class find_end_primitive_visitor : public ir_hierarchical_visitor {
> > +class find_emit_vertex_visitor : public ir_hierarchical_visitor {
> >  public:
> > -   find_end_primitive_visitor()
> > -      : found(false)
> > +   find_emit_vertex_visitor(int max_allowed)
> > +      : max_stream_allowed(max_allowed),
> > +        invalid_stream_id(0),
> > +        invalid_stream_id_from_emit_vertex(false),
> > +        end_primitive_found(false),
> > +        uses_non_zero_stream(false)
> >     {
> >        /* empty */
> >     }
> >  
> > -   virtual ir_visitor_status visit(ir_end_primitive *)
> > +   virtual ir_visitor_status visit_leave(ir_emit_vertex *ir)
> >     {
> > -      found = true;
> > -      return visit_stop;
> > +      int stream_id = ir->stream_id();
> > +
> > +      if (stream_id < 0) {
> > +         invalid_stream_id = stream_id;
> > +         invalid_stream_id_from_emit_vertex = true;
> > +         return visit_stop;
> > +      }
> > +
> > +      if (stream_id > max_stream_allowed) {
> > +         invalid_stream_id = stream_id;
> > +         invalid_stream_id_from_emit_vertex = true;
> > +         return visit_stop;
> > +      }
> > +
> > +      if (stream_id != 0)
> > +         uses_non_zero_stream = true;
> > +
> > +      return visit_continue;
> >     }
> >  
> > -   bool end_primitive_found()
> > +   virtual ir_visitor_status visit_leave(ir_end_primitive *ir)
> >     {
> > -      return found;
> > +      end_primitive_found = true;
> > +
> > +      int stream_id = ir->stream_id();
> > +
> > +      if (stream_id < 0) {
> > +         invalid_stream_id = stream_id;
> > +         invalid_stream_id_from_emit_vertex = false;
> > +         return visit_stop;
> > +      }
> > +
> > +      if (stream_id > max_stream_allowed) {
> > +         invalid_stream_id = stream_id;
> > +         invalid_stream_id_from_emit_vertex = false;
> > +         return visit_stop;
> > +      }
> > +
> > +      if (stream_id != 0)
> > +         uses_non_zero_stream = true;
> > +
> > +      return visit_continue;
> > +   }
> > +
> > +   bool error()
> > +   {
> > +      return invalid_stream_id != 0;
> > +   }
> > +
> > +   const char *error_func()
> > +   {
> > +      return invalid_stream_id_from_emit_vertex ?
> > +         "EmitStreamVertex" : "EndStreamPrimitive";
> > +   }
> > +
> > +   int error_stream()
> > +   {
> > +      return invalid_stream_id;
> > +   }
> > +
> > +   bool uses_streams()
> > +   {
> > +      return uses_non_zero_stream;
> > +   }
> > +
> > +   bool uses_end_primitive()
> > +   {
> > +      return end_primitive_found;
> >     }
> >  
> >  private:
> > -   bool found;
> > +   int max_stream_allowed;
> > +   int invalid_stream_id;
> > +   bool invalid_stream_id_from_emit_vertex;
> > +   bool end_primitive_found;
> > +   bool uses_non_zero_stream;
> >  };
> >  
> >  } /* anonymous namespace */
> > @@ -551,10 +620,58 @@ validate_geometry_shader_executable(struct gl_shader_program *prog,
> >  
> >     analyze_clip_usage(prog, shader, &prog->Geom.UsesClipDistance,
> >                        &prog->Geom.ClipDistanceArraySize);
> > +}
> > +
> > +/**
> > + * Check if geometry shaders emit to non-zero streams and do corresponding
> > + * validations.
> > + */
> > +static void
> > +validate_geometry_shader_emissions(struct gl_context *ctx,
> > +                                   struct gl_shader_program *prog)
> > +{
> > +   if (prog->_LinkedShaders[MESA_SHADER_GEOMETRY] != NULL) {
> > +      find_emit_vertex_visitor emit_vertex(ctx->Const.MaxVertexStreams - 1);
> > +      emit_vertex.run(prog->_LinkedShaders[MESA_SHADER_GEOMETRY]->ir);
> > +      if (emit_vertex.error()) {
> > +         linker_error(prog, "Invalid call %s(%d). Accepted values for the "
> > +                      "stream parameter are in the range [0, %d].",
> > +                      emit_vertex.error_func(),
> > +                      emit_vertex.error_stream(),
> > +                      ctx->Const.MaxVertexStreams - 1);
> > +      }
> > +      prog->Geom.UsesStreams = emit_vertex.uses_streams();
> > +      prog->Geom.UsesEndPrimitive = emit_vertex.uses_end_primitive();
> >  
> > -   find_end_primitive_visitor end_primitive;
> > -   end_primitive.run(shader->ir);
> > -   prog->Geom.UsesEndPrimitive = end_primitive.end_primitive_found();
> > +      /* From the ARB_gpu_shader5 spec:
> > +       *
> > +       *   "Multiple vertex streams are supported only if the output primitive
> > +       *    type is declared to be "points".  A program will fail to link if it
> > +       *    contains a geometry shader calling EmitStreamVertex() or
> > +       *    EndStreamPrimitive() if its output primitive type is not "points".
> > +       *
> > +       * However, in the same spec:
> > +       *
> > +       *   "The function EmitVertex() is equivalent to calling EmitStreamVertex()
> > +       *    with <stream> set to zero."
> > +       *
> > +       * And:
> > +       *
> > +       *   "The function EndPrimitive() is equivalent to calling
> > +       *    EndStreamPrimitive() with <stream> set to zero."
> > +       *
> > +       * Since we can call EmitVertex() and EndPrimitive() when we output
> > +       * primitives other than points, calling EmitStreamVertex(0) or
> > +       * EmitEndPrimitive(0) should not produce errors. This it also what Nvidia
> > +       * does. Currently we only set prog->Geom.UsesStreams to TRUE when
> > +       * EmitStreamVertex() or EmitEndPrimitive() are called with a non-zero
> > +       * stream.
> 
> Does AMD also behave this way?  If so, I can submit a spec bug to make
> these explicitly legal.  Otherwise, I think we should not allow it.  We
> probably ought to check Apple and the Intel Windows driver too...

I am not sure that I have access to all of these systems, but let me
see.

FWIW, the spec also says this:

"The function EmitVertex() is equivalent to calling EmitStreamVertex()
with <stream> set to zero."

And they can't be equivalent if ones goes through checks that the other
does not...

> > +       */
> > +      if (prog->Geom.UsesStreams && prog->Geom.OutputType != GL_POINTS) {
> > +         linker_error(prog, "EmitStreamVertex(n) and EndStreamPrimitive(n) "
> > +                      "with n>0 requires point output");
> > +      }
> > +   }
> >  }
> >  
> >  
> > @@ -2556,6 +2673,9 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> >  	 ;
> >     }
> >  
> > +   /* Check and validate stream emissions in geometry shaders */
> > +   validate_geometry_shader_emissions(ctx, prog);
> > +
> >     /* Mark all generic shader inputs and outputs as unpaired. */
> >     for (unsigned i = MESA_SHADER_VERTEX; i <= MESA_SHADER_FRAGMENT; i++) {
> >        if (prog->_LinkedShaders[i] != NULL) {
> > 
> 
>