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;
