[Mesa-dev] [PATCH] mesa/main: Rework the shader capture naming convention
Eero Tamminen
eero.t.tamminen at intel.com
Mon Apr 30 13:44:44 UTC 2018
Hi,
On 28.04.2018 17:39, Benedikt Schemmer wrote:
> Change from a purely number.shader_test naming scheme to sha_number.shader_test
> because especially games often use the same number for entirely different shaders
> based on graphics settings etc. and then already captured shaders get overwritten.
> It is also useful for capturing shaders from applications that reuse program
> numbers a lot i.e. piglit. This has helped to identify problems there as well.
>
> Plus minor cleanups, that could be split into a separate patch.
For review, you need to have these as separate patches:
- moving code around
- actual functional changes
- non-functional code "cleanups", like changing "foo == NULL" to "!foo"
(Small commits in your own repo and "git rebase -i" help.)
- Eero
> I have used this for approximately two months now, seems to work just fine.
>
> ---
> src/mesa/main/shaderapi.c | 315 ++++++++++++++++++++++++----------------------
> 1 file changed, 168 insertions(+), 147 deletions(-)
>
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 44b18af492..65cf0a67a2 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -64,6 +64,122 @@
> #include "util/mesa-sha1.h"
> #include "util/crc32.h"
>
> +
> +/**
> + * 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);
> +}
> +
> +
> +#ifdef ENABLE_SHADER_CACHE
> +/**
> + * Construct a full path for shader replacement functionality using
> + * following format:
> + *
> + * <path>/<stage prefix>_<CHECKSUM>.glsl
> + */
> +static char *
> +construct_name(const gl_shader_stage stage, const char *source,
> + const char *path)
> +{
> + char sha[64];
> + static const char *types[] = {
> + "VS", "TC", "TE", "GS", "FS", "CS",
> + };
> +
> + generate_sha1(source, sha);
> + return ralloc_asprintf(NULL, "%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)
> +{
> + static bool path_exists = true;
> + char *dump_path;
> + FILE *f;
> +
> + if (!path_exists)
> + return;
> +
> + dump_path = getenv("MESA_SHADER_DUMP_PATH");
> + if (!dump_path) {
> + path_exists = false;
> + return;
> + }
> +
> + char *name = construct_name(stage, source, dump_path);
> +
> + 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 (%s)", name,
> + strerror(errno));
> + }
> + ralloc_free(name);
> +}
> +
> +/**
> + * Read shader source code from a file.
> + * Useful for debugging to override an app's shader.
> + */
> +static GLcharARB *
> +read_shader(const gl_shader_stage stage, const char *source)
> +{
> + char *read_path;
> + static bool path_exists = true;
> + int len, shader_size = 0;
> + GLcharARB *buffer;
> + FILE *f;
> +
> + if (!path_exists)
> + return NULL;
> +
> + read_path = getenv("MESA_SHADER_READ_PATH");
> + if (!read_path) {
> + path_exists = false;
> + return NULL;
> + }
> +
> + char *name = construct_name(stage, source, read_path);
> + f = fopen(name, "r");
> + ralloc_free(name);
> + if (!f)
> + return NULL;
> +
> + /* allocate enough room for the entire shader */
> + fseek(f, 0, SEEK_END);
> + shader_size = ftell(f);
> + rewind(f);
> + assert(shader_size);
> +
> + /* add one for terminating zero */
> + shader_size++;
> +
> + buffer = malloc(shader_size);
> + assert(buffer);
> +
> + len = fread(buffer, 1, shader_size, f);
> + buffer[len] = 0;
> +
> + fclose(f);
> +
> + return buffer;
> +}
> +
> +#endif /* ENABLE_SHADER_CACHE */
> +
> /**
> * Return mask of GLSL_x flags by examining the MESA_GLSL env var.
> */
> @@ -125,11 +241,10 @@ _mesa_init_shader_state(struct gl_context *ctx)
> /* Device drivers may override these to control what kind of instructions
> * are generated by the GLSL compiler.
> */
> - struct gl_shader_compiler_options options;
> + struct gl_shader_compiler_options options = {};
> gl_shader_stage sh;
> int i;
>
> - memset(&options, 0, sizeof(options));
> options.MaxUnrollIterations = 32;
> options.MaxIfDepth = UINT_MAX;
>
> @@ -138,7 +253,7 @@ _mesa_init_shader_state(struct gl_context *ctx)
>
> ctx->Shader.Flags = _mesa_get_shader_flags();
>
> - if (ctx->Shader.Flags != 0)
> + if (ctx->Shader.Flags)
> ctx->Const.GenerateTemporaryNames = true;
>
> /* Extended for ARB_separate_shader_objects */
> @@ -655,7 +770,8 @@ get_programiv(struct gl_context *ctx, GLuint program, GLenum pname,
> GLint *params)
> {
> struct gl_shader_program *shProg
> - = _mesa_lookup_shader_program_err(ctx, program, "glGetProgramiv(program)");
> + = _mesa_lookup_shader_program_err(ctx, program,
> + "glGetProgramiv(program)");
>
> /* Is transform feedback available in this context?
> */
> @@ -837,7 +953,7 @@ get_programiv(struct gl_context *ctx, GLuint program, GLenum pname,
> *params = shProg->BinaryRetreivableHint;
> return;
> case GL_PROGRAM_BINARY_LENGTH:
> - if (ctx->Const.NumProgramBinaryFormats == 0) {
> + if (!ctx->Const.NumProgramBinaryFormats) {
> *params = 0;
> } else {
> _mesa_get_program_binary_length(ctx, shProg, params);
> @@ -858,7 +974,7 @@ get_programiv(struct gl_context *ctx, GLuint program, GLenum pname,
> "linked)");
> return;
> }
> - if (shProg->_LinkedShaders[MESA_SHADER_COMPUTE] == NULL) {
> + if (!shProg->_LinkedShaders[MESA_SHADER_COMPUTE]) {
> _mesa_error(ctx, GL_INVALID_OPERATION, "glGetProgramiv(no compute "
> "shaders)");
> return;
> @@ -1118,7 +1234,7 @@ _mesa_compile_shader(struct gl_context *ctx, struct gl_shader *sh)
> } else {
> if (ctx->_Shader->Flags & GLSL_DUMP) {
> _mesa_log("GLSL source for %s shader %d:\n",
> - _mesa_shader_stage_to_string(sh->Stage), sh->Name);
> + _mesa_shader_stage_to_string(sh->Stage), sh->Name);
> _mesa_log("%s\n", sh->Source);
> }
>
> @@ -1227,29 +1343,46 @@ link_program(struct gl_context *ctx, struct gl_shader_program *shProg,
> /* Capture .shader_test files. */
> const char *capture_path = _mesa_get_shader_capture_path();
> if (shProg->Name != 0 && shProg->Name != ~0 && capture_path != NULL) {
> +
> FILE *file;
> - char *filename = ralloc_asprintf(NULL, "%s/%u.shader_test",
> - capture_path, shProg->Name);
> + char *filename = NULL;
> + char *fsource = NULL;
> + char *ftemp = NULL;
> +
> + asprintf(&fsource, "[require]\nGLSL%s >= %u.%02u\n",
> + shProg->IsES ? " ES" : "",
> + shProg->data->Version / 100, shProg->data->Version % 100);
> +
> + if (shProg->SeparateShader) {
> + ftemp = fsource;
> + asprintf(&fsource, "%sGL_ARB_separate_shader_objects\nSSO ENABLED\n",
> + ftemp);
> + free(ftemp);
> + }
> +
> + for (unsigned i = 0; i < shProg->NumShaders; i++) {
> + ftemp = fsource;
> + asprintf(&fsource, "%s\n[%s shader]\n%s\n", ftemp,
> + _mesa_shader_stage_to_string(shProg->Shaders[i]->Stage),
> + shProg->Shaders[i]->Source);
> + free(ftemp);
> + }
> +
> + char shabuf[64] = {"mylittlebunny"};
> + generate_sha1(fsource, shabuf);
> +
> + asprintf(&filename, "%s/%s_%u.shader_test", capture_path,
> + shabuf, shProg->Name);
> file = fopen(filename, "w");
> if (file) {
> - fprintf(file, "[require]\nGLSL%s >= %u.%02u\n",
> - shProg->IsES ? " ES" : "",
> - shProg->data->Version / 100, shProg->data->Version % 100);
> - if (shProg->SeparateShader)
> - fprintf(file, "GL_ARB_separate_shader_objects\nSSO ENABLED\n");
> - fprintf(file, "\n");
> -
> - for (unsigned i = 0; i < shProg->NumShaders; i++) {
> - fprintf(file, "[%s shader]\n%s\n",
> - _mesa_shader_stage_to_string(shProg->Shaders[i]->Stage),
> - shProg->Shaders[i]->Source);
> - }
> + fprintf(file, "%s", fsource);
> fclose(file);
> } else {
> _mesa_warning(ctx, "Failed to open %s", filename);
> }
>
> - ralloc_free(filename);
> + free(filename);
> + free(fsource);
> }
>
> if (shProg->data->LinkStatus == LINKING_FAILURE &&
> @@ -1265,13 +1398,13 @@ link_program(struct gl_context *ctx, struct gl_shader_program *shProg,
> GLuint i;
>
> printf("Link %u shaders in program %u: %s\n",
> - shProg->NumShaders, shProg->Name,
> - shProg->data->LinkStatus ? "Success" : "Failed");
> + shProg->NumShaders, shProg->Name,
> + shProg->data->LinkStatus ? "Success" : "Failed");
>
> for (i = 0; i < shProg->NumShaders; i++) {
> printf(" shader %u, stage %u\n",
> - shProg->Shaders[i]->Name,
> - shProg->Shaders[i]->Stage);
> + shProg->Shaders[i]->Name,
> + shProg->Shaders[i]->Stage);
> }
> }
> }
> @@ -1344,7 +1477,7 @@ void
> _mesa_active_program(struct gl_context *ctx, struct gl_shader_program *shProg,
> const char *caller)
> {
> - if ((shProg != NULL) && !shProg->data->LinkStatus) {
> + if ((shProg) && !shProg->data->LinkStatus) {
> _mesa_error(ctx, GL_INVALID_OPERATION,
> "%s(program %u not linked)", caller, shProg->Name);
> return;
> @@ -1678,7 +1811,7 @@ void GLAPIENTRY
> _mesa_GetObjectParameterfvARB(GLhandleARB object, GLenum pname,
> GLfloat *params)
> {
> - GLint iparams[1] = {0}; /* XXX is one element enough? */
> + GLint iparams[1] = {}; /* XXX is one element enough? */
> _mesa_GetObjectParameterivARB(object, pname, iparams);
> params[0] = (GLfloat) iparams[0];
> }
> @@ -1775,119 +1908,7 @@ _mesa_LinkProgram(GLuint programObj)
> link_program_error(ctx, shProg);
> }
>
> -#ifdef ENABLE_SHADER_CACHE
> -/**
> - * 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 char *
> -construct_name(const gl_shader_stage stage, const char *source,
> - const char *path)
> -{
> - char sha[64];
> - static const char *types[] = {
> - "VS", "TC", "TE", "GS", "FS", "CS",
> - };
> -
> - generate_sha1(source, sha);
> - return ralloc_asprintf(NULL, "%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)
> -{
> - static bool path_exists = true;
> - char *dump_path;
> - FILE *f;
> -
> - if (!path_exists)
> - return;
> -
> - dump_path = getenv("MESA_SHADER_DUMP_PATH");
> - if (!dump_path) {
> - path_exists = false;
> - return;
> - }
> -
> - char *name = construct_name(stage, source, dump_path);
> -
> - 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 (%s)", name,
> - strerror(errno));
> - }
> - ralloc_free(name);
> -}
> -
> -/**
> - * Read shader source code from a file.
> - * Useful for debugging to override an app's shader.
> - */
> -static GLcharARB *
> -read_shader(const gl_shader_stage stage, const char *source)
> -{
> - char *read_path;
> - static bool path_exists = true;
> - int len, shader_size = 0;
> - GLcharARB *buffer;
> - FILE *f;
> -
> - if (!path_exists)
> - return NULL;
>
> - read_path = getenv("MESA_SHADER_READ_PATH");
> - if (!read_path) {
> - path_exists = false;
> - return NULL;
> - }
> -
> - char *name = construct_name(stage, source, read_path);
> - f = fopen(name, "r");
> - ralloc_free(name);
> - if (!f)
> - return NULL;
> -
> - /* allocate enough room for the entire shader */
> - fseek(f, 0, SEEK_END);
> - shader_size = ftell(f);
> - rewind(f);
> - assert(shader_size);
> -
> - /* add one for terminating zero */
> - shader_size++;
> -
> - buffer = malloc(shader_size);
> - assert(buffer);
> -
> - len = fread(buffer, 1, shader_size, f);
> - buffer[len] = 0;
> -
> - fclose(f);
> -
> - return buffer;
> -}
> -
> -#endif /* ENABLE_SHADER_CACHE */
>
> /**
> * Called via glShaderSource() and glShaderSourceARB() API functions.
> @@ -1908,7 +1929,7 @@ shader_source(struct gl_context *ctx, GLuint shaderObj, GLsizei count,
> if (!sh)
> return;
>
> - if (string == NULL) {
> + if (!string) {
> _mesa_error(ctx, GL_INVALID_VALUE, "glShaderSourceARB");
> return;
> }
> @@ -1921,7 +1942,7 @@ shader_source(struct gl_context *ctx, GLuint shaderObj, GLsizei count,
> * last element will be set to the total length of the source code.
> */
> offsets = malloc(count * sizeof(GLint));
> - if (offsets == NULL) {
> + if (!offsets) {
> _mesa_error(ctx, GL_OUT_OF_MEMORY, "glShaderSourceARB");
> return;
> }
> @@ -1948,7 +1969,7 @@ shader_source(struct gl_context *ctx, GLuint shaderObj, GLsizei count,
> */
> totalLength = offsets[count - 1] + 2;
> source = malloc(totalLength * sizeof(GLcharARB));
> - if (source == NULL) {
> + if (!source) {
> free((GLvoid *) offsets);
> _mesa_error(ctx, GL_OUT_OF_MEMORY, "glShaderSourceARB");
> return;
> @@ -2241,7 +2262,7 @@ _mesa_GetProgramBinary(GLuint program, GLsizei bufSize, GLsizei *length,
> * Ensure that length always points to valid storage to avoid multiple NULL
> * pointer checks below.
> */
> - if (length == NULL)
> + if (!length)
> length = &length_dummy;
>
>
> @@ -2259,7 +2280,7 @@ _mesa_GetProgramBinary(GLuint program, GLsizei bufSize, GLsizei *length,
> return;
> }
>
> - if (ctx->Const.NumProgramBinaryFormats == 0) {
> + if (!ctx->Const.NumProgramBinaryFormats) {
> *length = 0;
> _mesa_error(ctx, GL_INVALID_OPERATION,
> "glGetProgramBinary(driver supports zero binary formats)");
> @@ -2854,7 +2875,7 @@ _mesa_UniformSubroutinesuiv(GLenum shadertype, GLsizei count,
> bool flushed = false;
> do {
> struct gl_uniform_storage *uni = p->sh.SubroutineUniformRemapTable[i];
> - if (uni == NULL) {
> + if (!uni) {
> i++;
> continue;
> }
> @@ -3050,7 +3071,7 @@ _mesa_shader_write_subroutine_index(struct gl_context *ctx,
> {
> int i, j;
>
> - if (p->sh.NumSubroutineUniformRemapTable == 0)
> + if (!p->sh.NumSubroutineUniformRemapTable)
> return;
>
> i = 0;
>
More information about the mesa-dev
mailing list