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

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();
   }
 

source/blender/gpu/intern/gpu_shader_log.cc

@@ -143,7 +143,8 @@ void Shader::print_log(Span<StringRefNull> 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<StringRefNull> 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<char **>(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');
+}
 /** \} */
 
 /* -------------------------------------------------------------------- */

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");
 };
 

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);

source/blender/gpu/opengl/gl_shader.cc

@@ -35,6 +35,9 @@
 
 #include <sstream>
 #include <stdio.h>
+
+#include <fmt/format.h>
+
 #ifdef WIN32
 #  define popen _popen
 #  define pclose _pclose
@@ -1268,11 +1271,17 @@ GLuint GLShader::create_shader_stage(GLenum gl_stage,
     return 0;
   }
 
-  Array<const char *, 16> 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;

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);
     }
   }
 

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<StringRefNull> 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)

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) {

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);
     }
   }