Vulkan/Debug: Don't step for column updates
OpLine is produced for any line / column change.
Column information may be useful for stack frame information (say two function calls on the same line), diagnosing a crash in a complex expression, or some other tooling. However, it's not so useful for IDE debugger stepping, unless you have a location for the span end (which we don't).
Bug: b/155390706
Bug: b/148401179
Change-Id: Ic416c6e3c47044d707bfa36112e1153588669424
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/44549
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Tested-by: Ben Clayton <bclayton@google.com>
diff --git a/src/Pipeline/SpirvShaderDebugger.cpp b/src/Pipeline/SpirvShaderDebugger.cpp
index e936a4d..7011cb8 100644
--- a/src/Pipeline/SpirvShaderDebugger.cpp
+++ b/src/Pipeline/SpirvShaderDebugger.cpp
@@ -451,6 +451,7 @@
void process(const SpirvShader *shader, const InsnIterator &insn, EmitState *state, Pass pass);
+ void setNextSetLocationIsStep();
void setLocation(EmitState *state, const std::shared_ptr<vk::dbg::File> &, int line, int column);
void setLocation(EmitState *state, const std::string &path, int line, int column);
@@ -514,6 +515,8 @@
void defineOrEmit(InsnIterator insn, Pass pass, F &&emit);
std::unordered_map<std::string, std::shared_ptr<vk::dbg::File>> files;
+ bool nextSetLocationIsStep = true;
+ int lastSetLocationLine = 0;
};
////////////////////////////////////////////////////////////////////////////////
@@ -533,7 +536,7 @@
void enter(vk::dbg::Context::Lock &lock, const char *name);
void exit();
void updateActiveLaneMask(int lane, bool enabled);
- void updateLocation(vk::dbg::File::ID file, int line, int column);
+ void updateLocation(bool isStep, vk::dbg::File::ID file, int line, int column);
void createScope(const debug::Scope *);
void setScope(debug::SourceScope *newScope);
@@ -595,7 +598,7 @@
builtinsByLane[i] = lock.createVariableContainer();
}
- thread->update([&](vk::dbg::Frame &frame) {
+ thread->update(true, [&](vk::dbg::Frame &frame) {
rootScopes.locals = frame.locals;
rootScopes.hovers = frame.hovers;
for(int i = 0; i < sw::SIMD::Width; i++)
@@ -631,10 +634,10 @@
rootScopes.localsByLane[lane]->put("enabled", vk::dbg::make_constant(enabled));
}
-void SpirvShader::Impl::Debugger::State::updateLocation(vk::dbg::File::ID fileID, int line, int column)
+void SpirvShader::Impl::Debugger::State::updateLocation(bool isStep, vk::dbg::File::ID fileID, int line, int column)
{
auto file = debugger->ctx->lock().get(fileID);
- thread->update([&](vk::dbg::Frame &frame) {
+ thread->update(isStep, [&](vk::dbg::Frame &frame) {
frame.location = { file, line, column };
});
}
@@ -735,7 +738,7 @@
}
auto dbgScope = getScopes(srcScope->scope);
- thread->update([&](vk::dbg::Frame &frame) {
+ thread->update(true, [&](vk::dbg::Frame &frame) {
frame.locals = dbgScope.locals;
frame.hovers = dbgScope.hovers;
});
@@ -1137,9 +1140,21 @@
}
}
+void SpirvShader::Impl::Debugger::setNextSetLocationIsStep()
+{
+ nextSetLocationIsStep = true;
+}
+
void SpirvShader::Impl::Debugger::setLocation(EmitState *state, const std::shared_ptr<vk::dbg::File> &file, int line, int column)
{
- rr::Call(&State::updateLocation, state->routine->dbgState, file->id, line, column);
+ if(line != lastSetLocationLine)
+ {
+ // If the line number has changed, then this is always a step.
+ nextSetLocationIsStep = true;
+ lastSetLocationLine = line;
+ }
+ rr::Call(&State::updateLocation, state->routine->dbgState, nextSetLocationIsStep, file->id, line, column);
+ nextSetLocationIsStep = false;
}
void SpirvShader::Impl::Debugger::setLocation(EmitState *state, const std::string &path, int line, int column)
@@ -1579,6 +1594,16 @@
if(auto dbg = impl.debugger)
{
+ if(insn.opcode() == spv::OpLabel)
+ {
+ // Whenever we hit a label, force the next OpLine to be steppable.
+ // This handles the case where we have control flow on the same line
+ // For example:
+ // while(true) { foo(); }
+ // foo() should be repeatedly steppable.
+ dbg->setNextSetLocationIsStep();
+ }
+
if(extensionsImported.count(Extension::OpenCLDebugInfo100) == 0)
{
// We're emitting debugger logic for SPIR-V.
diff --git a/src/Vulkan/Debug/Thread.cpp b/src/Vulkan/Debug/Thread.cpp
index 5c819f7..8228465 100644
--- a/src/Vulkan/Debug/Thread.cpp
+++ b/src/Vulkan/Debug/Thread.cpp
@@ -96,15 +96,22 @@
frames.pop_back();
}
-void Thread::update(std::function<void(Frame &)> f)
+void Thread::update(bool isStep, std::function<void(Frame &)> f)
{
marl::lock lock(mutex);
auto &frame = *frames.back();
- auto oldLocation = frame.location;
- f(frame);
- if(frame.location != oldLocation)
+ if(isStep)
{
- onLocationUpdate(lock);
+ auto oldLocation = frame.location;
+ f(frame);
+ if(frame.location != oldLocation)
+ {
+ onLocationUpdate(lock);
+ }
+ }
+ else
+ {
+ f(frame);
}
}
diff --git a/src/Vulkan/Debug/Thread.hpp b/src/Vulkan/Debug/Thread.hpp
index cf946a9..690fb37 100644
--- a/src/Vulkan/Debug/Thread.hpp
+++ b/src/Vulkan/Debug/Thread.hpp
@@ -143,9 +143,14 @@
State state() const;
// update() calls f to modify the top most frame of the stack.
- // If the frame's location is changed, update() potentially blocks until the
- // thread is resumed with one of the methods below.
- void update(std::function<void(Frame &)> f);
+ // If the frame's location is changed and isStep is true, update()
+ // potentially blocks until the thread is resumed with one of the methods
+ // below.
+ // isStep is used to distinguish same-statement column position updates
+ // 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);
// resume() resumes execution of the thread by unblocking a call to
// update() and setting the thread's state to State::Running.
diff --git a/src/Vulkan/VkCommandBuffer.cpp b/src/Vulkan/VkCommandBuffer.cpp
index d46620d..f6075b0 100644
--- a/src/Vulkan/VkCommandBuffer.cpp
+++ b/src/Vulkan/VkCommandBuffer.cpp
@@ -1805,7 +1805,7 @@
#ifdef ENABLE_VK_DEBUGGER
if(debuggerThread)
{
- debuggerThread->update([&](vk::dbg::Frame &frame) {
+ debuggerThread->update(true, [&](vk::dbg::Frame &frame) {
frame.location = { debuggerFile, line++, 0 };
});
}
