Vulkan/Debug: Various fixes / improvements to Thread Remove the `Context::Lock` parameter from `enter()`. We don't want to force the lock to be held for the entire duration of the call. Add `UpdateFrame` type alias. Add the `UpdateFrame` parameter to `enter()` - you usually want to update the frame as soon as you've pushed the stack. Switch `pauseAtFrame` from a `shared_ptr` to a `weak_ptr`. If the frame is released before the thread finds it to pause, then the thread will instead pause at the next frame (following the logic of a null `pauseAtFrame`). Fixed the assignment of `pauseAtFrame` in `stepOut()` - it was looking at the current frame, not the next one up. Bug: b/145351270 Change-Id: I1cb85af58c666a7793145480abc95a4ec82e1859 Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/48695 Tested-by: Ben Clayton <bclayton@google.com> Reviewed-by: Antonio Maiorano <amaiorano@google.com> Kokoro-Result: kokoro <noreply+kokoro@google.com>
diff --git a/src/Pipeline/SpirvShaderDebugger.cpp b/src/Pipeline/SpirvShaderDebugger.cpp index 432b937..76f1fad 100644 --- a/src/Pipeline/SpirvShaderDebugger.cpp +++ b/src/Pipeline/SpirvShaderDebugger.cpp
@@ -856,10 +856,10 @@ static State *create(const Debugger *debugger, const char *name); static void destroy(State *); - State(const Debugger *debugger, const char *stackBase, vk::dbg::Context::Lock &lock); + State(const Debugger *debugger, const char *stackBase); ~State(); - void enter(vk::dbg::Context::Lock &lock, const char *name); + void enter(const char *name); void exit(); void updateActiveLaneMask(int lane, bool enabled); void updateLocation(bool isStep, vk::dbg::File::ID file, int line, int column); @@ -905,8 +905,7 @@ SpirvShader::Impl::Debugger::State *SpirvShader::Impl::Debugger::State::create(const Debugger *debugger, const char *name) { - auto lock = debugger->ctx->lock(); - return new State(debugger, name, lock); + return new State(debugger, name); } void SpirvShader::Impl::Debugger::State::destroy(State *state) @@ -914,13 +913,13 @@ delete state; } -SpirvShader::Impl::Debugger::State::State(const Debugger *debugger, const char *stackBase, vk::dbg::Context::Lock &lock) +SpirvShader::Impl::Debugger::State::State(const Debugger *debugger, const char *stackBase) : debugger(debugger) - , thread(lock.currentThread()) + , thread(debugger->ctx->lock().currentThread()) , shadow(new uint8_t[debugger->shadow.size]) , initialThreadDepth(thread->depth()) { - enter(lock, stackBase); + enter(stackBase); thread->update(true, [&](vk::dbg::Frame &frame) { globals.locals = frame.locals; @@ -942,9 +941,9 @@ } } -void SpirvShader::Impl::Debugger::State::enter(vk::dbg::Context::Lock &lock, const char *name) +void SpirvShader::Impl::Debugger::State::enter(const char *name) { - thread->enter(lock, debugger->spirvFile, name); + thread->enter(debugger->spirvFile, name); } void SpirvShader::Impl::Debugger::State::exit() @@ -1058,8 +1057,7 @@ if(hasDebuggerScope(srcScope->scope)) { - auto lock = debugger->ctx->lock(); - auto thread = lock.currentThread(); + auto thread = debugger->ctx->lock().currentThread(); debug::Function *oldFunction = oldSrcScope ? debug::find<debug::Function>(oldSrcScope->scope) : nullptr; debug::Function *newFunction = newSrcScope ? debug::find<debug::Function>(newSrcScope->scope) : nullptr; @@ -1067,7 +1065,7 @@ if(oldFunction != newFunction) { if(oldFunction) { thread->exit(); } - if(newFunction) { thread->enter(lock, newFunction->source->dbgFile, newFunction->name); } + if(newFunction) { thread->enter(newFunction->source->dbgFile, newFunction->name); } } auto dbgScope = getScopes(srcScope->scope);
diff --git a/src/Vulkan/Debug/Thread.cpp b/src/Vulkan/Debug/Thread.cpp index 9e57bc7..2f4dcc0 100644 --- a/src/Vulkan/Debug/Thread.cpp +++ b/src/Vulkan/Debug/Thread.cpp
@@ -23,7 +23,7 @@ Thread::Thread(ID id, Context *ctx) : id(id) - , broadcast(ctx->serverEventBroadcast()) + , ctx(ctx) {} void Thread::setName(const std::string &name) @@ -46,7 +46,7 @@ { if(location.file->hasBreakpoint(location.line)) { - broadcast->onLineBreakpointHit(id); + ctx->serverEventBroadcast()->onLineBreakpointHit(id); state_ = State::Paused; } } @@ -61,12 +61,23 @@ case State::Stepping: { - if(!pauseAtFrame || pauseAtFrame == frames.back()) + bool pause = false; + { - broadcast->onThreadStepped(id); + auto frame = pauseAtFrame.lock(); + pause = !frame; // Pause if there's no pause-at-frame... + if(frame == frames.back()) // ... or if we've reached the pause-at-frame + { + pause = true; + pauseAtFrame.reset(); + } + } + + if(pause) + { + ctx->serverEventBroadcast()->onThreadStepped(id); state_ = State::Paused; lock.wait(stateCV, [this]() REQUIRES(mutex) { return state_ != State::Paused; }); - pauseAtFrame = 0; } break; } @@ -76,17 +87,29 @@ } } -void Thread::enter(Context::Lock &ctxlck, const std::shared_ptr<File> &file, const std::string &function) +void Thread::enter(const std::shared_ptr<File> &file, const std::string &function, const UpdateFrame &f) { - auto frame = ctxlck.createFrame(file, function); - auto isFunctionBreakpoint = ctxlck.isFunctionBreakpoint(function); - - marl::lock lock(mutex); - frames.push_back(frame); - if(isFunctionBreakpoint) + std::shared_ptr<Frame> frame; + bool isFunctionBreakpoint; { - broadcast->onFunctionBreakpointHit(id); - state_ = State::Paused; + auto lock = ctx->lock(); + frame = lock.createFrame(file, function); + isFunctionBreakpoint = lock.isFunctionBreakpoint(function); + } + + { + marl::lock lock(mutex); + frames.push_back(frame); + + if(f) { f(*frame); } + + if(isFunctionBreakpoint) + { + ctx->serverEventBroadcast()->onFunctionBreakpointHit(id); + state_ = State::Paused; + } + + onLocationUpdate(lock); } } @@ -96,7 +119,7 @@ frames.pop_back(); } -void Thread::update(bool isStep, std::function<void(Frame &)> f) +void Thread::update(bool isStep, const UpdateFrame &f) { marl::lock lock(mutex); auto &frame = *frames.back(); @@ -180,7 +203,7 @@ { marl::lock lock(mutex); state_ = State::Stepping; - pauseAtFrame = (frames.size() > 1) ? frames[frames.size() - 1] : nullptr; + pauseAtFrame = (frames.size() > 1) ? frames[frames.size() - 2] : nullptr; stateCV.notify_all(); }
diff --git a/src/Vulkan/Debug/Thread.hpp b/src/Vulkan/Debug/Thread.hpp index 184d396..d9ba32d 100644 --- a/src/Vulkan/Debug/Thread.hpp +++ b/src/Vulkan/Debug/Thread.hpp
@@ -107,6 +107,8 @@ public: using ID = dbg::ID<Thread>; + using UpdateFrame = std::function<void(Frame &)>; + // The current execution state. enum class State { @@ -124,8 +126,8 @@ std::string name() const; // enter() pushes the thread's stack with a new frame created with the given - // file and function. - void enter(Context::Lock &lock, const std::shared_ptr<File> &file, const std::string &function); + // file and function, then calls f to modify the new frame of the stack. + void enter(const std::shared_ptr<File> &file, const std::string &function, const UpdateFrame &f = nullptr); // exit() pops the thread's stack frame. void exit(); @@ -150,7 +152,7 @@ // from full line updates. Note that we cannot simply examine line position // changes as single-line loops such as `while(true) { foo(); }` would not // be correctly steppable. - void update(bool isStep, std::function<void(Frame &)> f); + void update(bool isStep, const UpdateFrame &f); // resume() resumes execution of the thread by unblocking a call to // update() and setting the thread's state to State::Running. @@ -181,7 +183,7 @@ const ID id; private: - ServerEventListener *const broadcast; + Context *const ctx; void onLocationUpdate(marl::lock &lock) REQUIRES(mutex); @@ -189,7 +191,7 @@ std::string name_ GUARDED_BY(mutex); std::vector<std::shared_ptr<Frame>> frames GUARDED_BY(mutex); State state_ GUARDED_BY(mutex) = State::Running; - std::shared_ptr<Frame> pauseAtFrame GUARDED_BY(mutex); + std::weak_ptr<Frame> pauseAtFrame GUARDED_BY(mutex); std::condition_variable stateCV; };
diff --git a/src/Vulkan/VkCommandBuffer.cpp b/src/Vulkan/VkCommandBuffer.cpp index 87ec112..2fde442 100644 --- a/src/Vulkan/VkCommandBuffer.cpp +++ b/src/Vulkan/VkCommandBuffer.cpp
@@ -1810,11 +1810,9 @@ auto debuggerContext = device->getDebuggerContext(); if(debuggerContext) { - auto lock = debuggerContext->lock(); - debuggerThread = lock.currentThread(); + debuggerThread = debuggerContext->lock().currentThread(); debuggerThread->setName("vkQueue processor"); - debuggerThread->enter(lock, debuggerFile, "vkCommandBuffer::submit"); - lock.unlock(); + debuggerThread->enter(debuggerFile, "vkCommandBuffer::submit"); } defer(if(debuggerThread) { debuggerThread->exit(); }); int line = 1;