From 92f2004c4b519cbdd12ef2d93096f13ab3e41245 Mon Sep 17 00:00:00 2001 From: Ben Doherty Date: Mon, 29 Jun 2020 11:21:56 -0700 Subject: [PATCH] Improve matc error reporting (#2741) --- .../filamat/include/filamat/IncludeCallback.h | 44 ++-- .../filamat/include/filamat/MaterialBuilder.h | 14 +- libs/filamat/src/Includes.cpp | 139 ++++++++++++- libs/filamat/src/Includes.h | 20 +- libs/filamat/src/MaterialBuilder.cpp | 25 ++- libs/filamat/src/shaders/CodeGenerator.cpp | 4 + libs/filamat/src/shaders/ShaderGenerator.cpp | 3 +- libs/filamat/tests/MockIncluder.h | 11 +- libs/filamat/tests/test_includes.cpp | 193 ++++++++++++++++-- libs/utils/include/utils/CString.h | 1 + shaders/CMakeLists.txt | 9 +- tools/glslminifier/src/main.cpp | 27 ++- tools/matc/src/matc/DirIncluder.cpp | 18 +- tools/matc/src/matc/DirIncluder.h | 3 +- tools/matc/src/matc/MaterialCompiler.cpp | 9 +- tools/matc/tests/test_includer.cpp | 28 ++- 16 files changed, 460 insertions(+), 88 deletions(-) diff --git a/libs/filamat/include/filamat/IncludeCallback.h b/libs/filamat/include/filamat/IncludeCallback.h index 6be3f320f0..659ba2895d 100644 --- a/libs/filamat/include/filamat/IncludeCallback.h +++ b/libs/filamat/include/filamat/IncludeCallback.h @@ -24,34 +24,46 @@ namespace filamat { struct IncludeResult { - /** - * The full contents of the include file. This may contain additional, recursive include - * directives. - */ - utils::CString source; + // The include name of the root file, as if it were being included. + // I.e., 'foobar.h' in the case of #include "foobar.h" + const utils::CString includeName; - /** - * The name of the include file. This gets passed as "includerName" for any includes inside - * of source. This field isn't used by the include system; it's up to the callback to give - * meaning to this value and interpret it accordingly. - * In the case of DirIncluder, this is an empty string to represent the root include file, - * and a canonical path for subsequent included files. - */ + // The following fields should be filled out by the IncludeCallback when processing an include, + // or when calling resolveIncludes for the root file. + + // The full contents of the include file. This may contain additional, recursive include + // directives. + utils::CString text; + + // The line number for the first line of text (first line is 0). + size_t lineNumberOffset = 0; + + // The name of the include file. This gets passed as "includerName" for any includes inside of + // source. This field isn't used by the include system; it's up to the callback to give meaning + // to this value and interpret it accordingly. In the case of DirIncluder, this is an empty + // string to represent the root include file, and a canonical path for subsequent included + // files. utils::CString name; }; /** * A callback invoked by the include system when an #include "file.h" directive is found. - * @param headerName is the name referenced within the quotes. - * @param includerName is the value that was given to IncludeResult.name for this source file, or + * + * For example, if a file main.h includes file.h on line 10, then IncludeCallback would be called + * with the following: + * includeCallback("main.h", {.includeName = "file.h" }) + * It's then up to the IncludeCallback to fill out the .text, .name, and (optionally) + * lineNumberOffset fields. + * + * @param includedBy is the value that was given to IncludeResult.name for this source file, or * the empty string for the root source file. * @param result is the IncludeResult that the callback should fill out. * @return true, if the include was resolved successfully, false otherwise. + * * For an example of implementing this callback, see tools/matc/src/matc/DirIncluder.h. */ using IncludeCallback = std::function; } // namespace filamat diff --git a/libs/filamat/include/filamat/MaterialBuilder.h b/libs/filamat/include/filamat/MaterialBuilder.h index fb3dc690be..19bf4e97b4 100644 --- a/libs/filamat/include/filamat/MaterialBuilder.h +++ b/libs/filamat/include/filamat/MaterialBuilder.h @@ -195,6 +195,9 @@ public: //! Set the name of this material. MaterialBuilder& name(const char* name) noexcept; + //! Set the file name of this material file. Used in error reporting. + MaterialBuilder& fileName(const char* name) noexcept; + //! Set the shading model. MaterialBuilder& shading(Shading shading) noexcept; @@ -260,6 +263,10 @@ public: * postProcess.color = float4(1.0); * } * ~~~~~ + * + * @param code The source code of the material. + * @param line The line number offset of the material, where 0 is the first line. Used for error + * reporting */ MaterialBuilder& material(const char* code, size_t line = 0) noexcept; @@ -291,6 +298,10 @@ public: * * } * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + + * @param code The source code of the material. + * @param line The line number offset of the material, where 0 is the first line. Used for error + * reporting */ MaterialBuilder& materialVertex(const char* code, size_t line = 0) noexcept; @@ -509,6 +520,7 @@ private: bool isLit() const noexcept { return mShading != filament::Shading::UNLIT; } utils::CString mMaterialName; + utils::CString mFileName; class ShaderCode { public: @@ -519,7 +531,7 @@ private: } // Resolve all the #include directives, returns true if successful. - bool resolveIncludes(IncludeCallback callback) noexcept; + bool resolveIncludes(IncludeCallback callback, const utils::CString& fileName) noexcept; const utils::CString& getResolved() const noexcept { assert(mIncludesResolved); diff --git a/libs/filamat/src/Includes.cpp b/libs/filamat/src/Includes.cpp index 93cb289976..0a4e5ccedc 100644 --- a/libs/filamat/src/Includes.cpp +++ b/libs/filamat/src/Includes.cpp @@ -17,6 +17,8 @@ #include "Includes.h" #include +#include +#include #include @@ -26,45 +28,151 @@ static bool isWhitespace(char c) { return (c == ' ' || c == '\f' || c == '\n' || c == '\r' || c == '\t' || c == '\v'); } -bool resolveIncludes(const utils::CString& rootName, utils::CString& source, - IncludeCallback callback, size_t depth) { +bool resolveIncludes(IncludeResult& root, IncludeCallback callback, + const ResolveOptions& options, size_t depth) { if (depth > 30) { // This is probably an include cycle. Stop here and report an error so we don't overflow. utils::slog.e << "Include depth > 30. Include cycle?" << utils::io::endl; return false; } - std::vector includes = parseForIncludes(source); - while (!includes.empty()) { + const size_t lineNumberOffset = root.lineNumberOffset; + utils::CString& text = root.text; + + std::vector includes = parseForIncludes(text); + bool sourceDirty = false; + + // If we weren't given an include name, use "0", which is default when no #line directives are + // used. + const char* rootIncludeName = root.includeName.empty() ? "0" : root.includeName.c_str(); + + auto insertLineDirective = [&options](utils::io::ostream& stream, size_t line, const char* filename) { + if (options.insertLineDirectiveCheck) { + stream << "#if defined(GL_GOOGLE_cpp_style_line_directive)\n"; + // The #endif itself will count as a line, so subtact 1. + line--; + } + stream << "#line " << line << " \"" << filename << '\"'; + if (options.insertLineDirectiveCheck) { + stream << "\n#endif"; + } + }; + + // The #line directive must be on its own line and works like so: + // #line 10 "file.h" + // any code on this line is now considered line 10 of file.h + + // Add #line directives before / after each #include. We work backwards, otherwise we'd + // invalidate the offsets in FoundInclude. + for (auto it = includes.rbegin(); it < includes.rend() && options.insertLineDirectives; ++it) { + const auto include = *it; + + // Remember that text editors consider the first line of a file to be line 1. + // Consider the following file, called "root.h": + // 1 + // 2 #include "foo.h" + // 3 + + // We want to insert an opening and closing #line directive: + // 1 + // #line 1 "foo.h" + // 2 #include "foo.h" + // #line 3 "root.h" + // 3 + + // We want to insert a closing directive with a line number of 3. + // In this example, include.line is 2 and lineNumberOffset is 0. + // So, the math works out as such: + const size_t lineDirectiveLine = include.line + lineNumberOffset + 1; + + utils::io::sstream closingDirective; + + // This first newline is to ensure that the #line directive falls on a fresh line. + closingDirective << '\n'; + + // If there's a newline after the include, we'll use that to terminate the #line directive. + const size_t newlineCharacter = include.startPosition + include.length; + if (text.length() > newlineCharacter && text[newlineCharacter] == '\n') { + insertLineDirective(closingDirective, lineDirectiveLine, rootIncludeName); + } else { + // If there isn't one, be sure to add one. The included source might not have an + // newline at the end of the file. + + // We subtract 1 to handle additional code after the include directive. + // E.g., this include statement: + // #include "foobar.h" more code on same line + // + // should get translated to: + // #line 1 + // #include "foobar.h" + // #line 1 + // more code on same line + + insertLineDirective(closingDirective, lineDirectiveLine - 1, rootIncludeName); + closingDirective << '\n'; + } + + text.insert(include.startPosition + include.length, utils::CString(closingDirective.c_str())); + + // The included source always starts on line 1. + utils::io::sstream openingDirective; + insertLineDirective(openingDirective, 1, include.name.c_str()); + openingDirective << '\n'; + text.insert(include.startPosition, utils::CString(openingDirective.c_str())); + sourceDirty = true; + } + + // Add a line directive on the first line for the root include. + if (options.insertLineDirectives && depth == 0) { + utils::io::sstream lineDirective; + insertLineDirective(lineDirective, lineNumberOffset + 1, rootIncludeName); + lineDirective << '\n'; + text.insert(0, utils::CString(lineDirective.c_str())); + sourceDirty = true; + } + + // Re-parse for includes. If we've inserted any #line directives, then the line numbers have + // changed. + if (UTILS_LIKELY(sourceDirty)) { + includes = parseForIncludes(text); + } + + while (!includes.empty() && options.resolveIncludes) { const auto include = includes[0]; // Ask the includer to resolve this include. if (!callback) { return false; } - IncludeResult result; - if (!callback(include.name, rootName, result)) { + IncludeResult resolved { + .includeName = include.name + }; + if (!callback(root.name, resolved)) { utils::slog.e << "The included file \"" << include.name.c_str() << "\" could not be found." << utils::io::endl; return false; } // Recursively resolve all of its includes. - if (!resolveIncludes(result.name, result.source, callback, depth + 1)) { + if (!resolveIncludes(resolved, callback, options, depth + 1)) { return false; } - source.replace(include.startPosition, include.length, result.source); + text.replace(include.startPosition, include.length, resolved.text); - includes = parseForIncludes(source); + includes = parseForIncludes(text); } return true; } std::vector parseForIncludes(const utils::CString& source) { - std::string sourceString = source.c_str(); std::vector results; + if (source.empty()) { + return results; + } + std::string sourceString = source.c_str(); + size_t result = sourceString.find("#include"); while(result != std::string::npos) { @@ -102,7 +210,16 @@ std::vector parseForIncludes(const utils::CString& source) { // Grab the include name. const auto includeName = sourceString.substr(nameStart, nameEnd - nameStart + 1); - results.push_back({utils::CString(includeName.c_str()), includeStart, includeEnd - includeStart + 1}); + // Calculate the line number of the include. + size_t lineNumber = 1; + for (size_t i = 0; i < includeStart; i++) { + if (source[i] == '\n') { + lineNumber++; + } + } + + results.push_back({utils::CString(includeName.c_str()), includeStart, + includeEnd - includeStart + 1, lineNumber}); // Find next occurrence. result = sourceString.find("#include", result); diff --git a/libs/filamat/src/Includes.h b/libs/filamat/src/Includes.h index 5c1803fa90..2a75ede7c1 100644 --- a/libs/filamat/src/Includes.h +++ b/libs/filamat/src/Includes.h @@ -25,16 +25,30 @@ namespace filamat { -// Recursively handle all the includes inside of source. +struct ResolveOptions { + // If true, insert #line directives before / after each include. + bool insertLineDirectives = false; + + // Surrounds line directives with #if defined(GL_GOOGLE_cpp_style_line_directive) + // Some drivers may complain about the use of cpp style #line directives if they don't support + // it. + bool insertLineDirectiveCheck = false; + + // If true, process #include directives. + bool resolveIncludes = true; +}; + +// Recursively handle all the includes inside of root. // Returns true if all includes were handled successfully, false otherwise. // callback may be null, in which case any #include directives found will result in a failure. -bool resolveIncludes(const utils::CString& rootName, utils::CString& source, - IncludeCallback callback, size_t depth = 0); +bool resolveIncludes(IncludeResult& root, IncludeCallback callback, + const ResolveOptions& options, size_t depth = 0); struct FoundInclude { utils::CString name; size_t startPosition; size_t length; + size_t line; // the line number the include was found on (first line is 1) }; std::vector parseForIncludes(const utils::CString& source); diff --git a/libs/filamat/src/MaterialBuilder.cpp b/libs/filamat/src/MaterialBuilder.cpp index 069fd2323d..12c4666cde 100644 --- a/libs/filamat/src/MaterialBuilder.cpp +++ b/libs/filamat/src/MaterialBuilder.cpp @@ -132,6 +132,11 @@ MaterialBuilder& MaterialBuilder::name(const char* name) noexcept { return *this; } +MaterialBuilder& MaterialBuilder::fileName(const char* fileName) noexcept { + mFileName = CString(fileName); + return *this; +} + MaterialBuilder& MaterialBuilder::material(const char* code, size_t line) noexcept { mMaterialCode.setUnresolved(CString(code)); mMaterialCode.setLineOffset(line); @@ -512,11 +517,23 @@ bool MaterialBuilder::checkLiteRequirements() noexcept { return true; } -bool MaterialBuilder::ShaderCode::resolveIncludes(IncludeCallback callback) noexcept { +bool MaterialBuilder::ShaderCode::resolveIncludes(IncludeCallback callback, + const utils::CString& fileName) noexcept { if (!mCode.empty()) { - if (!::filamat::resolveIncludes(utils::CString(""), mCode, callback)) { + ResolveOptions options { + .insertLineDirectives = true, + .insertLineDirectiveCheck = true + }; + IncludeResult source { + .includeName = fileName, + .text = mCode, + .lineNumberOffset = getLineOffset(), + .name = utils::CString("") + }; + if (!::filamat::resolveIncludes(source, callback, options)) { return false; } + mCode = source.text; } mIncludesResolved = true; @@ -708,8 +725,8 @@ Package MaterialBuilder::build() noexcept { } // Resolve all the #include directives within user code. - if (!mMaterialCode.resolveIncludes(mIncludeCallback) || - !mMaterialVertexCode.resolveIncludes(mIncludeCallback)) { + if (!mMaterialCode.resolveIncludes(mIncludeCallback, mFileName) || + !mMaterialVertexCode.resolveIncludes(mIncludeCallback, mFileName)) { return Package::invalidPackage(); } diff --git a/libs/filamat/src/shaders/CodeGenerator.cpp b/libs/filamat/src/shaders/CodeGenerator.cpp index a5edca2e45..9045f76b63 100644 --- a/libs/filamat/src/shaders/CodeGenerator.cpp +++ b/libs/filamat/src/shaders/CodeGenerator.cpp @@ -64,6 +64,10 @@ io::sstream& CodeGenerator::generateProlog(io::sstream& out, ShaderType type, break; } + // This allows our includer system to use the #line directive to denote the source file for + // #included code. This way, glslang reports errors more accurately. + out << "#extension GL_GOOGLE_cpp_style_line_directive : enable\n\n"; + if (mTargetApi == TargetApi::VULKAN) { out << "#define TARGET_VULKAN_ENVIRONMENT\n"; } diff --git a/libs/filamat/src/shaders/ShaderGenerator.cpp b/libs/filamat/src/shaders/ShaderGenerator.cpp index 6881859539..0cc7a18c60 100644 --- a/libs/filamat/src/shaders/ShaderGenerator.cpp +++ b/libs/filamat/src/shaders/ShaderGenerator.cpp @@ -110,8 +110,7 @@ static void appendShader(utils::io::sstream& ss, const utils::CString& shader, size_t lineOffset) noexcept { if (!shader.empty()) { size_t lines = countLines(ss.c_str()); - ss << "#line " << lineOffset; - if (shader[0] != '\n') ss << "\n"; + ss << "#line " << lineOffset + 1 << '\n'; ss << shader.c_str(); if (shader[shader.size() - 1] != '\n') { ss << "\n"; diff --git a/libs/filamat/tests/MockIncluder.h b/libs/filamat/tests/MockIncluder.h index 06abec1737..08a11fbf41 100644 --- a/libs/filamat/tests/MockIncluder.h +++ b/libs/filamat/tests/MockIncluder.h @@ -35,9 +35,8 @@ public: return *this; } - bool operator()(const utils::CString& headerName, const utils::CString& includerName, - filamat::IncludeResult& result) { - auto key = headerName.c_str(); + bool operator()(const utils::CString& includedBy, filamat::IncludeResult& result) { + auto key = result.includeName.c_str(); auto found = mIncludeMap.find(key); if (found == mIncludeMap.end()) { @@ -47,12 +46,12 @@ public: auto include = found->second; if (!include.expectedIncluder.empty()) { - EXPECT_STREQ(includerName.c_str_safe(), include.expectedIncluder.c_str()); + EXPECT_STREQ(includedBy.c_str_safe(), include.expectedIncluder.c_str()); } if (!include.source.empty()) { - result.source = utils::CString(include.source.c_str()); - result.name = headerName; + result.text = utils::CString(include.source.c_str()); + result.name = result.includeName; return true; } diff --git a/libs/filamat/tests/test_includes.cpp b/libs/filamat/tests/test_includes.cpp index b08d6c1cd8..2a83a2200e 100644 --- a/libs/filamat/tests/test_includes.cpp +++ b/libs/filamat/tests/test_includes.cpp @@ -35,10 +35,11 @@ TEST(IncludeParser, NoIncludes) { TEST(IncludeParser, SingleInclude) { utils::CString code(R"(#include "foobar.h")"); auto result = filamat::parseForIncludes(code); - EXPECT_EQ(result.size(), 1); - EXPECT_EQ(result[0].name, "foobar.h"); - EXPECT_EQ(result[0].startPosition, 0); - EXPECT_EQ(result[0].length, 19); + EXPECT_EQ(1, result.size()); + EXPECT_STREQ("foobar.h", result[0].name.c_str()); + EXPECT_EQ(0, result[0].startPosition); + EXPECT_EQ(19, result[0].length); + EXPECT_EQ(1, result[0].line); } TEST(IncludeParser, MultipleIncludes) { @@ -49,10 +50,12 @@ TEST(IncludeParser, MultipleIncludes) { EXPECT_STREQ("foobar.h", result[0].name.c_str()); EXPECT_EQ(0, result[0].startPosition); EXPECT_EQ(19, result[0].length); + EXPECT_EQ(1, result[0].line); EXPECT_STREQ("bazbarfoo.h", result[1].name.c_str()); EXPECT_EQ(20, result[1].startPosition); EXPECT_EQ(22, result[1].length); + EXPECT_EQ(2, result[1].line); } TEST(IncludeParser, EmptyInclude) { @@ -63,6 +66,7 @@ TEST(IncludeParser, EmptyInclude) { EXPECT_STREQ("", result[0].name.c_str_safe()); EXPECT_EQ(0, result[0].startPosition); EXPECT_EQ(11, result[0].length); + EXPECT_EQ(1, result[0].line); } TEST(IncludeParser, Whitepsace) { @@ -73,6 +77,7 @@ TEST(IncludeParser, Whitepsace) { EXPECT_STREQ("foobarbaz.h", result[0].name.c_str()); EXPECT_EQ(2, result[0].startPosition); EXPECT_EQ(27, result[0].length); + EXPECT_EQ(1, result[0].line); } TEST(IncludeParser, InvalidIncludes) { @@ -89,6 +94,7 @@ TEST(IncludeParser, InvalidWithValidInclude) { EXPECT_STREQ("foobar.h", result[0].name.c_str()); EXPECT_EQ(9, result[0].startPosition); EXPECT_EQ(19, result[0].length); + EXPECT_EQ(1, result[0].line); } { @@ -98,17 +104,30 @@ TEST(IncludeParser, InvalidWithValidInclude) { EXPECT_STREQ("foo.h", result[0].name.c_str()); EXPECT_EQ(26, result[0].startPosition); EXPECT_EQ(15, result[0].length); + EXPECT_EQ(1, result[0].line); } } +TEST(IncludeParser, LineNumbers) { + utils::CString code("#include \"one.h\"\n#include \"two.h\"\n\n#include \"four.h\""); + auto result = filamat::parseForIncludes(code); + EXPECT_EQ(3, result.size()); + EXPECT_EQ(1, result[0].line); + EXPECT_EQ(2, result[1].line); + EXPECT_EQ(4, result[2].line); +} + // ------------------------------------------------------------------------------------------------- TEST(IncludeResolver, NoIncludes) { utils::CString code("no includes"); MockIncluder includer; - bool result = filamat::resolveIncludes(utils::CString(""), code, includer); + filamat::IncludeResult source { + .text = code + }; + bool result = filamat::resolveIncludes(source, includer, {}); EXPECT_TRUE(result); - EXPECT_STREQ("no includes", code.c_str()); + EXPECT_STREQ("no includes", source.text.c_str()); } TEST(IncludeResolver, SingleInclude) { @@ -116,9 +135,12 @@ TEST(IncludeResolver, SingleInclude) { MockIncluder includer; includer .sourceForInclude("test.h", "include"); - bool result = filamat::resolveIncludes(utils::CString(""), code, includer); + filamat::IncludeResult source { + .text = code + }; + bool result = filamat::resolveIncludes(source, includer, {}); EXPECT_TRUE(result); - EXPECT_STREQ("include", code.c_str()); + EXPECT_STREQ("include", source.text.c_str()); } TEST(IncludeResolver, MultipleIncludes) { @@ -134,13 +156,16 @@ TEST(IncludeResolver, MultipleIncludes) { .sourceForInclude("two.h", "2") .sourceForInclude("three.h", "3"); - bool result = filamat::resolveIncludes(utils::CString(""), code, includer); + filamat::IncludeResult source { + .text = code + }; + bool result = filamat::resolveIncludes(source, includer, {}); EXPECT_TRUE(result); EXPECT_STREQ(utils::CString(R"( 1 2 3 - )").c_str(), code.c_str()); + )").c_str(), source.text.c_str()); } TEST(IncludeResolver, IncludeWithinInclude) { @@ -156,13 +181,15 @@ TEST(IncludeResolver, IncludeWithinInclude) { .sourceForInclude("three.h", "3") .expectIncludeIncludedBy("three.h", "two.h"); - - bool result = filamat::resolveIncludes(utils::CString(""), code, includer); + filamat::IncludeResult source { + .text = code + }; + bool result = filamat::resolveIncludes(source, includer, {}); EXPECT_TRUE(result); EXPECT_STREQ(utils::CString(R"( 1 3 - )").c_str(), code.c_str()); + )").c_str(), source.text.c_str()); } TEST(IncludeResolver, Includers) { @@ -177,11 +204,14 @@ TEST(IncludeResolver, Includers) { .sourceForInclude("three.h", "3") .expectIncludeIncludedBy("two.h", "dir/one.h"); - bool result = filamat::resolveIncludes(utils::CString(""), code, includer); + filamat::IncludeResult source { + .text = code + }; + bool result = filamat::resolveIncludes(source, includer, {}); EXPECT_TRUE(result); EXPECT_STREQ(utils::CString(R"( 3 - )").c_str(), code.c_str()); + )").c_str(), source.text.c_str()); } TEST(IncludeResolver, IncludeFailure) { @@ -194,7 +224,10 @@ TEST(IncludeResolver, IncludeFailure) { includer .sourceForInclude("one.h", "#include \"two.h\""); - bool result = filamat::resolveIncludes(utils::CString(""), code, includer); + filamat::IncludeResult source { + .text = code + }; + bool result = filamat::resolveIncludes(source, includer, {}); EXPECT_FALSE(result); } { @@ -204,7 +237,10 @@ TEST(IncludeResolver, IncludeFailure) { MockIncluder includer; - bool result = filamat::resolveIncludes(utils::CString(""), code, includer); + filamat::IncludeResult source { + .text = code + }; + bool result = filamat::resolveIncludes(source, includer, {}); EXPECT_FALSE(result); } } @@ -219,11 +255,132 @@ TEST(IncludeResolver, Cycle) { .sourceForInclude("foo.h", "#include \"bar.h\"") .sourceForInclude("bar.h", "#include \"foo.h\""); - bool result = filamat::resolveIncludes(utils::CString(""), code, includer); + filamat::IncludeResult source { + .text = code + }; + bool result = filamat::resolveIncludes(source, includer, {}); // Include cycles are disallowed. We should still terminate in finite time and report false. EXPECT_FALSE(result); } +// Helper function that lets us write comparison cases with line breaks. +void EXPECT_STREQ_TRIMMARGIN(const char* expected, const char* actual) { + size_t len = strlen(expected); + char* trimmed = (char*) malloc(len); + + const char* end = expected + len; + const char* e = expected; + char* t = trimmed; + + while (e < end) { + while (e < end) { + if (*e == '|') { + e += 2; // eat | and space + break; + } + e++; + } + while (e < end) { + if (*e == '\n') { + *t++ = *e++; + break; + } + *t++ = *e++; + } + } + + *t++ = 0; + + EXPECT_STREQ(trimmed, actual); + + free(trimmed); +} + +TEST(IncludeResolver, SingleIncludeLineDirective) { + utils::CString code("#include \"test.h\""); + MockIncluder includer; + includer + .sourceForInclude("test.h", "include"); + filamat::ResolveOptions options = { + .insertLineDirectives = true, + .insertLineDirectiveCheck = false // makes it simplier to test + }; + filamat::IncludeResult source { + .includeName = utils::CString("root.h"), + .text = code + }; + bool result = filamat::resolveIncludes(source, includer, options); + EXPECT_TRUE(result); + EXPECT_STREQ_TRIMMARGIN(R"( + | #line 1 "root.h" + | #line 1 "test.h" + | include + | #line 1 "root.h" + | )", source.text.c_str()); +} + +TEST(IncludeResolver, MultipleIncludesLineDirective) { + utils::CString code("#include \"one.h\"\n#include \"two.h\"\n"); + + MockIncluder includer; + includer + .sourceForInclude("one.h", "1") + .sourceForInclude("two.h", "2"); + + filamat::ResolveOptions options = { + .insertLineDirectives = true, + .insertLineDirectiveCheck = false // makes it simplier to test + }; + filamat::IncludeResult source { + .includeName = utils::CString("root.h"), + .text = code, + }; + bool result = filamat::resolveIncludes(source, includer, options); + EXPECT_TRUE(result); + + EXPECT_STREQ_TRIMMARGIN(R"( + | #line 1 "root.h" + | #line 1 "one.h" + | 1 + | #line 2 "root.h" + | #line 1 "two.h" + | 2 + | #line 3 "root.h" + | )", source.text.c_str()); +} + +TEST(IncludeResolver, MultipleIncludesSameLineLineDirective) { + // includes are on the same line + utils::CString code("#include \"one.h\"#include \"two.h\"\n"); + + MockIncluder includer; + includer + .sourceForInclude("one.h", "1") + .sourceForInclude("two.h", "2") + .expectIncludeIncludedBy("three.h", "two.h"); + + filamat::ResolveOptions options = { + .insertLineDirectives = true, + .insertLineDirectiveCheck = false // makes it simplier to test + }; + filamat::IncludeResult source { + .includeName = utils::CString("root.h"), + .text = code + }; + bool result = filamat::resolveIncludes(source, includer, options); + EXPECT_TRUE(result); + + EXPECT_STREQ_TRIMMARGIN(R"( + | #line 1 "root.h" + | #line 1 "one.h" + | 1 + | #line 1 "root.h" + | #line 1 "two.h" + | 2 + | #line 2 "root.h" + | )", source.text.c_str()); +} + // ------------------------------------------------------------------------------------------------- #include diff --git a/libs/utils/include/utils/CString.h b/libs/utils/include/utils/CString.h index 018b18f6bf..2f53fb4891 100644 --- a/libs/utils/include/utils/CString.h +++ b/libs/utils/include/utils/CString.h @@ -260,6 +260,7 @@ public: const_iterator cend() const noexcept { return end(); } CString& replace(size_type pos, size_type len, const CString& str) noexcept; + CString& insert(size_type pos, const CString& str) noexcept { return replace(pos, 0, str); } const_reference operator[](size_type pos) const noexcept { assert(pos < size()); diff --git a/shaders/CMakeLists.txt b/shaders/CMakeLists.txt index 4c11e3cc18..be5cd96a46 100644 --- a/shaders/CMakeLists.txt +++ b/shaders/CMakeLists.txt @@ -68,11 +68,14 @@ foreach(SHADER_FILE ${SHADERS}) get_filename_component(SHADER_NAME ${SHADER_FILE} NAME) set(SHADER_RAW ${CMAKE_CURRENT_SOURCE_DIR}/${SHADER_FILE}) set(SHADER_MIN ${MINIFIED_DIR}/${SHADER_NAME}) - # For Debug builds, pass the "-Onone" flag to perform no minification. This is helpful for - # debugging shaders. + # For Debug builds, pass two additional flags to help with debugging: + # -Onone performs no minification. + # -lshader_name adds a #line directive at the beginning to help identify sources of errors. + set(OPT_FLAG "$<$:-Onone>") + set(LINE_FLAG "$<$:-l${SHADER_NAME}>") add_custom_command( OUTPUT ${SHADER_MIN} - COMMAND glslminifier "$<$:-Onone>" -o ${SHADER_MIN} ${SHADER_RAW} + COMMAND glslminifier ${OPT_FLAG} ${LINE_FLAG} -o ${SHADER_MIN} ${SHADER_RAW} DEPENDS glslminifier ${SHADER_RAW} WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} COMMENT "Minifying shader ${SHADER_NAME}" diff --git a/tools/glslminifier/src/main.cpp b/tools/glslminifier/src/main.cpp index 89b7566634..a6d2f565c0 100644 --- a/tools/glslminifier/src/main.cpp +++ b/tools/glslminifier/src/main.cpp @@ -32,6 +32,8 @@ static bool g_writeToStdOut = true; static const char* g_outputFile = ""; static const char* g_inputFile = ""; GlslMinifyOptions g_optimizationLevel = GlslMinifyOptions::ALL; +static bool g_outputLineDirectives = false; +static std::string g_lineDirectiveName; static const char* USAGE = R"TXT( GLSLMINIFIER minifies GLSL shader code by removing comments, blank lines and indentation. @@ -48,6 +50,13 @@ Options: Specify path to output file. If none provided, writes to stdout. --optimization, -O [none] Set the level of optimization. "none" performs a simple passthrough. + --line, -l [name] + Insert a #line directive on the first line of the shader with the given name. + For example, --line foobar.h will insert the following: + #if defined(GL_GOOGLE_cpp_style_line_directive) + #line 0 "foobar.h" + #endif + This option is meant to be used with -Onone optimization. Example: GLSLMINIFIER -o output.fs.min input.fs @@ -76,12 +85,13 @@ static void license() { } static int handleArguments(int argc, char* argv[]) { - static constexpr const char* OPTSTR = "hLo:O:"; + static constexpr const char* OPTSTR = "hLo:O:l:"; static const struct option OPTIONS[] = { { "help", no_argument, nullptr, 'h' }, { "license", no_argument, nullptr, 'L' }, { "output", required_argument, nullptr, 'o' }, { "optimization", required_argument, nullptr, 'O' }, + { "line", required_argument, nullptr, 'l' }, { nullptr, 0, nullptr, 0 } // termination of the option list }; @@ -109,6 +119,10 @@ static int handleArguments(int argc, char* argv[]) { std::cerr << "Warning: unknown optimization level." << std::endl; } break; + case 'l': + g_outputLineDirectives = true; + g_lineDirectiveName = arg; + break; } } @@ -139,6 +153,17 @@ int main(int argc, char* argv[]) { // Minify the GLSL. string result = minifyGlsl(inputStr, g_optimizationLevel); + // Add a #line directive at the beginning of the file, if requested. + if (g_outputLineDirectives) { + // We must check for support for the line directive, otherwise drivers may complain if they + // don't support it. + std::string lineDirective = + std::string("#if defined(GL_GOOGLE_cpp_style_line_directive)\n") + + "#line 0 \"" + g_lineDirectiveName + "\"\n" + + "#endif\n"; + result.insert(0, lineDirective); + } + if (g_writeToStdOut) { cout << result; return 0; diff --git a/tools/matc/src/matc/DirIncluder.cpp b/tools/matc/src/matc/DirIncluder.cpp index db3530fabd..a92dcc2c84 100644 --- a/tools/matc/src/matc/DirIncluder.cpp +++ b/tools/matc/src/matc/DirIncluder.cpp @@ -22,20 +22,20 @@ namespace matc { -bool DirIncluder::operator()(const utils::CString& headerName, const utils::CString& includerName, - filamat::IncludeResult& result) { - auto getHeaderPath = [&includerName, &headerName, this]() { - // If includer name is empty, then search from the root include directory. - if (includerName.empty()) { - return mIncludeDirectory.concat(headerName.c_str()); +bool DirIncluder::operator()(const utils::CString& includedBy, filamat::IncludeResult& result) { + auto getHeaderPath = [&result, &includedBy, this]() { + // includedBy is the path to the file that's including result.includeName. + // If it's empty, then search from the root include directory. + if (includedBy.empty()) { + return mIncludeDirectory.concat(result.includeName.c_str()); } // Otherwise, search relative to the includer file. - utils::Path includer(includerName.c_str()); + utils::Path includer(includedBy.c_str()); // TODO: this assert was firing only in CI during DirIncluder tests. Maybe because of // inadequate file permissions. // assert(includer.isFile()); - return includer.getParent() + headerName.c_str(); + return includer.getParent() + result.includeName.c_str(); }; const utils::Path headerPath = getHeaderPath(); @@ -60,7 +60,7 @@ bool DirIncluder::operator()(const utils::CString& headerName, const utils::CStr stream.close(); - result.source = utils::CString(contents.c_str()); + result.text = utils::CString(contents.c_str()); result.name = utils::CString(headerPath.c_str()); return true; diff --git a/tools/matc/src/matc/DirIncluder.h b/tools/matc/src/matc/DirIncluder.h index 4a76883efe..8a18b044d7 100644 --- a/tools/matc/src/matc/DirIncluder.h +++ b/tools/matc/src/matc/DirIncluder.h @@ -31,8 +31,7 @@ public: mIncludeDirectory = dir; } - bool operator()(const utils::CString& headerName, const utils::CString& includerName, - filamat::IncludeResult& result); + bool operator()(const utils::CString& includedBy, filamat::IncludeResult& result); private: utils::Path mIncludeDirectory; diff --git a/tools/matc/src/matc/MaterialCompiler.cpp b/tools/matc/src/matc/MaterialCompiler.cpp index b2a851da62..ad676f251e 100644 --- a/tools/matc/src/matc/MaterialCompiler.cpp +++ b/tools/matc/src/matc/MaterialCompiler.cpp @@ -84,7 +84,9 @@ bool MaterialCompiler::processVertexShader(const MaterialLexeme& lexeme, MaterialLexeme trimmedLexeme = lexeme.trimBlockMarkers(); std::string shaderStr = trimmedLexeme.getStringValue(); - builder.materialVertex(shaderStr.c_str(), trimmedLexeme.getLine() + 1); + // getLine() returns a line number, with 1 being the first line, but .material wants a 0-based + // line number offset, where 0 is the first line. + builder.materialVertex(shaderStr.c_str(), trimmedLexeme.getLine() - 1); return true; } @@ -94,7 +96,9 @@ bool MaterialCompiler::processFragmentShader(const MaterialLexeme& lexeme, MaterialLexeme trimmedLexeme = lexeme.trimBlockMarkers(); std::string shaderStr = trimmedLexeme.getStringValue(); - builder.material(shaderStr.c_str(), trimmedLexeme.getLine() + 1); + // getLine() returns a line number, with 1 being the first line, but .material wants a 0-based + // line number offset, where 0 is the first line. + builder.material(shaderStr.c_str(), trimmedLexeme.getLine() - 1); return true; } @@ -276,6 +280,7 @@ bool MaterialCompiler::run(const Config& config) { builder .includeCallback(includer) + .fileName(materialFilePath.getName().c_str()) .platform(config.getPlatform()) .targetApi(config.getTargetApi()) .optimization(config.getOptimizationLevel()) diff --git a/tools/matc/tests/test_includer.cpp b/tools/matc/tests/test_includer.cpp index b6edbf5926..5580363d65 100644 --- a/tools/matc/tests/test_includer.cpp +++ b/tools/matc/tests/test_includer.cpp @@ -28,14 +28,18 @@ const utils::Path root = utils::Path(__FILE__).getParent(); TEST(DirIncluder, DISABLED_IncludeNonexistent) { matc::DirIncluder includer; { - IncludeResult _; - bool success = includer(CString("nonexistent.h"), CString(""), _); + IncludeResult i { + .includeName = CString("nonexistent.h") + }; + bool success = includer(CString(""), i); EXPECT_FALSE(success); } { utils::CString includerFile((root + "Foo.h").getPath().c_str()); - IncludeResult _; - bool success = includer(CString("nonexistent.h"), includerFile, _); + IncludeResult i { + .includeName = CString("nonexistent.h") + }; + bool success = includer(includerFile, i); EXPECT_FALSE(success); } } @@ -44,13 +48,15 @@ TEST(DirIncluder, DISABLED_IncludeFile) { matc::DirIncluder includer; includer.setIncludeDirectory(root); - IncludeResult result; - bool success = includer(CString("Foo.h"), CString(""), result); + IncludeResult result { + .includeName = CString("Foo.h") + }; + bool success = includer(CString(""), result); EXPECT_TRUE(success); // The result's source should be set to the contents of the includer file. - EXPECT_STREQ("// test include file", result.source.c_str()); + EXPECT_STREQ("// test include file", result.text.c_str()); // The result's name should be set to the full path to the header file. EXPECT_STREQ((root + "Foo.h").c_str(), result.name.c_str()); @@ -62,10 +68,12 @@ TEST(DirIncluder, DISABLED_IncludeFileFromIncluder) { utils::CString includerFile((root + "Dir/Baz.h").c_str()); - IncludeResult result; - bool success = includer(CString("Bar.h"), includerFile, result); + IncludeResult result { + .includeName = CString("Bar.h") + }; + bool success = includer(includerFile, result); - EXPECT_STREQ("// Bar.h", result.source.c_str()); + EXPECT_STREQ("// Bar.h", result.text.c_str()); EXPECT_STREQ((root + "Dir/Bar.h").c_str(), result.name.c_str()); }