SpirvShaderDebugger: Improve stepping for inlined functions
Makes stepping around inlined functions less jumpy.
Fixes: b/170650010
Change-Id: Ie7b003722e3dee523a39bedf4dc42f1c0b05a041
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/49228
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
Tested-by: Ben Clayton <bclayton@google.com>
diff --git a/src/Pipeline/SpirvShaderDebugger.cpp b/src/Pipeline/SpirvShaderDebugger.cpp
index 45c01ba..2280fa0 100644
--- a/src/Pipeline/SpirvShaderDebugger.cpp
+++ b/src/Pipeline/SpirvShaderDebugger.cpp
@@ -2200,7 +2200,7 @@
auto shrink = [&](size_t maxLen) {
while(stack.size() > maxLen)
{
- thread->exit();
+ thread->exit(true);
stack.pop_back();
}
};
@@ -2216,10 +2216,10 @@
{
if(stack[i] != newStack[i])
{
- bool isTopMostFrame = i == (newStack.size() - 1);
+ bool wasTopMostFrame = i == (stack.size() - 1);
auto oldFunction = debug::find<debug::Function>(stack[i].block);
auto newFunction = debug::find<debug::Function>(newStack[i].block);
- if(isTopMostFrame && oldFunction == newFunction)
+ if(wasTopMostFrame && oldFunction == newFunction)
{
// Deviation is just a movement in the top most frame's
// function.
@@ -2227,8 +2227,20 @@
// be treated as a step out and step in, breaking stepping
// commands. Instead, just update the frame variables for
// the new scope.
- stack[i].block = newStack[i].block;
+ stack[i] = newStack[i];
thread->update(true, [&](vk::dbg::Frame &frame) {
+ // Update the frame location if we're entering a
+ // function. This allows the debugger to pause at the
+ // line (which may not have any instructions or OpLines)
+ // of a inlined function call. This is less jarring
+ // than magically appearing in another function before
+ // you've reached the line of the call site.
+ // See b/170650010 for more context.
+ if(stack.size() < newStack.size())
+ {
+ auto function = debug::find<debug::Function>(stack[i].block);
+ frame.location = vk::dbg::Location{ function->source->dbgFile, (int)stack[i].line };
+ }
updateFrameLocals(state, frame, stack[i].block);
});
}
@@ -2240,17 +2252,34 @@
}
}
- // Now rebuild the parts of stack frames that are new
+ // Now rebuild the parts of stack frames that are new.
+ //
+ // This is done in two stages:
+ // (1) thread->enter() is called to construct the new stack frame with
+ // the opening scope line. The frames locals and hovers are built
+ // and assigned.
+ // (2) thread->update() is called to adjust the frame's location to
+ // entry.line. This may be different to the function entry in the
+ // case of multiple nested inline functions. If its the same, then
+ // this is a no-op.
+ //
+ // This two-stage approach allows the debugger to step through chains of
+ // inlined function calls without having a jarring jump from the outer
+ // function to the first statement within the function.
+ // See b/170650010 for more context.
for(size_t i = stack.size(); i < newStack.size(); i++)
{
auto entry = newStack[i];
stack.emplace_back(entry);
auto function = debug::find<debug::Function>(entry.block);
thread->enter(entry.block->source->dbgFile, function->name, [&](vk::dbg::Frame &frame) {
- frame.location = vk::dbg::Location{ function->source->dbgFile, (int)entry.line };
+ frame.location = vk::dbg::Location{ function->source->dbgFile, (int)function->line };
frame.hovers->variables->extend(std::make_shared<HoversFromLocals>(frame.locals->variables));
updateFrameLocals(state, frame, entry.block);
});
+ thread->update(true, [&](vk::dbg::Frame &frame) {
+ frame.location.line = (int)entry.line;
+ });
}
}
diff --git a/src/Vulkan/Debug/Thread.cpp b/src/Vulkan/Debug/Thread.cpp
index 2f4dcc0..dd9e22d 100644
--- a/src/Vulkan/Debug/Thread.cpp
+++ b/src/Vulkan/Debug/Thread.cpp
@@ -113,10 +113,14 @@
}
}
-void Thread::exit()
+void Thread::exit(bool isStep /* = false */)
{
marl::lock lock(mutex);
frames.pop_back();
+ if(isStep)
+ {
+ onLocationUpdate(lock);
+ }
}
void Thread::update(bool isStep, const UpdateFrame &f)
diff --git a/src/Vulkan/Debug/Thread.hpp b/src/Vulkan/Debug/Thread.hpp
index d9ba32d..1730146 100644
--- a/src/Vulkan/Debug/Thread.hpp
+++ b/src/Vulkan/Debug/Thread.hpp
@@ -130,7 +130,10 @@
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();
+ // If isStep is true, then this will be considered a line-steppable event,
+ // and the debugger may pause at the function call at the new top most stack
+ // frame.
+ void exit(bool isStep = false);
// frame() returns a copy of the thread's top most stack frame.
Frame frame() const;
