[Mesa-dev] [PATCH 1/5] gallium/util: add a helper util_create_shader_from_text, cleanup some code

[Jose Fonseca]

On 09/12/14 11:28, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> It's exposed, because I plan to use it elsewhere later.

I hesitate in repeating myself, but I can't close my eyes.  This is such 
a bad idea, at so many levels.

How will this cope when the application is running on a different 
locale, that adds ',' '.' or ' ' to thousands, or something unexpected 
like that?

Sure, so far the templates only use %s, but this 
util_create_shader_from_text is just glorifying a bad coding practices, 
being a matter of time until someone adds a %f or %d .

NAK, FWIW.

> ---
>   src/gallium/auxiliary/util/u_simple_shaders.c | 103 ++++++++++++--------------
>   src/gallium/auxiliary/util/u_simple_shaders.h |   3 +
>   2 files changed, 49 insertions(+), 57 deletions(-)
>
> diff --git a/src/gallium/auxiliary/util/u_simple_shaders.c b/src/gallium/auxiliary/util/u_simple_shaders.c
> index edb3037..abe07ef 100644
> --- a/src/gallium/auxiliary/util/u_simple_shaders.c
> +++ b/src/gallium/auxiliary/util/u_simple_shaders.c
> @@ -48,6 +48,43 @@
>   #include <stdio.h> /* include last */
>
>
> +/**
> + * Create a shader CSO from a textual TGSI shader representation.
> + * The text can be formatted like printf.
> + */
> +void *
> +util_create_shader_from_text(struct pipe_context *ctx, const char *text, ...)
> +{
> +   char final[4096];
> +   struct tgsi_token tokens[1024];

8K. Might be OK, but if it needs to get any bigger we should malloc to 
avoid stack overflow on Windows.


> +   struct pipe_shader_state state = {tokens};
> +   va_list ap;
> +
> +   va_start(ap, text);
> +   if (vsnprintf(final, sizeof(final), text, ap) >= sizeof(final)) {

vsnprintf -> util_vsnprintf


> +      va_end(ap);
> +      fprintf(stderr, "Cannot format a shader: %s\n", text);

fprintf(stderr) is usually not visible on Windows. Please use _debug_printf.



Jose

> +      return NULL;
> +   }
> +   va_end(ap);
> +
> +   if (!tgsi_text_translate(final, tokens, Elements(tokens))) {
> +      fprintf(stderr, "Cannot translate a shader:\n%s\n", final);
> +      return NULL;
> +   }
> +
> +   if (!memcmp(final, "VERT", 4))
> +      return ctx->create_vs_state(ctx, &state);
> +   else if (!memcmp(final, "GEOM", 4))
> +      return ctx->create_gs_state(ctx, &state);
> +   else if (!memcmp(final, "FRAG", 4))
> +      return ctx->create_fs_state(ctx, &state);
> +   else {
> +      assert(0);
> +      return NULL;
> +   }
> +}
> +
>
>   /**
>    * Make simple vertex pass-through shader.
> @@ -120,14 +157,8 @@ void *util_make_layered_clear_vertex_shader(struct pipe_context *pipe)
>            "MOV OUT[1], IN[1]\n"
>            "MOV OUT[2], SV[0]\n"
>            "END\n";
> -   struct tgsi_token tokens[1000];
> -   struct pipe_shader_state state = {tokens};
>
> -   if (!tgsi_text_translate(text, tokens, Elements(tokens))) {
> -      assert(0);
> -      return NULL;
> -   }
> -   return pipe->create_vs_state(pipe, &state);
> +   return util_create_shader_from_text(pipe, text);
>   }
>
>   /**
> @@ -436,24 +467,10 @@ util_make_fragment_passthrough_shader(struct pipe_context *pipe,
>            "MOV OUT[0], IN[0]\n"
>            "END\n";
>
> -   char text[sizeof(shader_templ)+100];
> -   struct tgsi_token tokens[1000];
> -   struct pipe_shader_state state = {tokens};
> -
> -   sprintf(text, shader_templ,
> -           write_all_cbufs ? "PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1\n" : "",
> -           tgsi_semantic_names[input_semantic],
> -           tgsi_interpolate_names[input_interpolate]);
> -
> -   if (!tgsi_text_translate(text, tokens, Elements(tokens))) {
> -      assert(0);
> -      return NULL;
> -   }
> -#if 0
> -   tgsi_dump(state.tokens, 0);
> -#endif
> -
> -   return pipe->create_fs_state(pipe, &state);
> +   return util_create_shader_from_text(pipe, shader_templ,
> +            write_all_cbufs ? "PROPERTY FS_COLOR0_WRITES_ALL_CBUFS 1\n" : "",
> +            tgsi_semantic_names[input_semantic],
> +            tgsi_interpolate_names[input_interpolate]);
>   }
>
>
> @@ -520,26 +537,12 @@ util_make_fs_blit_msaa_gen(struct pipe_context *pipe,
>            "TXF OUT[0]%s, TEMP[0], SAMP[0], %s\n"
>            "END\n";
>
> -   const char *type = tgsi_texture_names[tgsi_tex];
> -   char text[sizeof(shader_templ)+100];
> -   struct tgsi_token tokens[1000];
> -   struct pipe_shader_state state = {tokens};
> -
>      assert(tgsi_tex == TGSI_TEXTURE_2D_MSAA ||
>             tgsi_tex == TGSI_TEXTURE_2D_ARRAY_MSAA);
>
> -   sprintf(text, shader_templ, output_semantic, output_mask, type);
> -
> -   if (!tgsi_text_translate(text, tokens, Elements(tokens))) {
> -      puts(text);
> -      assert(0);
> -      return NULL;
> -   }
> -#if 0
> -   tgsi_dump(state.tokens, 0);
> -#endif
> -
> -   return pipe->create_fs_state(pipe, &state);
> +   return util_create_shader_from_text(pipe, shader_templ, output_semantic,
> +                                       output_mask,
> +                                       tgsi_texture_names[tgsi_tex]);
>   }
>
>
> @@ -608,26 +611,12 @@ util_make_fs_blit_msaa_depthstencil(struct pipe_context *pipe,
>            "TXF OUT[0].z, TEMP[0], SAMP[0], %s\n"
>            "TXF OUT[1].y, TEMP[0], SAMP[1], %s\n"
>            "END\n";
> -
>      const char *type = tgsi_texture_names[tgsi_tex];
> -   char text[sizeof(shader_templ)+100];
> -   struct tgsi_token tokens[1000];
> -   struct pipe_shader_state state = {tokens};
>
>      assert(tgsi_tex == TGSI_TEXTURE_2D_MSAA ||
>             tgsi_tex == TGSI_TEXTURE_2D_ARRAY_MSAA);
>
> -   sprintf(text, shader_templ, type, type);
> -
> -   if (!tgsi_text_translate(text, tokens, Elements(tokens))) {
> -      assert(0);
> -      return NULL;
> -   }
> -#if 0
> -   tgsi_dump(state.tokens, 0);
> -#endif
> -
> -   return pipe->create_fs_state(pipe, &state);
> +   return util_create_shader_from_text(pipe, shader_templ, type, type);
>   }
>
>
> diff --git a/src/gallium/auxiliary/util/u_simple_shaders.h b/src/gallium/auxiliary/util/u_simple_shaders.h
> index dd282e0..12f50f8 100644
> --- a/src/gallium/auxiliary/util/u_simple_shaders.h
> +++ b/src/gallium/auxiliary/util/u_simple_shaders.h
> @@ -44,6 +44,9 @@ extern "C" {
>
>
>   extern void *
> +util_create_shader_from_text(struct pipe_context *ctx, const char *text, ...);
> +
> +extern void *
>   util_make_vertex_passthrough_shader(struct pipe_context *pipe,
>                                       uint num_attribs,
>                                       const uint *semantic_names,
>