From 7bdf8a8d8871535b9a004ae8bb80f449aa36ac72 Mon Sep 17 00:00:00 2001 From: Jake Turner Date: Wed, 13 Aug 2025 12:21:59 +0100 Subject: [PATCH] Make pointer read/write methods return DeviceOpResult ThreadState DeviceOpResult ReadPointerValue(Id pointer, ShaderVariable &ret); Debugger DeviceOpResult ReadFromPointer(const ShaderVariable &ptr, ShaderVariable &ret) const; DeviceOpResult GetPointerValue(const ShaderVariable &v, ShaderVariable &ret) const; --- .../driver/shaders/spirv/spirv_debug.cpp | 177 +++++++++-- renderdoc/driver/shaders/spirv/spirv_debug.h | 14 +- .../shaders/spirv/spirv_debug_setup.cpp | 59 ++-- renderdoc/driver/vulkan/vk_shaderdebug.cpp | 286 ++++++++++++++---- 4 files changed, 414 insertions(+), 122 deletions(-) diff --git a/renderdoc/driver/shaders/spirv/spirv_debug.cpp b/renderdoc/driver/shaders/spirv/spirv_debug.cpp index d688e8e46..a83c041f0 100644 --- a/renderdoc/driver/shaders/spirv/spirv_debug.cpp +++ b/renderdoc/driver/shaders/spirv/spirv_debug.cpp @@ -356,16 +356,22 @@ DeviceOpResult ThreadState::WritePointerValue(Id pointer, const ShaderVariable & { // if this is a write to a SSBO pointer, don't record any alias changes, just record a no-op // change to this pointer - basechange.after = basechange.before = debugger.GetPointerValue(ids[pointer]); + if(debugger.GetPointerValue(ids[pointer], basechange.before) == DeviceOpResult::NeedsDevice) + return DeviceOpResult::NeedsDevice; + basechange.after = basechange.before; if(debugger.WriteThroughPointer(var, val) == DeviceOpResult::NeedsDevice) return DeviceOpResult::NeedsDevice; pendingDebugState.changes.push_back(basechange); return DeviceOpResult::Succeeded; } - rdcarray changes; - basechange.before = debugger.GetPointerValue(ids[ptrid]); + DeviceOpResult opResult; + // Check if the base is available (cached), if it is available then so are all its aliases. + opResult = debugger.GetPointerValue(ids[ptrid], basechange.before); + if(opResult == DeviceOpResult::NeedsDevice) + return DeviceOpResult::NeedsDevice; + rdcarray changes; rdcarray &pointers = pointersForId[ptrid]; changes.resize(pointers.size()); @@ -375,26 +381,32 @@ DeviceOpResult ThreadState::WritePointerValue(Id pointer, const ShaderVariable & { Id id = pointers[i]; if(id != ptrid && live.contains(id)) - changes[i].before = debugger.GetPointerValue(ids[id]); + { + opResult = debugger.GetPointerValue(ids[id], changes[i].before); + RDCASSERTEQUAL(opResult, DeviceOpResult::Succeeded); + } } - if(debugger.WriteThroughPointer(var, val) == DeviceOpResult::NeedsDevice) - return DeviceOpResult::NeedsDevice; + opResult = debugger.WriteThroughPointer(var, val); + RDCASSERTEQUAL(opResult, DeviceOpResult::Succeeded); // now evaluate the value after for(size_t i = 0; i < pointers.size(); i++) { Id id = pointers[i]; if(id != ptrid && live.contains(id)) - changes[i].after = debugger.GetPointerValue(ids[id]); + { + opResult = debugger.GetPointerValue(ids[id], changes[i].after); + RDCASSERTEQUAL(opResult, DeviceOpResult::Succeeded); + } } // For GSM memory update the global data as well as the local cache, do not send the changes to the UI auto gsmPtrIt = gsmPointers.find(pointer); if(gsmPtrIt != gsmPointers.end()) { - if(debugger.WriteThroughPointer(gsmPtrIt->second, val) == DeviceOpResult::NeedsDevice) - return DeviceOpResult::NeedsDevice; + opResult = debugger.WriteThroughPointer(gsmPtrIt->second, val); + RDCASSERTEQUAL(opResult, DeviceOpResult::Succeeded); } // if the pointer we're writing is one of the aliased pointers, be sure we add it even if @@ -419,7 +431,8 @@ DeviceOpResult ThreadState::WritePointerValue(Id pointer, const ShaderVariable & // always add a change for the base storage variable written itself, even if that's a no-op. // This one is not included in any of the pointers lists above - basechange.after = debugger.GetPointerValue(ids[ptrid]); + opResult = debugger.GetPointerValue(ids[ptrid], basechange.after); + RDCASSERTEQUAL(opResult, DeviceOpResult::Succeeded); // if this is the first local write, mark this variable as becoming alive here, instead of at // its declaration @@ -439,9 +452,9 @@ DeviceOpResult ThreadState::WritePointerValue(Id pointer, const ShaderVariable & return DeviceOpResult::Succeeded; } -ShaderVariable ThreadState::ReadPointerValue(Id pointer) +DeviceOpResult ThreadState::ReadPointerValue(Id pointer, ShaderVariable &ret) { - return debugger.ReadFromPointer(GetSrc(pointer)); + return debugger.ReadFromPointer(GetSrc(pointer), ret); } void ThreadState::DebugBreak() @@ -456,6 +469,22 @@ void ThreadState::SetDst(Id id, const ShaderVariable &val) if(IsPendingResultPending()) return; + ShaderVariable cur = val; + cur.name = GetRawName(id); + + ShaderVariable afterVal; + DeviceOpResult opResult; + if(m_State) + { + // Check if the variable is available (cached) + opResult = debugger.GetPointerValue(cur, afterVal); + if(opResult == DeviceOpResult::NeedsDevice) + { + SetStepNeedsDeviceThread(); + return; + } + } + if(m_State && ContainsNaNInf(val)) pendingDebugState.flags |= ShaderEvents::GeneratedNanOrInf; @@ -466,8 +495,7 @@ void ThreadState::SetDst(Id id, const ShaderVariable &val) if(prev.name.empty() && prev.type == VarType::Unknown) callstack.back()->idsCreated.push_back(id); - ids[id] = val; - ids[id].name = GetRawName(id); + ids[id] = cur; lastWrite[id] = m_State ? m_State->stepIndex : nextInstruction; @@ -494,8 +522,12 @@ void ThreadState::SetDst(Id id, const ShaderVariable &val) { ShaderVariableChange change; if(wasLive) - change.before = debugger.GetPointerValue(prev); - change.after = debugger.GetPointerValue(ids[id]); + { + // The variable was live and written to, it should be cached + opResult = debugger.GetPointerValue(prev, change.before); + RDCASSERTEQUAL(opResult, DeviceOpResult::Succeeded); + } + change.after = afterVal; pendingDebugState.changes.push_back(change); } } @@ -511,12 +543,14 @@ void ThreadState::ProcessScopeChange(const rdcarray &oldLive, const rdcarray const rdcarray &liveGlobals = debugger.GetLiveGlobals(); + ShaderVariable val; for(const Id &id : oldLive) { if(liveGlobals.contains(id)) continue; - pendingDebugState.changes.push_back({debugger.GetPointerValue(ids[id])}); + RDCASSERTEQUAL(debugger.GetPointerValue(ids[id], val), DeviceOpResult::Succeeded); + pendingDebugState.changes.push_back({val}); if(ids[id].type == VarType::GPUPointer && !debugger.IsOpaquePointer(ids[id]) && !debugger.IsPhysicalPointer(ids[id])) @@ -532,7 +566,8 @@ void ThreadState::ProcessScopeChange(const rdcarray &oldLive, const rdcarray if(liveGlobals.contains(id)) continue; - pendingDebugState.changes.push_back({ShaderVariable(), debugger.GetPointerValue(ids[id])}); + RDCASSERTEQUAL(debugger.GetPointerValue(ids[id], val), DeviceOpResult::Succeeded); + pendingDebugState.changes.push_back({ShaderVariable(), val}); } } @@ -831,7 +866,13 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray (void)load.memoryAccess; // get the pointer value, evaluate it (i.e. dereference) and store the result - SetDst(load.result, ReadPointerValue(load.pointer)); + ShaderVariable val; + if(ReadPointerValue(load.pointer, val) == DeviceOpResult::NeedsDevice) + { + SetStepNeedsDeviceThread(); + break; + } + SetDst(load.result, val); break; } @@ -843,7 +884,10 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray (void)store.memoryAccess; if(WritePointerValue(store.pointer, GetSrc(store.object)) == DeviceOpResult::NeedsDevice) + { SetStepNeedsDeviceThread(); + break; + } break; } @@ -855,8 +899,19 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray (void)copy.memoryAccess0; (void)copy.memoryAccess1; - if(WritePointerValue(copy.target, ReadPointerValue(copy.source)) == DeviceOpResult::NeedsDevice) + ShaderVariable val; + { + if(ReadPointerValue(copy.source, val) == DeviceOpResult::NeedsDevice) + { + SetStepNeedsDeviceThread(); + break; + } + } + if(WritePointerValue(copy.target, val) == DeviceOpResult::NeedsDevice) + { SetStepNeedsDeviceThread(); + break; + } break; } @@ -948,7 +1003,14 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray ShaderVariable result; result.rows = result.columns = 1; - ShaderBindIndex bind = debugger.GetPointerValue(structPointer).GetBindIndex(); + ShaderVariable val; + if(debugger.GetPointerValue(structPointer, val) == DeviceOpResult::NeedsDevice) + { + SetStepNeedsDeviceThread(); + break; + } + + ShaderBindIndex bind = val.GetBindIndex(); uint64_t byteLen = debugger.GetAPIWrapper()->GetBufferLength(bind) - offset; @@ -1080,7 +1142,9 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray debugger.MakeCompositePointer(ids[extract.composite], extract.composite, extract.indexes); // then evaluate it, to get the extracted value - SetDst(extract.result, debugger.ReadFromPointer(ptr)); + ShaderVariable val; + RDCASSERTEQUAL(debugger.ReadFromPointer(ptr, val), DeviceOpResult::Succeeded); + SetDst(extract.result, val); break; } @@ -4055,6 +4119,9 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray { if(hasReturnValueData) SetDst(call.result, returnValue); + if(IsPendingResultPending()) + break; + returnValue = ShaderVariable(); hasReturnValueData = false; // The instruction after a function call is defined to be a convergence point, mark that we entered it @@ -4139,11 +4206,18 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray // read/write texel use. OpImageTexelPointer ptr(it); + ShaderVariable val; + if(ReadPointerValue(ptr.image, val) == DeviceOpResult::NeedsDevice) + { + SetStepNeedsDeviceThread(); + break; + } + ShaderVariable result; result.rows = 0; result.columns = 0; result.type = VarType::Struct; - result.members = {ReadPointerValue(ptr.image), GetSrc(ptr.coordinate), GetSrc(ptr.sample)}; + result.members = {val, GetSrc(ptr.coordinate), GetSrc(ptr.sample)}; result.members[0].name = "image"; result.members[1].name = "coord"; result.members[2].name = "sample"; @@ -4164,7 +4238,11 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray if(ptr.members.empty()) { - result = ReadPointerValue(load.pointer); + if(ReadPointerValue(load.pointer, result) == DeviceOpResult::NeedsDevice) + { + SetStepNeedsDeviceThread(); + break; + } } else { @@ -4204,7 +4282,10 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray if(ptr.members.empty()) { if(WritePointerValue(store.pointer, value) == DeviceOpResult::NeedsDevice) + { SetStepNeedsDeviceThread(); + break; + } } else { @@ -4232,9 +4313,16 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray if(ptr.members.empty()) { - result = ReadPointerValue(excg.pointer); - if(WritePointerValue(excg.pointer, value) == DeviceOpResult::NeedsDevice) + if(ReadPointerValue(excg.pointer, result) == DeviceOpResult::NeedsDevice) + { SetStepNeedsDeviceThread(); + break; + } + if(WritePointerValue(excg.pointer, value) == DeviceOpResult::NeedsDevice) + { + SetStepNeedsDeviceThread(); + break; + } } else { @@ -4286,7 +4374,11 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray if(ptr.members.empty()) { - result = ReadPointerValue(cmpexcg.pointer); + if(ReadPointerValue(cmpexcg.pointer, result) == DeviceOpResult::NeedsDevice) + { + SetStepNeedsDeviceThread(); + break; + } } else { @@ -4309,7 +4401,7 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray } } - SetDst(cmpexcg.result, result); + ShaderVariable ssaResult(result); uint64_t resultVal = 0, compareVal = 0; @@ -4329,7 +4421,10 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray if(ptr.members.empty()) { if(WritePointerValue(cmpexcg.pointer, value) == DeviceOpResult::NeedsDevice) + { SetStepNeedsDeviceThread(); + break; + } } else { @@ -4342,6 +4437,7 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray } } } + SetDst(cmpexcg.result, ssaResult); break; } case Op::AtomicIIncrement: @@ -4358,7 +4454,11 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray if(ptr.members.empty()) { - result = ReadPointerValue(atomic.pointer); + if(ReadPointerValue(atomic.pointer, result) == DeviceOpResult::NeedsDevice) + { + SetStepNeedsDeviceThread(); + break; + } } else { @@ -4381,7 +4481,7 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray } } - SetDst(atomic.result, result); + ShaderVariable ssaResult(result); { #undef _IMPL @@ -4398,7 +4498,10 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray if(ptr.members.empty()) { if(WritePointerValue(atomic.pointer, result) == DeviceOpResult::NeedsDevice) + { SetStepNeedsDeviceThread(); + break; + } } else { @@ -4409,6 +4512,7 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray break; } } + SetDst(atomic.result, ssaResult); break; } case Op::AtomicFAddEXT: @@ -4436,7 +4540,11 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray if(ptr.members.empty()) { - result = ReadPointerValue(atomic.pointer); + if(ReadPointerValue(atomic.pointer, result) == DeviceOpResult::NeedsDevice) + { + SetStepNeedsDeviceThread(); + break; + } } else { @@ -4459,7 +4567,7 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray } } - SetDst(atomic.result, result); + ShaderVariable ssaResult(result); if(opdata.op == Op::AtomicIAdd) { @@ -4547,7 +4655,10 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray if(ptr.members.empty()) { if(WritePointerValue(atomic.pointer, result) == DeviceOpResult::NeedsDevice) + { SetStepNeedsDeviceThread(); + break; + } } else { @@ -4558,6 +4669,8 @@ void ThreadState::StepNext(ShaderDebugState *state, const rdcarray break; } } + + SetDst(atomic.result, ssaResult); break; } diff --git a/renderdoc/driver/shaders/spirv/spirv_debug.h b/renderdoc/driver/shaders/spirv/spirv_debug.h index cb7198bc3..985d7d357 100644 --- a/renderdoc/driver/shaders/spirv/spirv_debug.h +++ b/renderdoc/driver/shaders/spirv/spirv_debug.h @@ -95,11 +95,11 @@ public: virtual ResourceId GetShaderID() = 0; - virtual uint64_t GetBufferLength(ShaderBindIndex bind) = 0; + virtual uint64_t GetBufferLength(const ShaderBindIndex &bind) = 0; - virtual void ReadBufferValue(ShaderBindIndex bind, uint64_t offset, uint64_t byteSize, + virtual void ReadBufferValue(const ShaderBindIndex &bind, uint64_t offset, uint64_t byteSize, void *dst) = 0; - virtual void WriteBufferValue(ShaderBindIndex bind, uint64_t offset, uint64_t byteSize, + virtual void WriteBufferValue(const ShaderBindIndex &bind, uint64_t offset, uint64_t byteSize, const void *src) = 0; virtual void ReadAddress(uint64_t address, uint64_t byteSize, void *dst) = 0; @@ -115,6 +115,8 @@ public: virtual uint32_t GetThreadProperty(uint32_t threadIndex, ThreadProperty prop) = 0; virtual bool IsImageCached(const ShaderBindIndex &bind) = 0; + virtual bool IsBufferCached(const ShaderBindIndex &bind) = 0; + virtual bool IsBufferCached(uint64_t address) = 0; enum TextureType { @@ -336,7 +338,7 @@ struct ThreadState const ShaderVariable &GetSrc(Id id) const; DeviceOpResult WritePointerValue(Id pointer, const ShaderVariable &val); - ShaderVariable ReadPointerValue(Id pointer); + DeviceOpResult ReadPointerValue(Id pointer, ShaderVariable &ret); void DebugBreak(); @@ -624,8 +626,8 @@ public: rdcstr GetHumanName(Id id) const; void AllocateVariable(Id id, Id typeId, ShaderVariable &outVar) const; - ShaderVariable ReadFromPointer(const ShaderVariable &v) const; - ShaderVariable GetPointerValue(const ShaderVariable &v) const; + DeviceOpResult ReadFromPointer(const ShaderVariable &ptr, ShaderVariable &ret) const; + DeviceOpResult GetPointerValue(const ShaderVariable &v, ShaderVariable &ret) const; uint64_t GetPointerByteOffset(const ShaderVariable &ptr) const; DebugAPIWrapper::TextureType GetTextureType(const ShaderVariable &img) const; ShaderVariable MakePointerVariable(Id id, const ShaderVariable *v, uint8_t scalar0 = 0xff, diff --git a/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp b/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp index d104ca143..d798f5af9 100644 --- a/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp +++ b/renderdoc/driver/shaders/spirv/spirv_debug_setup.cpp @@ -2597,15 +2597,25 @@ rdcarray Debugger::ContinueDebug() } // globals won't be filled out by entering the entry point, ensure their change is registered. + ShaderVariable val; + DeviceOpResult opResult; for(const Id &v : liveGlobals) - initial.changes.push_back({ShaderVariable(), GetPointerValue(active.ids[v])}); + { + opResult = GetPointerValue(active.ids[v], val); + RDCASSERTEQUAL(opResult, DeviceOpResult::Succeeded); + initial.changes.push_back({ShaderVariable(), val}); + } if(m_DebugInfo.valid) { // debug info can refer to constants for source variable values. Add an initial change for any // that are so referenced for(const Id &v : m_DebugInfo.constants) - initial.changes.push_back({ShaderVariable(), GetPointerValue(active.ids[v])}); + { + opResult = GetPointerValue(active.ids[v], val); + RDCASSERTEQUAL(opResult, DeviceOpResult::Succeeded); + initial.changes.push_back({ShaderVariable(), val}); + } } ret.push_back(std::move(initial)); @@ -3143,36 +3153,39 @@ DebugAPIWrapper::TextureType Debugger::GetTextureType(const ShaderVariable &img) return getTextureType(img); } -ShaderVariable Debugger::GetPointerValue(const ShaderVariable &ptr) const +DeviceOpResult Debugger::GetPointerValue(const ShaderVariable &ptr, ShaderVariable &ret) const { // opaque pointers display as their inner value if(IsOpaquePointer(ptr)) { const ShaderVariable *inner = getPointer(ptr); - ShaderVariable ret = *inner; + ret = *inner; ret.name = ptr.name; // inherit any array index from the pointer ShaderBindIndex bind = ret.GetBindIndex(); bind.arrayElement = getBindArrayIndex(ptr); ret.SetBindIndex(bind); - return ret; + return DeviceOpResult::Succeeded; } // physical pointers which haven't been dereferenced are returned as-is, they're ready for display else if(IsPhysicalPointer(ptr) && !checkPointerFlags(ptr, PointerFlags::DereferencedPhysical)) { - return ptr; + ret = ptr; + return DeviceOpResult::Succeeded; } // every other kind of pointer displays as its contents - return ReadFromPointer(ptr); + return ReadFromPointer(ptr, ret); } -ShaderVariable Debugger::ReadFromPointer(const ShaderVariable &ptr) const +DeviceOpResult Debugger::ReadFromPointer(const ShaderVariable &ptr, ShaderVariable &ret) const { if(ptr.type != VarType::GPUPointer) - return ptr; + { + ret = ptr; + return DeviceOpResult::Succeeded; + } - ShaderVariable ret; // values for setting up pointer reads, either from a physical pointer or from an opaque pointer rdcspv::Id typeId; Decorations parentDecorations; @@ -3182,7 +3195,10 @@ ShaderVariable Debugger::ReadFromPointer(const ShaderVariable &ptr) const std::function pointerReadCallback; if(IsPhysicalPointer(ptr)) { - CHECK_DEBUGGER_THREAD(); + baseAddress = ptr.GetPointer().pointer; + if(!IsDeviceThread() && !apiWrapper->IsBufferCached(baseAddress)) + return DeviceOpResult::NeedsDevice; + if(checkPointerFlags(ptr, PointerFlags::DereferencedPhysical)) typeId = getBufferTypeId(ptr); else @@ -3203,7 +3219,6 @@ ShaderVariable Debugger::ReadFromPointer(const ShaderVariable &ptr) const Decorations::Flags(parentDecorations.flags | Decorations::HasMatrixStride); parentDecorations.matrixStride = varMatrixStride; } - baseAddress = ptr.GetPointer().pointer; pointerReadCallback = [this, baseAddress](uint64_t offset, uint64_t size, void *dst) { apiWrapper->ReadAddress(baseAddress + offset, size, dst); }; @@ -3213,11 +3228,13 @@ ShaderVariable Debugger::ReadFromPointer(const ShaderVariable &ptr) const const ShaderVariable *inner = getPointer(ptr); if(inner->type == VarType::ReadWriteResource && checkPointerFlags(*inner, PointerFlags::SSBO)) { - CHECK_DEBUGGER_THREAD(); typeId = getBufferTypeId(ptr); byteOffset = getByteOffset(ptr); bind = inner->GetBindIndex(); bind.arrayElement = getBindArrayIndex(ptr); + if(!IsDeviceThread() && !apiWrapper->IsBufferCached(bind)) + return DeviceOpResult::NeedsDevice; + uint32_t varMatrixStride = getMatrixStride(ptr); if(varMatrixStride != 0) { @@ -3320,7 +3337,7 @@ ShaderVariable Debugger::ReadFromPointer(const ShaderVariable &ptr) const rdcstr(), readCallback); ret.name = ptr.name; - return ret; + return DeviceOpResult::Succeeded; } // this is the case of 'reading' from a pointer where the data is entirely contained within the @@ -3384,7 +3401,7 @@ ShaderVariable Debugger::ReadFromPointer(const ShaderVariable &ptr) const } } - return ret; + return DeviceOpResult::Succeeded; } Id Debugger::GetPointerBaseId(const ShaderVariable &ptr) const @@ -3450,6 +3467,10 @@ DeviceOpResult Debugger::WriteThroughPointer(ShaderVariable &ptr, const ShaderVa if(IsPhysicalPointer(ptr)) { + baseAddress = ptr.GetPointer().pointer; + if(!IsDeviceThread() && !apiWrapper->IsBufferCached(baseAddress)) + return DeviceOpResult::NeedsDevice; + if(checkPointerFlags(ptr, PointerFlags::DereferencedPhysical)) typeId = getBufferTypeId(ptr); else @@ -3468,7 +3489,6 @@ DeviceOpResult Debugger::WriteThroughPointer(ShaderVariable &ptr, const ShaderVa Decorations::Flags(parentDecorations.flags | Decorations::HasMatrixStride); parentDecorations.matrixStride = varMatrixStride; } - baseAddress = ptr.GetPointer().pointer; pointerWriteCallback = [this, baseAddress](uint64_t offset, uint64_t size, const void *src) { apiWrapper->WriteAddress(baseAddress + offset, size, src); }; @@ -3482,6 +3502,9 @@ DeviceOpResult Debugger::WriteThroughPointer(ShaderVariable &ptr, const ShaderVa byteOffset = getByteOffset(ptr); bind = inner->GetBindIndex(); bind.arrayElement = getBindArrayIndex(ptr); + if(!IsDeviceThread() && !apiWrapper->IsBufferCached(bind)) + return DeviceOpResult::NeedsDevice; + uint32_t varMatrixStride = getMatrixStride(ptr); if(varMatrixStride != 0) { @@ -4737,7 +4760,9 @@ void Debugger::InternalStepThread(uint32_t lane, rdcarray *ret { thread.live.erase(l); ShaderVariableChange change; - change.before = GetPointerValue(thread.ids[id]); + DeviceOpResult opResult = GetPointerValue(thread.ids[id], change.before); + // The variable was live and written to, it should be cached + RDCASSERTEQUAL(opResult, DeviceOpResult::Succeeded); activeDebugState.changes.push_back(change); continue; } diff --git a/renderdoc/driver/vulkan/vk_shaderdebug.cpp b/renderdoc/driver/vulkan/vk_shaderdebug.cpp index 6034ec20d..8d1dee0ba 100644 --- a/renderdoc/driver/vulkan/vk_shaderdebug.cpp +++ b/renderdoc/driver/vulkan/vk_shaderdebug.cpp @@ -295,48 +295,78 @@ public: virtual ResourceId GetShaderID() override { return m_ShaderID; } - virtual uint64_t GetBufferLength(ShaderBindIndex bind) override + virtual uint64_t GetBufferLength(const ShaderBindIndex &bind) override { - CHECK_DEVICE_THREAD(); - return PopulateBuffer(bind).size(); + rdcspv::DeviceOpResult opResult; + size_t length = 0; + // BufferFunction guarantees the buffer cache readlock whilst the function is called + bool succeeded = BufferFunction( + bind, [&length](bytebuf *data) { length = data->size(); }, opResult); + RDCASSERT(succeeded); + RDCASSERTEQUAL(opResult, rdcspv::DeviceOpResult::Succeeded); + return length; } - virtual void ReadBufferValue(ShaderBindIndex bind, uint64_t offset, uint64_t byteSize, + virtual void ReadBufferValue(const ShaderBindIndex &bind, uint64_t offset, uint64_t byteSize, void *dst) override { - CHECK_DEVICE_THREAD(); - const bytebuf &data = PopulateBuffer(bind); - - if(offset + byteSize <= data.size()) - memcpy(dst, data.data() + (size_t)offset, (size_t)byteSize); + rdcspv::DeviceOpResult opResult; + // BufferFunction guarantees the buffer cache readlock whilst the function is called + bool succeeded = BufferFunction( + bind, + [offset, byteSize, dst](bytebuf *data) { + if(offset + byteSize <= data->size()) + memcpy(dst, data->data() + (size_t)offset, (size_t)byteSize); + }, + opResult); + RDCASSERT(succeeded); + RDCASSERTEQUAL(opResult, rdcspv::DeviceOpResult::Succeeded); } - virtual void WriteBufferValue(ShaderBindIndex bind, uint64_t offset, uint64_t byteSize, + virtual void WriteBufferValue(const ShaderBindIndex &bind, uint64_t offset, uint64_t byteSize, const void *src) override { - CHECK_DEVICE_THREAD(); - bytebuf &data = PopulateBuffer(bind); - - if(offset + byteSize <= data.size()) - memcpy(data.data() + (size_t)offset, src, (size_t)byteSize); + rdcspv::DeviceOpResult opResult; + // BufferFunction guarantees the buffer cache readlock whilst the function is called + bool succeeded = BufferFunction( + bind, + [offset, byteSize, src](bytebuf *data) { + if(offset + byteSize <= data->size()) + memcpy(data->data() + (size_t)offset, src, (size_t)byteSize); + }, + opResult); + RDCASSERT(succeeded); + RDCASSERTEQUAL(opResult, rdcspv::DeviceOpResult::Succeeded); } virtual void ReadAddress(uint64_t address, uint64_t byteSize, void *dst) override { - CHECK_DEVICE_THREAD(); - size_t offset; - const bytebuf &data = PopulateBuffer(address, offset); - if(offset + byteSize <= data.size()) - memcpy(dst, data.data() + offset, (size_t)byteSize); + rdcspv::DeviceOpResult opResult; + // BufferFunction guarantees the buffer cache readlock whilst the function is called + bool succeeded = BufferFunction( + address, + [byteSize, dst](bytebuf *data, size_t offset) { + if(offset + byteSize <= data->size()) + memcpy(dst, data->data() + offset, (size_t)byteSize); + }, + opResult); + RDCASSERT(succeeded); + RDCASSERTEQUAL(opResult, rdcspv::DeviceOpResult::Succeeded); } virtual void WriteAddress(uint64_t address, uint64_t byteSize, const void *src) override { - CHECK_DEVICE_THREAD(); - size_t offset; - bytebuf &data = PopulateBuffer(address, offset); - if(offset + byteSize <= data.size()) - memcpy(data.data() + offset, src, (size_t)byteSize); + rdcspv::DeviceOpResult opResult; + // BufferFunction guarantees the buffer cache readlock whilst the function is called + bool succeeded = BufferFunction( + address, + [byteSize, src](bytebuf *data, size_t offset) { + if(offset + byteSize <= data->size()) + memcpy(data->data() + offset, src, (size_t)byteSize); + }, + opResult); + RDCASSERT(succeeded); + RDCASSERTEQUAL(opResult, rdcspv::DeviceOpResult::Succeeded); } // Called from any thread @@ -1768,6 +1798,7 @@ private: typedef rdcpair SamplerBiasKey; std::map m_BiasSamplers; + Threading::RWLock bufferCacheLock; std::map bufferCache; VkCommandBuffer queuedOpCmdBuffer = VK_NULL_HANDLE; @@ -1866,33 +1897,175 @@ private: return m_SamplerDescriptors[a]; } - bytebuf &PopulateBuffer(uint64_t address, size_t &offs) + // Called from any thread + ShaderBindIndex GenerateBufferBind(const uint64_t address, size_t &offs) + { + ResourceId id; + uint64_t ptrOffs; + m_pDriver->GetResIDFromAddr(address, id, ptrOffs); + offs = size_t(ptrOffs); + + ShaderBindIndex bind; + bind.arrayElement = (address - offs) & 0xFFFFFFFFU; + + return bind; + } + + // Called from any thread + bool IsBufferCached(const ShaderBindIndex &bind) override + { + SCOPED_READLOCK(bufferCacheLock); + return bufferCache.find(bind) != bufferCache.end(); + } + + // Called from any thread + bool IsBufferCached(uint64_t address) override + { + size_t offs = 0; + ShaderBindIndex bind = GenerateBufferBind(address, offs); + return IsBufferCached(bind); + } + + // Called from any thread + bytebuf *GetBufferDataFromCache(const ShaderBindIndex &bind, rdcspv::DeviceOpResult &opResult) + { + // Calling function responsible for acquiring bufferCache Read lock + auto findIt = bufferCache.find(bind); + if(findIt != bufferCache.end()) + { + opResult = rdcspv::DeviceOpResult::Succeeded; + return &findIt->second; + } + + opResult = rdcspv::DeviceOpResult::Failed; + + // Not in the cache : populate must happen on the device thread + if(!IsDeviceThread()) + opResult = rdcspv::DeviceOpResult::NeedsDevice; + + return NULL; + } + + // Called from any thread + bool BufferFunction(const ShaderBindIndex &bind, const std::function &func, + rdcspv::DeviceOpResult &opResult) + { + bool isCached = false; + { + SCOPED_READLOCK(bufferCacheLock); + isCached = GetBufferDataFromCache(bind, opResult) != NULL; + if(opResult == rdcspv::DeviceOpResult::NeedsDevice) + return false; + } + + if(!isCached) + { + // Add buffer data to the cache : cache should not be locked by this thread + PopulateBuffer(bind); + } + + { + SCOPED_READLOCK(bufferCacheLock); + bytebuf *result = GetBufferDataFromCache(bind, opResult); + if(result) + { + // Guarantee the buffer cache readlock whilst the function is called + func(result); + return true; + } + + RDCASSERTEQUAL(opResult, rdcspv::DeviceOpResult::Failed); + opResult = rdcspv::DeviceOpResult::Failed; + return false; + } + } + + // Called from any thread + bool BufferFunction(uint64_t address, const std::function &func, + rdcspv::DeviceOpResult &opResult) + { + size_t offs = 0; + ShaderBindIndex bind = GenerateBufferBind(address, offs); + bool isCached = false; + { + SCOPED_READLOCK(bufferCacheLock); + isCached = GetBufferDataFromCache(bind, opResult) != NULL; + if(opResult == rdcspv::DeviceOpResult::NeedsDevice) + return false; + } + + if(!isCached) + { + // Add buffer data to the cache : cache should not be locked by this thread + PopulateBuffer(address, bind); + } + + { + SCOPED_READLOCK(bufferCacheLock); + bytebuf *result = GetBufferDataFromCache(bind, opResult); + if(result) + { + // Guarantee the buffer cache readlock whilst the function is called + func(result, offs); + return true; + } + + RDCASSERTEQUAL(opResult, rdcspv::DeviceOpResult::Failed); + opResult = rdcspv::DeviceOpResult::Failed; + return false; + } + } + + // Must be called from the replay manager thread (the debugger thread) + void PopulateBuffer(uint64_t address, ShaderBindIndex bind) { CHECK_DEVICE_THREAD(); // pick a non-overlapping bind namespace for direct pointer access - ShaderBindIndex bind; ResourceId id; uint64_t ptrOffs; m_pDriver->GetResIDFromAddr(address, id, ptrOffs); if(id == ResourceId()) { + ShaderBindIndex noBind; CHECK_DEVICE_THREAD(); - bind.arrayElement = 0; - auto insertIt = bufferCache.insert(std::make_pair(bind, bytebuf())); + auto insertIt = bufferCache.insert(std::make_pair(noBind, bytebuf())); m_pDriver->AddDebugMessage(MessageCategory::Execution, MessageSeverity::High, MessageSource::RuntimeWarning, StringFormat::Fmt("invalid or OOB pointer access detected .")); - return insertIt.first->second; + return; } - bind.arrayElement = (address - offs) & 0xFFFFFFFFU; - offs = size_t(ptrOffs); - auto insertIt = bufferCache.insert(std::make_pair(bind, bytebuf())); - bytebuf &data = insertIt.first->second; - if(insertIt.second) + // if the resources might be dirty from side-effects from the action, replay back to right + // before it. + if(m_ResourcesDirty) + { + VkMarkerRegion region("un-dirtying resources"); + m_pDriver->ReplayLog(0, m_EventID, eReplay_WithoutDraw); + m_ResourcesDirty = false; + } + + bytebuf data; + m_pDriver->GetDebugManager()->GetBufferData(id, 0, 0, data); + + { + // Insert atomically with all the data filled in : to prevent race conditions + SCOPED_WRITELOCK(bufferCacheLock); + auto insertIt = bufferCache.insert(std::make_pair(bind, data)); + RDCASSERT(insertIt.second); + } + } + + // Must be called from the replay manager thread (the debugger thread) + void PopulateBuffer(const ShaderBindIndex &bind) + { + CHECK_DEVICE_THREAD(); + bytebuf data; + + bool valid = true; + const Descriptor &bufData = GetDescriptor("accessing buffer value", bind, valid); + if(valid) { - CHECK_DEVICE_THREAD(); // if the resources might be dirty from side-effects from the action, replay back to right // before it. if(m_ResourcesDirty) @@ -1901,42 +2074,21 @@ private: m_pDriver->ReplayLog(0, m_EventID, eReplay_WithoutDraw); m_ResourcesDirty = false; } - m_pDriver->GetDebugManager()->GetBufferData(id, 0, 0, data); - } - return data; - } - bytebuf &PopulateBuffer(ShaderBindIndex bind) - { - CHECK_DEVICE_THREAD(); - auto insertIt = bufferCache.insert(std::make_pair(bind, bytebuf())); - bytebuf &data = insertIt.first->second; - if(insertIt.second) - { - CHECK_DEVICE_THREAD(); - bool valid = true; - const Descriptor &bufData = GetDescriptor("accessing buffer value", bind, valid); - if(valid) + if(bufData.resource != ResourceId()) { - // if the resources might be dirty from side-effects from the action, replay back to right - // before it. - if(m_ResourcesDirty) - { - VkMarkerRegion region("un-dirtying resources"); - m_pDriver->ReplayLog(0, m_EventID, eReplay_WithoutDraw); - m_ResourcesDirty = false; - } - - if(bufData.resource != ResourceId()) - { - m_pDriver->GetReplay()->GetBufferData( - m_pDriver->GetResourceManager()->GetLiveID(bufData.resource), bufData.byteOffset, - bufData.byteSize, data); - } + m_pDriver->GetReplay()->GetBufferData( + m_pDriver->GetResourceManager()->GetLiveID(bufData.resource), bufData.byteOffset, + bufData.byteSize, data); } } - return data; + { + // Insert atomically with all the data filled in : to prevent race conditions + SCOPED_WRITELOCK(bufferCacheLock); + auto insertIt = bufferCache.insert(std::make_pair(bind, data)); + RDCASSERT(insertIt.second); + } } // Must be called from the replay manager thread (the debugger thread)