[Mesa-dev] [PATCH] mesa: allocate transform_feedback_info::Outputs array dynamically
Paul Berry
stereotype441 at gmail.com
Thu Jan 19 18:41:55 PST 2012
On 17 January 2012 11:29, 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.
>
> v2: added assertions
>
> NOTE: This is a candidate for the 8.0 branch.
> ---
> src/glsl/linker.cpp | 64
> ++++++++++++++++++++++++++++++------------------
> src/mesa/main/mtypes.h | 32 ++++++++++++-----------
> 2 files changed, 57 insertions(+), 39 deletions(-)
>
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 0d85aee..b6c5754 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -1388,9 +1388,10 @@ 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;
> + unsigned varying, const unsigned max_outputs) 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,28 @@ 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 unsigned max_outputs) const
> +{
> /* From GL_EXT_transform_feedback:
> * A program will fail to link if:
> *
> @@ -1663,19 +1679,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;
> @@ -1683,6 +1686,7 @@ tfeedback_decl::store(struct gl_context *ctx, struct
> gl_shader_program *prog,
> for (unsigned index = 0; index < translated_size; ++index) {
> for (unsigned v = 0; v < this->matrix_columns; ++v) {
> unsigned num_components = this->vector_elements;
> + assert(info->NumOutputs < max_outputs);
> info->Outputs[info->NumOutputs].ComponentOffset = 0;
> if (this->is_clip_distance_mesa) {
> if (this->is_subscripted) {
> @@ -1976,6 +1980,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,12 +1993,23 @@ 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);
>
I know I already gave you a review over IRC, but I just realized that this
is a memory leak. The first argument of rzalloc_array needs to be prog,
not prog->LinkedTransformFeedback.Outputs. See Mesa commit 5a0f395 ("glsl:
Fix leak of LinkedTransformFeedback.Varyings.") from Eric Anholt for the
parallel fix to prog->LinkedTransformFeedback.Varyings.
Sorry for missing that on my earlier review.
> +
> for (unsigned i = 0; i < num_tfeedback_decls; ++i) {
> unsigned buffer = separate_attribs_mode ? i : 0;
> if (!tfeedback_decls[i].store(ctx, prog,
> &prog->LinkedTransformFeedback,
> - buffer, i))
> + buffer, i, num_outputs))
> return false;
> }
> + assert(prog->LinkedTransformFeedback.NumOutputs == num_outputs);
>
> return true;
> }
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 9fdabf9..95450f9 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -1823,6 +1823,22 @@ struct gl_transform_feedback_varying_info {
> GLint Size;
> };
>
> +struct gl_transform_feedback_output {
> + unsigned OutputRegister;
> + unsigned OutputBuffer;
> + unsigned NumComponents;
> +
> + /** offset (in DWORDs) of this output within the interleaved structure
> */
> + unsigned DstOffset;
> +
> + /**
> + * Offset into the output register of the data to output. For example,
> + * if NumComponents is 2 and ComponentOffset is 1, then the data to
> + * offset is in the y and z components of the output register.
> + */
> + unsigned ComponentOffset;
> +};
> +
> /** Post-link transform feedback info. */
> struct gl_transform_feedback_info {
> unsigned NumOutputs;
> @@ -1832,21 +1848,7 @@ struct gl_transform_feedback_info {
> */
> unsigned NumBuffers;
>
> - struct {
> - unsigned OutputRegister;
> - unsigned OutputBuffer;
> - unsigned NumComponents;
> -
> - /** offset (in DWORDs) of this output within the interleaved
> structure */
> - unsigned DstOffset;
> -
> - /**
> - * Offset into the output register of the data to output. For
> example,
> - * if NumComponents is 2 and ComponentOffset is 1, then the data to
> - * offset is in the y and z components of the output register.
> - */
> - unsigned ComponentOffset;
> - } Outputs[MAX_PROGRAM_OUTPUTS];
> + struct gl_transform_feedback_output *Outputs;
>
> /** Transform feedback varyings used for the linking of this shader
> program.
> *
> --
> 1.7.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120119/9aa8a563/attachment-0001.htm>
More information about the mesa-dev
mailing list