[Mesa-dev] [PATCH] mesa: change 'SHADER_SUBST' facility to work with env variables

[Brian Paul]

Looks good.  Just some minor nitpicks below.

Reviewed-by: Brian Paul <brianp at vmware.com>


On 08/31/2015 01:23 AM, Tapani Pälli wrote:
> Patch modifies existing shader source and replace functionality to work
> with environment variables rather than enable dumping on compile time.
> Also instead of _mesa_str_checksum, _mesa_sha1_compute is used to avoid
> collisions.
>
> Functionality is controlled via 2 environment variables:

s/2/two/


>
> MESA_SHADER_DUMP_PATH - path where shader sources are dumped
> MESA_SHADER_READ_PATH - path where replacement shaders are read
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> Suggested-by: Eero Tamminen <eero.t.tamminen at intel.com>
> ---
>   docs/shading.html         |   9 +++
>   src/mesa/main/shaderapi.c | 136 ++++++++++++++++++++++++++++++++--------------
>   2 files changed, 105 insertions(+), 40 deletions(-)
>
> diff --git a/docs/shading.html b/docs/shading.html
> index 77a0ee4..784329e 100644
> --- a/docs/shading.html
> +++ b/docs/shading.html
> @@ -63,6 +63,15 @@ execution.  These are generally used for debugging.
>   Example:  export MESA_GLSL=dump,nopt
>   </p>
>
> +<p>
> +Shaders can be dumped and replaced on runtime for debugging purposes.
> +This is controlled via following environment variables:
> +<ul>
> +<li><b>MESA_SHADER_DUMP_PATH</b> - path where shader sources are dumped
> +<li><b>MESA_SHADER_READ_PATH</b> - path where replacement shaders are read

Do these directories need to be different to prevent collisions between 
the dumped shaders and the replacement shaders?


