[Mesa-dev] [PATCH] mesa: allocate transform_feedback_info::Outputs array dynamically

[Paul Berry]

On 17 January 2012 09:31, Christoph Bumiller
<e0425955 at student.tuwien.ac.at>wrote:

> The nvc0 gallium driver is advertising 128 MAX_INTERLEAVED_COMPS
> which made it always assert in the linker when TFB was used since
> the Outputs array was smaller than that maximum.
>
> NOTE: This is a candidate for the 8.0 branch.
> ---
>  src/glsl/linker.cpp    |   57
> +++++++++++++++++++++++++++++------------------
>  src/mesa/main/mtypes.h |   32 ++++++++++++++------------
>  2 files changed, 52 insertions(+), 37 deletions(-)
>
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 0d85aee..1a986ef 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -1388,6 +1388,7 @@ public:
>    static bool is_same(const tfeedback_decl &x, const tfeedback_decl &y);
>    bool assign_location(struct gl_context *ctx, struct gl_shader_program
> *prog,
>                         ir_variable *output_var);
> +   bool accumulate_num_outputs(struct gl_shader_program *prog, unsigned
> *count);
>    bool store(struct gl_context *ctx, struct gl_shader_program *prog,
>               struct gl_transform_feedback_info *info, unsigned buffer,
>              unsigned varying) const;
> @@ -1624,16 +1625,9 @@ tfeedback_decl::assign_location(struct gl_context
> *ctx,
>  }
>
>
> -/**
> - * Update gl_transform_feedback_info to reflect this tfeedback_decl.
> - *
> - * If an error occurs, the error is reported through linker_error() and
> false
> - * is returned.
> - */
>  bool
> -tfeedback_decl::store(struct gl_context *ctx, struct gl_shader_program
> *prog,
> -                      struct gl_transform_feedback_info *info,
> -                      unsigned buffer, unsigned varying) const
> +tfeedback_decl::accumulate_num_outputs(struct gl_shader_program *prog,
> +                                       unsigned *count)
>  {
>    if (!this->is_assigned()) {
>       /* From GL_EXT_transform_feedback:
> @@ -1648,6 +1642,27 @@ tfeedback_decl::store(struct gl_context *ctx,
> struct gl_shader_program *prog,
>       return false;
>    }
>
> +   unsigned translated_size = this->size;
> +   if (this->is_clip_distance_mesa)
> +      translated_size = (translated_size + 3) / 4;
> +
> +   *count += translated_size * this->matrix_columns;
> +
> +   return true;
> +}
> +
> +
> +/**
> + * Update gl_transform_feedback_info to reflect this tfeedback_decl.
> + *
> + * If an error occurs, the error is reported through linker_error() and
> false
> + * is returned.
> + */
> +bool
> +tfeedback_decl::store(struct gl_context *ctx, struct gl_shader_program
> *prog,
> +                      struct gl_transform_feedback_info *info,
> +                      unsigned buffer, unsigned varying) const
> +{
>    /* From GL_EXT_transform_feedback:
>     *   A program will fail to link if:
>     *
> @@ -1663,19 +1678,6 @@ tfeedback_decl::store(struct gl_context *ctx,
> struct gl_shader_program *prog,
>       return false;
>    }
>
> -   /* Verify that the checks on
> MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS
> -    * and MAX_TRANSFORM_FEEDBACK_SEPARATE_COMPONENTS are sufficient to
> prevent
> -    * overflow of info->Outputs[].  In worst case we generate one entry in
> -    * Outputs[] per component so a conservative check is to verify that
> the
> -    * size of the array is greater than or equal to both
> -    * MAX_TRANSFORM_FEEDBACK_INTERLEAVED_COMPONENTS and
> -    * MAX_TRANSFORM_FEEDBACK_SEPARATE_COMPONENTS.
> -    */
> -   assert(Elements(info->Outputs) >=
> -          ctx->Const.MaxTransformFeedbackInterleavedComponents);
> -   assert(Elements(info->Outputs) >=
> -          ctx->Const.MaxTransformFeedbackSeparateComponents);
> -
>    unsigned translated_size = this->size;
>    if (this->is_clip_distance_mesa)
>       translated_size = (translated_size + 3) / 4;
> @@ -1976,6 +1978,7 @@ store_tfeedback_info(struct gl_context *ctx, struct
> gl_shader_program *prog,
>       prog->TransformFeedback.BufferMode == GL_SEPARATE_ATTRIBS;
>
>    ralloc_free(prog->LinkedTransformFeedback.Varyings);
> +   ralloc_free(prog->LinkedTransformFeedback.Outputs);
>
>    memset(&prog->LinkedTransformFeedback, 0,
>           sizeof(prog->LinkedTransformFeedback));
> @@ -1988,6 +1991,16 @@ store_tfeedback_info(struct gl_context *ctx, struct
> gl_shader_program *prog,
>                    struct gl_transform_feedback_varying_info,
>                    num_tfeedback_decls);
>
> +   unsigned num_outputs = 0;
> +   for (unsigned i = 0; i < num_tfeedback_decls; ++i)
> +      if (!tfeedback_decls[i].accumulate_num_outputs(prog, &num_outputs))
> +         return false;
> +
> +   prog->LinkedTransformFeedback.Outputs =
> +      rzalloc_array(prog->LinkedTransformFeedback.Outputs,
> +                    struct gl_transform_feedback_output,
> +                    num_outputs);
> +
>

Can we add an assertion to the bottom of store_tfeedback_info to verify
that num_outputs == prog->LinkedTransformFeedback.NumOutputs?  That would
give me a lot more confidence in this patch.

With that change, this patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120117/409c4f27/attachment.htm>