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

[Ian Romanick]

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...

> +       */
> +      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) {
>