From 043c5bf966a276c02c536846f44a1335e082789c Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Thu, 29 Jul 2021 14:13:27 -0700 Subject: [PATCH] intel/compiler: Add id parameter to shader_debug_log callback There are two problems with the current architecture. In OpenGL, the id is supposed to be a unique identifier for a particular log source. This is done so that applications can (theoretically) filter particular log messages. The debug callback infrastructure in Mesa assigns a uniqe value when a value of 0 is passed in. This causes the id to get set once to a unique value for each message. By passing a stack variable that is initialized to 0 on every call, every time the same message is logged, it will have a different id. This isn't great, but it's also not catastrophic. When threaded shader compiles are used, the id *pointer* is saved and dereferenced at a possibly much later time on a possibly different thread. This causes one thread to access the stack from a different thread... and that stack frame might not be valid any more. :( This fixes shader-db crashes of various kinds on Iris with threaded shader compiles enabled. Fixes: 42c34e1ac8d ("iris: Enable threaded shader compilation") Reviewed-by: Matt Turner Reviewed-by: Anuj Phogat Part-of: --- src/gallium/drivers/crocus/crocus_screen.c | 5 ++--- src/gallium/drivers/iris/iris_screen.c | 5 ++--- src/intel/compiler/brw_compiler.h | 7 +++++- src/intel/compiler/brw_fs_generator.cpp | 26 +++++++++++----------- src/intel/compiler/brw_vec4_generator.cpp | 14 ++++++------ src/intel/vulkan/anv_device.c | 2 +- src/mesa/drivers/dri/i965/brw_screen.c | 5 ++--- 7 files changed, 33 insertions(+), 31 deletions(-) diff --git a/src/gallium/drivers/crocus/crocus_screen.c b/src/gallium/drivers/crocus/crocus_screen.c index 2647fb923ee..9a29ada93bd 100644 --- a/src/gallium/drivers/crocus/crocus_screen.c +++ b/src/gallium/drivers/crocus/crocus_screen.c @@ -674,17 +674,16 @@ crocus_get_default_l3_config(const struct intel_device_info *devinfo, } static void -crocus_shader_debug_log(void *data, const char *fmt, ...) +crocus_shader_debug_log(void *data, unsigned *id, const char *fmt, ...) { struct pipe_debug_callback *dbg = data; - unsigned id = 0; va_list args; if (!dbg->debug_message) return; va_start(args, fmt); - dbg->debug_message(dbg->data, &id, PIPE_DEBUG_TYPE_SHADER_INFO, fmt, args); + dbg->debug_message(dbg->data, id, PIPE_DEBUG_TYPE_SHADER_INFO, fmt, args); va_end(args); } diff --git a/src/gallium/drivers/iris/iris_screen.c b/src/gallium/drivers/iris/iris_screen.c index ab868495ad6..612c0c6a5bd 100644 --- a/src/gallium/drivers/iris/iris_screen.c +++ b/src/gallium/drivers/iris/iris_screen.c @@ -717,17 +717,16 @@ iris_get_default_l3_config(const struct intel_device_info *devinfo, } static void -iris_shader_debug_log(void *data, const char *fmt, ...) +iris_shader_debug_log(void *data, unsigned *id, const char *fmt, ...) { struct pipe_debug_callback *dbg = data; - unsigned id = 0; va_list args; if (!dbg->debug_message) return; va_start(args, fmt); - dbg->debug_message(dbg->data, &id, PIPE_DEBUG_TYPE_SHADER_INFO, fmt, args); + dbg->debug_message(dbg->data, id, PIPE_DEBUG_TYPE_SHADER_INFO, fmt, args); va_end(args); } diff --git a/src/intel/compiler/brw_compiler.h b/src/intel/compiler/brw_compiler.h index e7c7278306f..5f07e9cc435 100644 --- a/src/intel/compiler/brw_compiler.h +++ b/src/intel/compiler/brw_compiler.h @@ -69,7 +69,7 @@ struct brw_compiler { struct ra_class *aligned_bary_class; } fs_reg_sets[3]; - void (*shader_debug_log)(void *, const char *str, ...) PRINTFLIKE(2, 3); + void (*shader_debug_log)(void *, unsigned *id, const char *str, ...) PRINTFLIKE(3, 4); void (*shader_perf_log)(void *, const char *str, ...) PRINTFLIKE(2, 3); bool scalar_stage[MESA_ALL_SHADER_STAGES]; @@ -121,6 +121,11 @@ struct brw_compiler { bool indirect_ubos_use_sampler; }; +#define brw_shader_debug_log(compiler, data, fmt, ... ) do { \ + static unsigned id = 0; \ + compiler->shader_debug_log(data, &id, fmt, ##__VA_ARGS__); \ +} while (0) + /** * We use a constant subgroup size of 32. It really only needs to be a * maximum and, since we do SIMD32 for compute shaders in some cases, it diff --git a/src/intel/compiler/brw_fs_generator.cpp b/src/intel/compiler/brw_fs_generator.cpp index 068e4191534..8740c7a65f3 100644 --- a/src/intel/compiler/brw_fs_generator.cpp +++ b/src/intel/compiler/brw_fs_generator.cpp @@ -2794,19 +2794,19 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width, #endif assert(validated); - compiler->shader_debug_log(log_data, - "%s SIMD%d shader: %d inst, %d loops, %u cycles, " - "%d:%d spills:fills, %u sends, " - "scheduled with mode %s, " - "Promoted %u constants, " - "compacted %d to %d bytes.", - _mesa_shader_stage_to_abbrev(stage), - dispatch_width, before_size / 16 - nop_count, - loop_count, perf.latency, - spill_count, fill_count, send_count, - shader_stats.scheduler_mode, - shader_stats.promoted_constants, - before_size, after_size); + brw_shader_debug_log(compiler, log_data, + "%s SIMD%d shader: %d inst, %d loops, %u cycles, " + "%d:%d spills:fills, %u sends, " + "scheduled with mode %s, " + "Promoted %u constants, " + "compacted %d to %d bytes.", + _mesa_shader_stage_to_abbrev(stage), + dispatch_width, before_size / 16 - nop_count, + loop_count, perf.latency, + spill_count, fill_count, send_count, + shader_stats.scheduler_mode, + shader_stats.promoted_constants, + before_size, after_size); if (stats) { stats->dispatch_width = dispatch_width; stats->instructions = before_size / 16 - nop_count; diff --git a/src/intel/compiler/brw_vec4_generator.cpp b/src/intel/compiler/brw_vec4_generator.cpp index 1d20d87b2a0..e1fb00fcfaa 100644 --- a/src/intel/compiler/brw_vec4_generator.cpp +++ b/src/intel/compiler/brw_vec4_generator.cpp @@ -2263,13 +2263,13 @@ generate_code(struct brw_codegen *p, ralloc_free(disasm_info); assert(validated); - compiler->shader_debug_log(log_data, - "%s vec4 shader: %d inst, %d loops, %u cycles, " - "%d:%d spills:fills, %u sends, " - "compacted %d to %d bytes.", - stage_abbrev, before_size / 16, - loop_count, perf.latency, spill_count, - fill_count, send_count, before_size, after_size); + brw_shader_debug_log(compiler, log_data, + "%s vec4 shader: %d inst, %d loops, %u cycles, " + "%d:%d spills:fills, %u sends, " + "compacted %d to %d bytes.", + stage_abbrev, before_size / 16, + loop_count, perf.latency, spill_count, + fill_count, send_count, before_size, after_size); if (stats) { stats->dispatch_width = 0; stats->instructions = before_size / 16; diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c index 52d819aad6f..84c34e0e31f 100644 --- a/src/intel/vulkan/anv_device.c +++ b/src/intel/vulkan/anv_device.c @@ -85,7 +85,7 @@ static const driOptionDescription anv_dri_options[] = { #endif static void -compiler_debug_log(void *data, const char *fmt, ...) +compiler_debug_log(void *data, UNUSED unsigned *id, const char *fmt, ...) { char str[MAX_DEBUG_MESSAGE_LENGTH]; struct anv_device *device = (struct anv_device *)data; diff --git a/src/mesa/drivers/dri/i965/brw_screen.c b/src/mesa/drivers/dri/i965/brw_screen.c index dad4bc29463..20ca5c21451 100644 --- a/src/mesa/drivers/dri/i965/brw_screen.c +++ b/src/mesa/drivers/dri/i965/brw_screen.c @@ -2472,14 +2472,13 @@ set_max_gl_versions(struct brw_screen *screen) } static void -shader_debug_log_mesa(void *data, const char *fmt, ...) +shader_debug_log_mesa(void *data, unsigned *msg_id, const char *fmt, ...) { struct brw_context *brw = (struct brw_context *)data; va_list args; va_start(args, fmt); - GLuint msg_id = 0; - _mesa_gl_vdebugf(&brw->ctx, &msg_id, + _mesa_gl_vdebugf(&brw->ctx, msg_id, MESA_DEBUG_SOURCE_SHADER_COMPILER, MESA_DEBUG_TYPE_OTHER, MESA_DEBUG_SEVERITY_NOTIFICATION, fmt, args);