From 1f2c906e2a85bef6ed3a2e820e2004542b864c86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Foucault?= Date: Tue, 26 Aug 2025 12:46:46 +0200 Subject: [PATCH] GPU: Shader: Always mute line directives for GLSL This is motivated by the latest changes to the preprocessor which outputs a lot of line directives when code is generated or unrolled. In this case the reported line would be correct but not correctly displayed. Moreover the system of outputing hashes inside the `#line` directive proved to be incompatible with some compilers and tools (renderdoc). This commit always comments the line directives before compilation (solves the compatibility issue). When error logging, we then scan the commented directives to output the correct filename and source line. The log line is kept untouched and will show the correct final generated code that triggered the error. This also fixed the error line parsing for vulkan. Pull Request: https://projects.blender.org/blender/blender/pulls/145096 --- .../gpu/glsl_preprocess/glsl_preprocess.hh | 20 ++----- source/blender/gpu/intern/gpu_shader_log.cc | 54 +++++++++++++++++-- .../blender/gpu/intern/gpu_shader_private.hh | 6 ++- source/blender/gpu/metal/mtl_shader_log.mm | 5 +- source/blender/gpu/opengl/gl_shader.cc | 17 ++++-- source/blender/gpu/opengl/gl_shader_log.cc | 27 ++++------ source/blender/gpu/vulkan/vk_shader.cc | 16 +----- .../blender/gpu/vulkan/vk_shader_compiler.cc | 16 +++++- source/blender/gpu/vulkan/vk_shader_log.cc | 42 ++++++--------- 9 files changed, 116 insertions(+), 87 deletions(-) diff --git a/source/blender/gpu/glsl_preprocess/glsl_preprocess.hh b/source/blender/gpu/glsl_preprocess/glsl_preprocess.hh index 634a51839ba..c895f077e76 100644 --- a/source/blender/gpu/glsl_preprocess/glsl_preprocess.hh +++ b/source/blender/gpu/glsl_preprocess/glsl_preprocess.hh @@ -2244,23 +2244,9 @@ class Preprocessor { std::string filename = std::regex_replace(filepath, std::regex(R"((?:.*)\/(.*))"), "$1"); std::stringstream suffix; - suffix << "#line 1 "; -#ifdef __APPLE__ - /* For now, only Metal supports filename in line directive. - * There is no way to know the actual backend, so we assume Apple uses Metal. */ - /* TODO(fclem): We could make it work using a macro to choose between the filename and the hash - * at runtime. i.e.: `FILENAME_MACRO(12546546541, 'filename.glsl')` This should work for both - * MSL and GLSL. */ - if (!filename.empty()) { - suffix << "\"" << filename << "\""; - } -#else - uint64_t hash_value = metadata::hash(filename); - /* Fold the value so it fits the GLSL spec. */ - hash_value = (hash_value ^ (hash_value >> 32)) & (~uint64_t(0) >> 33); - suffix << std::to_string(uint64_t(hash_value)); -#endif - suffix << "\n"; + /* NOTE: This is not supported by GLSL. All line directives are muted at runtime and the + * sources are scanned after error reporting for the locating the muted line. */ + suffix << "#line 1 \"" << filename << "\"\n"; return suffix.str(); } diff --git a/source/blender/gpu/intern/gpu_shader_log.cc b/source/blender/gpu/intern/gpu_shader_log.cc index 8f11c9f60f0..872e7332b4d 100644 --- a/source/blender/gpu/intern/gpu_shader_log.cc +++ b/source/blender/gpu/intern/gpu_shader_log.cc @@ -143,7 +143,8 @@ void Shader::print_log(Span sources, { const char *src_line_end; found_line_id = false; - int src_line_index = 0; + /* Lines are 1 based. */ + int src_line_index = 1; while ((src_line_end = strchr(src_line, '\n'))) { if (src_line_index >= log_item.cursor.row) { found_line_id = true; @@ -208,10 +209,11 @@ void Shader::print_log(Span sources, row_in_file -= sources_end_line[source_index - 1]; } /* Print the filename the error line is coming from. */ - if (!log_item.cursor.file_name_and_error_line.is_empty()) { + if (!log_item.cursor.file_name_and_error_line.empty()) { char name_buf[256]; - log_item.cursor.file_name_and_error_line.substr(0, sizeof(name_buf) - 1) - .copy_utf8_truncated(name_buf); + std::string string = log_item.cursor.file_name_and_error_line.substr(0, + sizeof(name_buf) - 1); + StringRefNull(string).copy_utf8_truncated(name_buf); BLI_dynstr_appendf(dynstr, "%s%s: %s", info_col, name_buf, reset_col); } else if (source_index > 0) { @@ -316,6 +318,50 @@ int GPULogParser::parse_number(const char *log_line, const char **r_new_position return int(strtol(log_line, const_cast(r_new_position), 10)); } +size_t GPULogParser::line_start_get(StringRefNull source_combined, size_t target_line) +{ + size_t cursor = 0; + size_t current_line = 1; + for (char c : source_combined) { + if (current_line >= target_line) { + return cursor + 1; + } + if (c == '\n') { + current_line++; + } + cursor++; + } + return -1; +} + +StringRef GPULogParser::filename_get(StringRefNull source_combined, size_t pos) +{ + StringRef sub_str = source_combined.substr(0, pos); + StringRefNull directive = "#line 1 \""; + size_t nearest_line_directive = sub_str.rfind(directive); + if (nearest_line_directive != std::string::npos) { + size_t start_of_file_name = nearest_line_directive + directive.size(); + size_t end_of_file_name = sub_str.find('\"', start_of_file_name); + if (end_of_file_name != std::string::npos) { + return sub_str.substr(start_of_file_name, end_of_file_name - start_of_file_name); + } + } + return {}; +} + +/* Original source file line. Found by looking up #line directives. */ +size_t GPULogParser::source_line_get(StringRefNull source_combined, size_t pos) +{ + StringRef sub_str = source_combined.substr(0, pos); + StringRefNull directive = "#line "; + size_t nearest_line_directive = sub_str.rfind(directive); + size_t line_count = 1; + if (nearest_line_directive != std::string::npos) { + sub_str = sub_str.substr(nearest_line_directive + directive.size()); + line_count = std::stoll(sub_str) - 1; + } + return line_count + std::count(sub_str.begin(), sub_str.end(), '\n'); +} /** \} */ /* -------------------------------------------------------------------- */ diff --git a/source/blender/gpu/intern/gpu_shader_private.hh b/source/blender/gpu/intern/gpu_shader_private.hh index 072efd3f204..72729453bbf 100644 --- a/source/blender/gpu/intern/gpu_shader_private.hh +++ b/source/blender/gpu/intern/gpu_shader_private.hh @@ -301,7 +301,7 @@ struct LogCursor { int source = -1; int row = -1; int column = -1; - StringRef file_name_and_error_line = {}; + std::string file_name_and_error_line; }; struct GPULogItem { @@ -327,6 +327,10 @@ class GPULogParser { bool at_any(const char *log_line, const StringRef chars) const; int parse_number(const char *log_line, const char **r_new_position) const; + static size_t line_start_get(StringRefNull source_combined, size_t target_line); + static StringRef filename_get(StringRefNull source_combined, size_t pos); + static size_t source_line_get(StringRefNull source_combined, size_t pos); + MEM_CXX_CLASS_ALLOC_FUNCS("GPULogParser"); }; diff --git a/source/blender/gpu/metal/mtl_shader_log.mm b/source/blender/gpu/metal/mtl_shader_log.mm index 4090c82abcf..4fb32e3d0df 100644 --- a/source/blender/gpu/metal/mtl_shader_log.mm +++ b/source/blender/gpu/metal/mtl_shader_log.mm @@ -35,8 +35,9 @@ const char *MTLLogParser::parse_line(const char *source_combined, log_line = error_line_number_end; log_line = skip_separators(log_line, ": "); log_item.cursor.column = parse_number(log_line, &error_line_number_end); - /* For some reason the column is off by one. */ - log_item.cursor.column--; + /* For some reason the row and column is 0 based in C++ / Metal compiler. */ + log_item.cursor.row -= 1; + log_item.cursor.column -= 1; log_line = error_line_number_end; /* Simply copy the start of the error line since it is already in the format we want. */ log_item.cursor.file_name_and_error_line = StringRef(name_start, error_line_number_end); diff --git a/source/blender/gpu/opengl/gl_shader.cc b/source/blender/gpu/opengl/gl_shader.cc index 5ee87a0ce1f..001f92da7f2 100644 --- a/source/blender/gpu/opengl/gl_shader.cc +++ b/source/blender/gpu/opengl/gl_shader.cc @@ -35,6 +35,9 @@ #include #include + +#include + #ifdef WIN32 # define popen _popen # define pclose _pclose @@ -1268,11 +1271,17 @@ GLuint GLShader::create_shader_stage(GLenum gl_stage, return 0; } - Array c_str_sources(sources.size()); - for (const int i : sources.index_range()) { - c_str_sources[i] = sources[i].c_str(); + std::string concat_source = fmt::to_string(fmt::join(sources, "")); + + /* Patch line directives so that we can make error reporting consistent. */ + size_t start_pos = 0; + while ((start_pos = concat_source.find("#line ", start_pos)) != std::string::npos) { + concat_source[start_pos] = '/'; + concat_source[start_pos + 1] = '/'; } - glShaderSource(shader, c_str_sources.size(), c_str_sources.data(), nullptr); + + const char *str_ptr = concat_source.c_str(); + glShaderSource(shader, 1, &str_ptr, nullptr); glCompileShader(shader); GLint status; diff --git a/source/blender/gpu/opengl/gl_shader_log.cc b/source/blender/gpu/opengl/gl_shader_log.cc index 88899bc4e4d..c28bb190882 100644 --- a/source/blender/gpu/opengl/gl_shader_log.cc +++ b/source/blender/gpu/opengl/gl_shader_log.cc @@ -58,24 +58,15 @@ const char *GLLogParser::parse_line(const char *source_combined, } } - /* TODO: Temporary fix for new line directive. Eventually this whole parsing should be done in - * C++ with regex for simplicity. */ - if (log_item.cursor.source != -1) { - StringRefNull src(source_combined); - std::string needle = std::string("#line 1 ") + std::to_string(log_item.cursor.source); - - int64_t file_start = src.find(needle); - if (file_start == -1) { - /* Can be generated code or wrapper code outside of the main sources. */ - log_item.cursor.row = -1; - } - else { - StringRef previous_sources(source_combined, file_start); - for (const char c : previous_sources) { - if (c == '\n') { - log_item.cursor.row++; - } - } + if (log_item.cursor.row != -1) { + /* Get to the wanted line. */ + size_t line_start_character = line_start_get(source_combined, log_item.cursor.row); + StringRef filename = filename_get(source_combined, line_start_character); + size_t line_number = source_line_get(source_combined, line_start_character); + log_item.cursor.file_name_and_error_line = std::string(filename) + ':' + + std::to_string(line_number); + if (log_item.cursor.column != -1) { + log_item.cursor.file_name_and_error_line += ':' + std::to_string(log_item.cursor.column + 1); } } diff --git a/source/blender/gpu/vulkan/vk_shader.cc b/source/blender/gpu/vulkan/vk_shader.cc index f888acc43a5..f0fe3e6f0c1 100644 --- a/source/blender/gpu/vulkan/vk_shader.cc +++ b/source/blender/gpu/vulkan/vk_shader.cc @@ -485,21 +485,7 @@ static std::string main_function_wrapper(std::string &pre_main, std::string &pos static std::string combine_sources(Span sources) { - std::string result = fmt::to_string(fmt::join(sources, "")); - /* Renderdoc step-by-step debugger cannot be used when using the #line directive. The indexed - * based is not supported as it doesn't make sense in Vulkan and Blender misuses this to store a - * hash. The filename based directive cannot be used as it cannot find the actual file on disk - * and state is set incorrectly. - * - * When running in renderdoc we scramble `#line` into `//ine` to work around these limitation. */ - if (G.debug & G_DEBUG_GPU_RENDERDOC) { - size_t start_pos = 0; - while ((start_pos = result.find("#line ", start_pos)) != std::string::npos) { - result[start_pos] = '/'; - result[start_pos + 1] = '/'; - } - } - return result; + return fmt::to_string(fmt::join(sources, "")); } VKShader::VKShader(const char *name) : Shader(name) diff --git a/source/blender/gpu/vulkan/vk_shader_compiler.cc b/source/blender/gpu/vulkan/vk_shader_compiler.cc index f9e1a5b3c65..c9ea8e19413 100644 --- a/source/blender/gpu/vulkan/vk_shader_compiler.cc +++ b/source/blender/gpu/vulkan/vk_shader_compiler.cc @@ -162,6 +162,17 @@ static StringRef to_stage_name(shaderc_shader_kind stage) return "unknown stage"; } +static std::string patch_line_directives(std::string source) +{ + /* Patch line directives so that we can make error reporting consistent. */ + size_t start_pos = 0; + while ((start_pos = source.find("#line ", start_pos)) != std::string::npos) { + source[start_pos] = '/'; + source[start_pos + 1] = '/'; + } + return source; +} + static bool compile_ex(shaderc::Compiler &compiler, VKShader &shader, shaderc_shader_kind stage, @@ -201,9 +212,12 @@ static bool compile_ex(shaderc::Compiler &compiler, options.SetGenerateDebugInfo(); } + /* Removes line directive. */ + std::string sources = patch_line_directives(shader_module.combined_sources); + std::string full_name = shader.name_get() + "_" + to_stage_name(stage); shader_module.compilation_result = compiler.CompileGlslToSpv( - shader_module.combined_sources, stage, full_name.c_str(), options); + sources, stage, full_name.c_str(), options); bool compilation_succeeded = shader_module.compilation_result.GetCompilationStatus() == shaderc_compilation_status_success; if (compilation_succeeded) { diff --git a/source/blender/gpu/vulkan/vk_shader_log.cc b/source/blender/gpu/vulkan/vk_shader_log.cc index 6ba5b0507b2..6b451dffbd7 100644 --- a/source/blender/gpu/vulkan/vk_shader_log.cc +++ b/source/blender/gpu/vulkan/vk_shader_log.cc @@ -16,13 +16,8 @@ const char *VKLogParser::parse_line(const char *source_combined, const char *log_line, GPULogItem &log_item) { - if (at_number(log_line)) { - /* Parse error file. */ - const char *error_line_number_end; - log_item.cursor.source = parse_number(log_line, &error_line_number_end); - log_line = error_line_number_end; - } - + /* Shader name. */ + log_line = skip_name(log_line); log_line = skip_separators(log_line, ":"); /* Parse error line & char numbers. */ @@ -30,6 +25,12 @@ const char *VKLogParser::parse_line(const char *source_combined, const char *error_line_number_end; log_item.cursor.row = parse_number(log_line, &error_line_number_end); log_line = error_line_number_end; + log_line = skip_separators(log_line, ": "); + } + if (at_number(log_line)) { + const char *number_end; + log_item.cursor.column = parse_number(log_line, &number_end); + log_line = number_end; } log_line = skip_separators(log_line, ": "); @@ -37,24 +38,15 @@ const char *VKLogParser::parse_line(const char *source_combined, log_line = skip_severity_keyword(log_line, log_item); log_line = skip_separators(log_line, ": "); - /* TODO: Temporary fix for new line directive. Eventually this whole parsing should be done in - * C++ with regex for simplicity. */ - if (log_item.cursor.source != -1) { - StringRefNull src(source_combined); - std::string needle = std::string("#line 1 ") + std::to_string(log_item.cursor.source); - - int64_t file_start = src.find(needle); - if (file_start == -1) { - /* Can be generated code or wrapper code outside of the main sources. */ - log_item.cursor.row = -1; - } - else { - StringRef previous_sources(source_combined, file_start); - for (const char c : previous_sources) { - if (c == '\n') { - log_item.cursor.row++; - } - } + if (log_item.cursor.row != -1) { + /* Get to the wanted line. */ + size_t line_start_character = line_start_get(source_combined, log_item.cursor.row); + StringRef filename = filename_get(source_combined, line_start_character); + size_t line_number = source_line_get(source_combined, line_start_character); + log_item.cursor.file_name_and_error_line = std::string(filename) + ':' + + std::to_string(line_number); + if (log_item.cursor.column != -1) { + log_item.cursor.file_name_and_error_line += ':' + std::to_string(log_item.cursor.column + 1); } }