Hide function return values after a step.
Previously, return values like `[func].result` would stick around in the
Variables table indefinitely. This felt very counterintuitive. Now they
only appear for one step, then vanish.
Change-Id: Iedfc7d2ddf136111005b26aaefb380ffc6281d05
Bug: skia:12666
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/483605
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/tracing/SkVMDebugTracePlayer.cpp b/src/sksl/tracing/SkVMDebugTracePlayer.cpp
index 5206cba..85bc892 100644
--- a/src/sksl/tracing/SkVMDebugTracePlayer.cpp
+++ b/src/sksl/tracing/SkVMDebugTracePlayer.cpp
@@ -20,10 +20,17 @@
/*fLine=*/-1,
/*fDisplayMask=*/SkBitSet(nslots)});
fDirtyMask.emplace(nslots);
+ fReturnValues.emplace(nslots);
+
+ for (size_t slotIdx = 0; slotIdx < nslots; ++slotIdx) {
+ if (fDebugTrace->fSlotInfo[slotIdx].fnReturnValue >= 0) {
+ fReturnValues->set(slotIdx);
+ }
+ }
}
void SkVMDebugTracePlayer::step() {
- fDirtyMask->reset();
+ this->tidy();
while (!this->traceHasCompleted()) {
if (this->execute(fCursor++)) {
break;
@@ -32,7 +39,7 @@
}
void SkVMDebugTracePlayer::stepOver() {
- fDirtyMask->reset();
+ this->tidy();
size_t initialStackDepth = fStack.size();
while (!this->traceHasCompleted()) {
bool canEscapeFromThisStackDepth = (fStack.size() <= initialStackDepth);
@@ -43,7 +50,7 @@
}
void SkVMDebugTracePlayer::stepOut() {
- fDirtyMask->reset();
+ this->tidy();
size_t initialStackDepth = fStack.size();
while (!this->traceHasCompleted()) {
if (this->execute(fCursor++) && (fStack.size() < initialStackDepth)) {
@@ -52,6 +59,16 @@
}
}
+void SkVMDebugTracePlayer::tidy() {
+ fDirtyMask->reset();
+
+ // Conceptually this is `fStack.back().fDisplayMask &= ~fReturnValues`, but SkBitSet doesn't
+ // support masking one set of bits against another.
+ fReturnValues->forEachSetIndex([&](int slot) {
+ fStack.back().fDisplayMask.reset(slot);
+ });
+}
+
bool SkVMDebugTracePlayer::traceHasCompleted() const {
return !fDebugTrace || fCursor >= fDebugTrace->fTraceInfo.size();
}
diff --git a/src/sksl/tracing/SkVMDebugTracePlayer.h b/src/sksl/tracing/SkVMDebugTracePlayer.h
index 34e803e..605ad81 100644
--- a/src/sksl/tracing/SkVMDebugTracePlayer.h
+++ b/src/sksl/tracing/SkVMDebugTracePlayer.h
@@ -59,6 +59,11 @@
*/
bool execute(size_t position);
+ /**
+ * Cleans up temporary state between steps, such as the dirty mask and function return values.
+ */
+ void tidy();
+
/** Returns a vector of the indices and values of each slot that is enabled in `bits`. */
std::vector<VariableData> getVariablesForDisplayMask(const SkBitSet& bits) const;
@@ -73,6 +78,7 @@
std::vector<StackFrame> fStack; // the execution stack
skstd::optional<SkBitSet> fDirtyMask; // variable slots touched during the most-recently
// executed step
+ skstd::optional<SkBitSet> fReturnValues; // variable slots containing return values
};
} // namespace SkSL
diff --git a/tests/SkVMDebugTracePlayerTest.cpp b/tests/SkVMDebugTracePlayerTest.cpp
index efe77e0..0e753d0 100644
--- a/tests/SkVMDebugTracePlayerTest.cpp
+++ b/tests/SkVMDebugTracePlayerTest.cpp
@@ -243,9 +243,9 @@
DEF_TEST(SkSLTracePlayerVariables, r) {
sk_sp<SkSL::SkVMDebugTrace> trace = make_trace(r,
R"( // Line 1
-void func() { // Line 2
- int z = 456; // Line 3
- return; // Line 4
+float func() { // Line 2
+ float z = 456; // Line 3
+ return z; // Line 4
} // Line 5
int main() { // Line 6
int a = 123; // Line 7
@@ -277,18 +277,19 @@
player.step();
REPORTER_ASSERT(r, player.getCurrentLine() == 3);
- REPORTER_ASSERT(r, make_stack_string(*trace, player) == "int main() -> void func()");
+ REPORTER_ASSERT(r, make_stack_string(*trace, player) == "int main() -> float func()");
REPORTER_ASSERT(r, make_local_vars_string(*trace, player) == "");
player.step();
REPORTER_ASSERT(r, player.getCurrentLine() == 4);
- REPORTER_ASSERT(r, make_stack_string(*trace, player) == "int main() -> void func()");
+ REPORTER_ASSERT(r, make_stack_string(*trace, player) == "int main() -> float func()");
REPORTER_ASSERT(r, make_local_vars_string(*trace, player) == "##z = 456");
player.step();
REPORTER_ASSERT(r, player.getCurrentLine() == 9);
REPORTER_ASSERT(r, make_stack_string(*trace, player) == "int main()");
- REPORTER_ASSERT(r, make_local_vars_string(*trace, player) == "a = 123, b = true");
+ REPORTER_ASSERT(r, make_local_vars_string(*trace, player) ==
+ "a = 123, b = true, ##[func].result = 456");
player.step();
REPORTER_ASSERT(r, player.getCurrentLine() == 10);
diff --git a/tools/viewer/SkSLDebuggerSlide.cpp b/tools/viewer/SkSLDebuggerSlide.cpp
index 119f1a6..a7acc9e 100644
--- a/tools/viewer/SkSLDebuggerSlide.cpp
+++ b/tools/viewer/SkSLDebuggerSlide.cpp
@@ -146,6 +146,8 @@
std::vector<SkSL::SkVMDebugTracePlayer::VariableData> vars;
if (frame >= 0) {
vars = fPlayer.getLocalVariables(frame);
+ } else {
+ vars = fPlayer.getGlobalVariables();
}
if (vars.empty()) {
return;