[Mesa-dev] [PATCH] mesa: change 'SHADER_SUBST' facility to work with env variables
Tapani Pälli
tapani.palli at intel.com
Tue Sep 1 22:58:01 PDT 2015
On 09/01/2015 04:40 PM, Brian Paul wrote:
> On 09/01/2015 06:04 AM, Tapani Pälli wrote:
>>
>>
>> On 08/31/2015 05:56 PM, Brian Paul wrote:
>>> Looks good. Just some minor nitpicks below.
>>>
>>> Reviewed-by: Brian Paul <brianp at vmware.com>
>>
>> Thanks! I have one comment below ..
>>
>>>
>>> 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?
>>
>> Yes, I thought a bit of solutions to work only in one dir but I think
>> having 2 makes everything more clear and here user can be only one
>> overwriting edited contents. I could add some text here : "These paths
>> should be separate to avoid overwriting modified contents." (?)
>
> How about "When both are set, these paths should be different so the
> dumped shaders do not clobber the replacement shaders."
>
OK, I'll go with that. I've also now noticed one big problem about the
patch, if one does not have one of the sha-1 libraries installed this
fails in awful way. I think I need to add some define like HAVE_SHA and
surround the code with this.
> -Brian
>
>
>
>>
>>>
>>>> +</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);
>>>> }
>>>>
>>>>
>>>
>
More information about the mesa-dev
mailing list