From 0eee2d45d053dfc566baa58442a9b1b708e4f2a7 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Mon, 14 Sep 2020 13:53:24 +0100 Subject: [PATCH] Crash fix + debugger improvement (#912) * dxc: Use a CComPtr for the include handler The include handler was constructed and passed down to `IDxcCompiler::Compile()`, but was not referenced by the caller. Within `IDxcCompiler::Compile()`, the include handler may be referenced, released, and an attempt to be referenced again (using deleted memory). Use `CComPtr` to keep the COM object alive for the full duration of the compile. * Pass a unique shader filename down to DXC In order for the debugger to be able to fetch the source of a shader that has its source embedded within an amber script file, we need to have a known and unique file name. The file name was previously generated just before calling into DXC to compile the file, and could easily have multiple shaders with the same name. Instead we now add a optional file path property to the `amber::Shader`, which is populated by the parser with either the `VIRTUAL_FILE` path, or a generated path from the shader name for embedded shaders. The parser also adds the shader's source to the `VirtualFileStore` so it can be found by the debugger engine. If no file path is specified by the parser, dxc_helper defaults to its previous naming scheme. --- src/amberscript/parser.cc | 38 ++++++++++++++++----------- src/amberscript/parser_shader_test.cc | 37 ++++++++++++++++++++++++++ src/dxc_helper.cc | 15 ++++++----- src/dxc_helper.h | 3 ++- src/shader.h | 4 +++ src/shader_compiler.cc | 2 +- 6 files changed, 74 insertions(+), 25 deletions(-) diff --git a/src/amberscript/parser.cc b/src/amberscript/parser.cc index 3b7534875..0936bcd76 100644 --- a/src/amberscript/parser.cc +++ b/src/amberscript/parser.cc @@ -515,34 +515,40 @@ Result Parser::ParseShaderBlock() { if (!token->IsIdentifier() && !token->IsString()) return Result("expected virtual file path after VIRTUAL_FILE"); - r = ValidateEndOfStatement("SHADER command"); - if (!r.IsSuccess()) - return r; - auto path = token->AsString(); std::string data; r = script_->GetVirtualFile(path, &data); - if (!r.IsSuccess()) { + if (!r.IsSuccess()) return r; - } shader->SetData(data); - } else { - r = ValidateEndOfStatement("SHADER command"); + shader->SetFilePath(path); + + r = script_->AddShader(std::move(shader)); if (!r.IsSuccess()) return r; - std::string data = tokenizer_->ExtractToNext("END"); - if (data.empty()) - return Result("SHADER must not be empty"); + return ValidateEndOfStatement("SHADER command"); + } - shader->SetData(data); + r = ValidateEndOfStatement("SHADER command"); + if (!r.IsSuccess()) + return r; - token = tokenizer_->NextToken(); - if (!token->IsIdentifier() || token->AsString() != "END") - return Result("SHADER missing END command"); - } + std::string data = tokenizer_->ExtractToNext("END"); + if (data.empty()) + return Result("SHADER must not be empty"); + + shader->SetData(data); + + auto path = "embedded-shaders/" + shader->GetName(); + script_->AddVirtualFile(path, data); + shader->SetFilePath(path); + + token = tokenizer_->NextToken(); + if (!token->IsIdentifier() || token->AsString() != "END") + return Result("SHADER missing END command"); r = script_->AddShader(std::move(shader)); if (!r.IsSuccess()) diff --git a/src/amberscript/parser_shader_test.cc b/src/amberscript/parser_shader_test.cc index 83aaf2b63..657cc674f 100644 --- a/src/amberscript/parser_shader_test.cc +++ b/src/amberscript/parser_shader_test.cc @@ -461,5 +461,42 @@ END ASSERT_TRUE(r.IsSuccess()); } +TEST_F(AmberScriptParserTest, ShaderDefaultFilePath) { + std::string in = R"(#!amber +SHADER fragment shader_name GLSL +void main() { + gl_FragColor = vec3(2, 3, 4); +} +END)"; + + Parser parser; + Result r = parser.Parse(in); + ASSERT_TRUE(r.IsSuccess()) << r.Error(); + + auto script = parser.GetScript(); + auto shader = script->GetShader("shader_name"); + EXPECT_EQ("embedded-shaders/shader_name", shader->GetFilePath()); +} + +TEST_F(AmberScriptParserTest, ShaderVirtualFilePath) { + std::string in = R"(#!amber +VIRTUAL_FILE my_fragment_shader +void main() { + gl_FragColor = vec3(2, 3, 4); +} +END + +SHADER fragment shader_name GLSL VIRTUAL_FILE my_fragment_shader +)"; + + Parser parser; + Result r = parser.Parse(in); + ASSERT_TRUE(r.IsSuccess()) << r.Error(); + + auto script = parser.GetScript(); + auto shader = script->GetShader("shader_name"); + EXPECT_EQ("my_fragment_shader", shader->GetFilePath()); +} + } // namespace amberscript } // namespace amber diff --git a/src/dxc_helper.cc b/src/dxc_helper.cc index 7dc4f4e81..8fb0f8a26 100644 --- a/src/dxc_helper.cc +++ b/src/dxc_helper.cc @@ -135,6 +135,7 @@ Result Compile(const std::string& src, const std::string& entry, const std::string& profile, const std::string& spv_env, + const std::string& filename, const VirtualFileStore* virtual_files, std::vector* generated_binary) { if (hlsl::options::initHlslOptTable()) { @@ -163,8 +164,8 @@ Result Compile(const std::string& src, return Result("DXC compile failure: CreateIncludeHandler"); } - IDxcIncludeHandler* include_handler = - new IncludeHandler(virtual_files, dxc_lib, fallback_include_handler); + CComPtr include_handler( + new IncludeHandler(virtual_files, dxc_lib, fallback_include_handler)); IDxcCompiler* compiler; if (DxcCreateInstance(CLSID_DxcCompiler, __uuidof(IDxcCompiler), @@ -173,9 +174,7 @@ Result Compile(const std::string& src, return Result("DXCCreateInstance for DXCCompiler failed"); } - IDxcOperationResult* result; - std::wstring src_filename = - L"amber." + std::wstring(profile.begin(), profile.end()); + std::string filepath = filename.empty() ? ("amber." + profile) : filename; std::vector dxc_flags(kDxcFlags, &kDxcFlags[kDxcFlagsCount]); const wchar_t* target_env = nullptr; @@ -191,9 +190,11 @@ Result Compile(const std::string& src, if (target_env) dxc_flags.push_back(target_env); + IDxcOperationResult* result; if (compiler->Compile( - source, /* source text */ - src_filename.c_str(), /* original file source */ + source, /* source text */ + std::wstring(filepath.begin(), filepath.end()) + .c_str(), /* original file source */ std::wstring(entry.begin(), entry.end()) .c_str(), /* entry point name */ std::wstring(profile.begin(), profile.end()) diff --git a/src/dxc_helper.h b/src/dxc_helper.h index 22082cea8..eb8321636 100644 --- a/src/dxc_helper.h +++ b/src/dxc_helper.h @@ -28,7 +28,8 @@ namespace dxchelper { // Passes the HLSL source code to the DXC compiler with SPIR-V CodeGen. // Returns the generated SPIR-V binary via |generated_binary| argument. -Result Compile(const std::string& src_str, +Result Compile(const std::string& src, + const std::string& filename, const std::string& entry_str, const std::string& profile_str, const std::string& spv_env, diff --git a/src/shader.h b/src/shader.h index 10526626c..29d6e5e55 100644 --- a/src/shader.h +++ b/src/shader.h @@ -33,6 +33,9 @@ class Shader { void SetName(const std::string& name) { name_ = name; } const std::string& GetName() const { return name_; } + void SetFilePath(const std::string& path) { file_path_ = path; } + const std::string& GetFilePath() const { return file_path_; } + void SetFormat(ShaderFormat fmt) { shader_format_ = fmt; } ShaderFormat GetFormat() const { return shader_format_; } @@ -49,6 +52,7 @@ class Shader { ShaderFormat shader_format_; std::string data_; std::string name_; + std::string file_path_; std::string target_env_; }; diff --git a/src/shader_compiler.cc b/src/shader_compiler.cc index 8f0d8a4ae..285dd9738 100644 --- a/src/shader_compiler.cc +++ b/src/shader_compiler.cc @@ -276,7 +276,7 @@ Result ShaderCompiler::CompileHlsl(const Shader* shader, return Result("Unknown shader type"); return dxchelper::Compile(shader->GetData(), "main", target, spv_env_, - virtual_files_, result); + shader->GetFilePath(), virtual_files_, result); } #else Result ShaderCompiler::CompileHlsl(const Shader*,