From 6a8f65b5666737fb60409f8e3c192b6d2cda3217 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sat, 23 Sep 2023 10:09:12 +0200 Subject: [PATCH 1/3] Some assert paranoia, remove unused "failed_" variable --- Common/GPU/Vulkan/VulkanMemory.cpp | 2 +- Common/Thread/Promise.h | 1 + GPU/Vulkan/ShaderManagerVulkan.cpp | 22 +++++++--------------- GPU/Vulkan/ShaderManagerVulkan.h | 8 +------- 4 files changed, 10 insertions(+), 23 deletions(-) diff --git a/Common/GPU/Vulkan/VulkanMemory.cpp b/Common/GPU/Vulkan/VulkanMemory.cpp index 457d75d51f15..f29fc33d0b18 100644 --- a/Common/GPU/Vulkan/VulkanMemory.cpp +++ b/Common/GPU/Vulkan/VulkanMemory.cpp @@ -291,7 +291,7 @@ VulkanPushPool::Block VulkanPushPool::CreateBlock(size_t size) { _assert_(result == VK_SUCCESS); result = vmaMapMemory(vulkan_->Allocator(), block.allocation, (void **)(&block.writePtr)); - _assert_msg_(result == VK_SUCCESS, "VulkanPushPool: Failed to map memory (result = %08x)", result); + _assert_msg_(result == VK_SUCCESS, "VulkanPushPool: Failed to map memory (result = %s)", VulkanResultToString(result)); _assert_msg_(block.writePtr != nullptr, "VulkanPushPool: Failed to map memory on block of size %d", (int)block.size); return block; diff --git a/Common/Thread/Promise.h b/Common/Thread/Promise.h index f8dbaf9e6a1e..93e4dfd98507 100644 --- a/Common/Thread/Promise.h +++ b/Common/Thread/Promise.h @@ -45,6 +45,7 @@ class PromiseTask : public Task { template class Promise { public: + // Never fails. static Promise *Spawn(ThreadManager *threadman, std::function fun, TaskType taskType, TaskPriority taskPriority = TaskPriority::NORMAL) { Mailbox *mailbox = new Mailbox(); diff --git a/GPU/Vulkan/ShaderManagerVulkan.cpp b/GPU/Vulkan/ShaderManagerVulkan.cpp index 452395a00f2f..319f586bfa0a 100644 --- a/GPU/Vulkan/ShaderManagerVulkan.cpp +++ b/GPU/Vulkan/ShaderManagerVulkan.cpp @@ -47,6 +47,7 @@ // Most drivers treat vkCreateShaderModule as pretty much a memcpy. What actually // takes time here, and makes this worthy of parallelization, is GLSLtoSPV. // Takes ownership over tag. +// This always returns something, checking the return value for null is not meaningful. static Promise *CompileShaderModuleAsync(VulkanContext *vulkan, VkShaderStageFlagBits stage, const char *code, std::string *tag) { auto compile = [=] { PROFILE_THIS_SCOPE("shadercomp"); @@ -112,13 +113,10 @@ static Promise *CompileShaderModuleAsync(VulkanContext *vulkan, VulkanFragmentShader::VulkanFragmentShader(VulkanContext *vulkan, FShaderID id, FragmentShaderFlags flags, const char *code) : vulkan_(vulkan), id_(id), flags_(flags) { + _assert_(!id.is_invalid()); source_ = code; module_ = CompileShaderModuleAsync(vulkan, VK_SHADER_STAGE_FRAGMENT_BIT, source_.c_str(), new std::string(FragmentShaderDesc(id))); - if (!module_) { - failed_ = true; - } else { - VERBOSE_LOG(G3D, "Compiled fragment shader:\n%s\n", (const char *)code); - } + VERBOSE_LOG(G3D, "Compiled fragment shader:\n%s\n", (const char *)code); } VulkanFragmentShader::~VulkanFragmentShader() { @@ -147,13 +145,10 @@ std::string VulkanFragmentShader::GetShaderString(DebugShaderStringType type) co VulkanVertexShader::VulkanVertexShader(VulkanContext *vulkan, VShaderID id, VertexShaderFlags flags, const char *code, bool useHWTransform) : vulkan_(vulkan), useHWTransform_(useHWTransform), flags_(flags), id_(id) { + _assert_(!id.is_invalid()); source_ = code; module_ = CompileShaderModuleAsync(vulkan, VK_SHADER_STAGE_VERTEX_BIT, source_.c_str(), new std::string(VertexShaderDesc(id))); - if (!module_) { - failed_ = true; - } else { - VERBOSE_LOG(G3D, "Compiled vertex shader:\n%s\n", (const char *)code); - } + VERBOSE_LOG(G3D, "Compiled vertex shader:\n%s\n", (const char *)code); } VulkanVertexShader::~VulkanVertexShader() { @@ -182,13 +177,10 @@ std::string VulkanVertexShader::GetShaderString(DebugShaderStringType type) cons VulkanGeometryShader::VulkanGeometryShader(VulkanContext *vulkan, GShaderID id, const char *code) : vulkan_(vulkan), id_(id) { + _assert_(!id.is_invalid()); source_ = code; module_ = CompileShaderModuleAsync(vulkan, VK_SHADER_STAGE_GEOMETRY_BIT, source_.c_str(), new std::string(GeometryShaderDesc(id).c_str())); - if (!module_) { - failed_ = true; - } else { - VERBOSE_LOG(G3D, "Compiled geometry shader:\n%s\n", (const char *)code); - } + VERBOSE_LOG(G3D, "Compiled geometry shader:\n%s\n", (const char *)code); } VulkanGeometryShader::~VulkanGeometryShader() { diff --git a/GPU/Vulkan/ShaderManagerVulkan.h b/GPU/Vulkan/ShaderManagerVulkan.h index 80c1f39a131f..176af955d187 100644 --- a/GPU/Vulkan/ShaderManagerVulkan.h +++ b/GPU/Vulkan/ShaderManagerVulkan.h @@ -43,8 +43,6 @@ class VulkanFragmentShader { const std::string &source() const { return source_; } - bool Failed() const { return failed_; } - std::string GetShaderString(DebugShaderStringType type) const; Promise *GetModule() { return module_; } const FShaderID &GetID() const { return id_; } @@ -68,7 +66,6 @@ class VulkanVertexShader { const std::string &source() const { return source_; } - bool Failed() const { return failed_; } bool UseHWTransform() const { return useHWTransform_; } // TODO: Roll into flags VertexShaderFlags Flags() const { return flags_; } @@ -81,7 +78,6 @@ class VulkanVertexShader { VulkanContext *vulkan_; std::string source_; - bool failed_ = false; bool useHWTransform_; VShaderID id_; VertexShaderFlags flags_; @@ -94,9 +90,8 @@ class VulkanGeometryShader { const std::string &source() const { return source_; } - bool Failed() const { return failed_; } - std::string GetShaderString(DebugShaderStringType type) const; + Promise *GetModule() const { return module_; } const GShaderID &GetID() { return id_; } @@ -105,7 +100,6 @@ class VulkanGeometryShader { VulkanContext *vulkan_; std::string source_; - bool failed_ = false; GShaderID id_; }; From 8fc01e37d94306e3d89af2084a59efd7ae3553f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sat, 23 Sep 2023 10:09:55 +0200 Subject: [PATCH 2/3] Check for bad indices in GetReplacementFunc to avoid crashes --- Core/HLE/ReplaceTables.cpp | 5 ++++- Core/HLE/ReplaceTables.h | 2 +- Core/MIPS/ARM64/Arm64Jit.cpp | 1 + 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/Core/HLE/ReplaceTables.cpp b/Core/HLE/ReplaceTables.cpp index 30afa6e26b2c..f71b8b945cb2 100644 --- a/Core/HLE/ReplaceTables.cpp +++ b/Core/HLE/ReplaceTables.cpp @@ -1590,7 +1590,10 @@ std::vector GetReplacementFuncIndexes(u64 hash, int funcSize) { return emptyResult; } -const ReplacementTableEntry *GetReplacementFunc(int i) { +const ReplacementTableEntry *GetReplacementFunc(size_t i) { + if (i >= ARRAY_SIZE(entries)) { + return nullptr; + } return &entries[i]; } diff --git a/Core/HLE/ReplaceTables.h b/Core/HLE/ReplaceTables.h index 94ee26d69ea1..980f506b6af1 100644 --- a/Core/HLE/ReplaceTables.h +++ b/Core/HLE/ReplaceTables.h @@ -64,7 +64,7 @@ void Replacement_Shutdown(); int GetNumReplacementFuncs(); std::vector GetReplacementFuncIndexes(u64 hash, int funcSize); -const ReplacementTableEntry *GetReplacementFunc(int index); +const ReplacementTableEntry *GetReplacementFunc(size_t index); void WriteReplaceInstructions(u32 address, u64 hash, int size); void RestoreReplacedInstruction(u32 address); diff --git a/Core/MIPS/ARM64/Arm64Jit.cpp b/Core/MIPS/ARM64/Arm64Jit.cpp index d1f1062f1ef3..c412a2a34b0a 100644 --- a/Core/MIPS/ARM64/Arm64Jit.cpp +++ b/Core/MIPS/ARM64/Arm64Jit.cpp @@ -563,6 +563,7 @@ void Arm64Jit::Comp_ReplacementFunc(MIPSOpcode op) const ReplacementTableEntry *entry = GetReplacementFunc(index); if (!entry) { ERROR_LOG(HLE, "Invalid replacement op %08x", op.encoding); + // TODO: What should we do here? We're way off in the weeds probably. return; } From e64d1e94fe6eb6ff1852fe832930166ebb037e01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sat, 23 Sep 2023 11:39:20 +0200 Subject: [PATCH 3/3] add reporting to the invalid replacement op --- Core/MIPS/ARM/ArmJit.cpp | 2 +- Core/MIPS/ARM64/Arm64Jit.cpp | 2 +- Core/MIPS/x86/Jit.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Core/MIPS/ARM/ArmJit.cpp b/Core/MIPS/ARM/ArmJit.cpp index c0ae1071a5ac..eab53366291f 100644 --- a/Core/MIPS/ARM/ArmJit.cpp +++ b/Core/MIPS/ARM/ArmJit.cpp @@ -561,7 +561,7 @@ void ArmJit::Comp_ReplacementFunc(MIPSOpcode op) const ReplacementTableEntry *entry = GetReplacementFunc(index); if (!entry) { - ERROR_LOG(HLE, "Invalid replacement op %08x", op.encoding); + ERROR_LOG_REPORT_ONCE(replFunc, HLE, "Invalid replacement op %08x at %08x", op.encoding, js.compilerPC); return; } diff --git a/Core/MIPS/ARM64/Arm64Jit.cpp b/Core/MIPS/ARM64/Arm64Jit.cpp index c412a2a34b0a..b8e8b6f560f9 100644 --- a/Core/MIPS/ARM64/Arm64Jit.cpp +++ b/Core/MIPS/ARM64/Arm64Jit.cpp @@ -562,7 +562,7 @@ void Arm64Jit::Comp_ReplacementFunc(MIPSOpcode op) const ReplacementTableEntry *entry = GetReplacementFunc(index); if (!entry) { - ERROR_LOG(HLE, "Invalid replacement op %08x", op.encoding); + ERROR_LOG_REPORT_ONCE(replFunc, HLE, "Invalid replacement op %08x at %08x", op.encoding, js.compilerPC); // TODO: What should we do here? We're way off in the weeds probably. return; } diff --git a/Core/MIPS/x86/Jit.cpp b/Core/MIPS/x86/Jit.cpp index c7e2e2fed802..62225275e504 100644 --- a/Core/MIPS/x86/Jit.cpp +++ b/Core/MIPS/x86/Jit.cpp @@ -605,7 +605,7 @@ void Jit::Comp_ReplacementFunc(MIPSOpcode op) { const ReplacementTableEntry *entry = GetReplacementFunc(index); if (!entry) { - ERROR_LOG(HLE, "Invalid replacement op %08x", op.encoding); + ERROR_LOG_REPORT_ONCE(replFunc, HLE, "Invalid replacement op %08x at %08x", op.encoding, js.compilerPC); return; }