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>

src/Pipeline/SpirvShaderDebugger.cpp

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;
+			});
 		}
 	}
 

src/Vulkan/Debug/Thread.cpp

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)

src/Vulkan/Debug/Thread.hpp

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;