> +</ul>
> +Note, path set must exist before running for dumping or replacing to work.
> +</p>
>
>   <h2 id="support">GLSL Version</h2>
>
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 0e0e0d6..d3abfc9 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -53,15 +53,13 @@
>   #include "program/prog_parameter.h"
>   #include "util/ralloc.h"
>   #include "util/hash_table.h"
> +#include "util/mesa-sha1.h"
>   #include <stdbool.h>
>   #include "../glsl/glsl_parser_extras.h"
>   #include "../glsl/ir.h"
>   #include "../glsl/ir_uniform.h"
>   #include "../glsl/program.h"
>
> -/** Define this to enable shader substitution (see below) */
> -#define SHADER_SUBST 0
> -
>
>   /**
>    * Return mask of GLSL_x flags by examining the MESA_GLSL env var.
> @@ -1512,24 +1510,101 @@ _mesa_LinkProgram(GLhandleARB programObj)
>      link_program(ctx, programObj);
>   }
>
> +/**
> + * Generate a SHA-1 hash value string for given source string.
> + */
> +static void
> +generate_sha1(const char *source, char sha_str[64])
> +{
> +   unsigned char sha[20];
> +   _mesa_sha1_compute(source, strlen(source), sha);
> +   _mesa_sha1_format(sha_str, sha);
> +}
> +
> +/**
> + * Construct a full path for shader replacement functionality using
> + * following format:
> + *
> + * <path>/<stage prefix>_<CHECKSUM>.glsl
> + */
> +static void
> +construct_name(const gl_shader_stage stage, const char *source,
> +               const char *path, char *name, unsigned length)
> +{
> +   char sha[64];
> +   static const char *types[] = {
> +      "VS", "TC", "TE", "GS", "FS", "CS",
> +   };
>
> +   generate_sha1(source, sha);
> +   _mesa_snprintf(name, length, "%s/%s_%s.glsl", path, types[stage],
> +                  sha);
> +}
> +
> +/**
> + * Write given shader source to a file in MESA_SHADER_DUMP_PATH.
> + */
> +static void
> +dump_shader(const gl_shader_stage stage, const char *source)
> +{
> +   char name[PATH_MAX];
> +   static bool path_exists = true;
> +   char *dump_path;
> +   FILE *f;
> +
> +   if (!path_exists)
> +      return NULL;
> +
> +   dump_path = getenv("MESA_SHADER_DUMP_PATH");
> +

Could remove that empty line.

> +   if (!dump_path) {
> +      path_exists = false;
> +      return NULL;
> +   }
> +
> +   construct_name(stage, source, dump_path, name, PATH_MAX);
> +
> +   f = fopen(name, "w");
> +   if (f) {
> +      fputs(source, f);
> +      fclose(f);
> +   } else {
> +      GET_CURRENT_CONTEXT(ctx);
> +      _mesa_warning(ctx, "could not open %s for dumping shader", name);
> +   }
> +}
>
>   /**
>    * Read shader source code from a file.
>    * Useful for debugging to override an app's shader.
>    */
>   static GLcharARB *
> -read_shader(const char *fname)
> +read_shader(const gl_shader_stage stage, const char *source)
>   {
> -   int shader_size = 0;
> -   FILE *f = fopen(fname, "r");
> -   GLcharARB *buffer, *shader;
> -   int len;
> +   char name[PATH_MAX];
> +   char *read_path;
> +   static bool path_exists = true;
> +   int len, shader_size = 0;
> +   GLcharARB *buffer;
> +   FILE *f;
>
> -   if (!f) {
> +   if (!path_exists)
> +      return NULL;
> +
> +   read_path = getenv("MESA_SHADER_READ_PATH");
> +
> +   if (!read_path) {
> +      path_exists = false;
>         return NULL;
>      }
>
> +   construct_name(stage, source, read_path, name, PATH_MAX);
> +
> +   f = fopen(name, "r");
> +

Could remove that empty line.

> +   if (!f)
> +      return NULL;
> +
>      /* allocate enough room for the entire shader */
>      fseek(f, 0, SEEK_END);
>      shader_size = ftell(f);
> @@ -1547,13 +1622,9 @@ read_shader(const char *fname)
>
>      fclose(f);
>
> -   shader = strdup(buffer);
> -   free(buffer);
> -
> -   return shader;
> +   return buffer;
>   }
>
> -
>   /**
>    * Called via glShaderSource() and glShaderSourceARB() API functions.
>    * Basically, concatenate the source code strings into one long string
> @@ -1566,14 +1637,15 @@ _mesa_ShaderSource(GLhandleARB shaderObj, GLsizei count,
>      GET_CURRENT_CONTEXT(ctx);
>      GLint *offsets;
>      GLsizei i, totalLength;
> -   GLcharARB *source;
> -   GLuint checksum;
> +   GLcharARB *source, *replacement;
>
>      if (!shaderObj || string == NULL) {
>         _mesa_error(ctx, GL_INVALID_VALUE, "glShaderSourceARB");
>         return;
>      }
>
> +   struct gl_shader *sh = _mesa_lookup_shader(ctx, shaderObj);

For C sources, I think we're still trying to declare all variables at 
the top of scope.


> +
>      /*
>       * This array holds offsets of where the appropriate string ends, thus the
>       * last element will be set to the total length of the source code.
> @@ -1620,35 +1692,19 @@ _mesa_ShaderSource(GLhandleARB shaderObj, GLsizei count,
>      source[totalLength - 1] = '\0';
>      source[totalLength - 2] = '\0';
>
> -   if (SHADER_SUBST) {
> -      /* Compute the shader's source code checksum then try to open a file
> -       * named newshader_<CHECKSUM>.  If it exists, use it in place of the
> -       * original shader source code.  For debugging.
> -       */
> -      char filename[100];
> -      GLcharARB *newSource;
> -
> -      checksum = _mesa_str_checksum(source);
> -
> -      _mesa_snprintf(filename, sizeof(filename), "newshader_%d", checksum);
> +   /* Dump original shader source to MESA_SHADER_DUMP_PATH and replace
> +    * if corresponding entry found from MESA_SHADER_READ_PATH.
> +    */
> +   dump_shader(sh->Stage, source);
>
> -      newSource = read_shader(filename);
> -      if (newSource) {
> -         fprintf(stderr, "Mesa: Replacing shader %u chksum=%d with %s\n",
> -                       shaderObj, checksum, filename);
> -         free(source);
> -         source = newSource;
> -      }
> +   replacement = read_shader(sh->Stage, source);
> +   if (replacement) {
> +      free(source);
> +      source = replacement;
>      }
>
>      shader_source(ctx, shaderObj, source);
>
> -   if (SHADER_SUBST) {
> -      struct gl_shader *sh = _mesa_lookup_shader(ctx, shaderObj);
> -      if (sh)
> -         sh->SourceChecksum = checksum; /* save original checksum */
> -   }
> -
>      free(offsets);
>   }
>
>