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>

src/sksl/tracing/SkVMDebugTracePlayer.cpp

@@ -20,10 +20,17 @@ void SkVMDebugTracePlayer::reset(sk_sp<SkVMDebugTrace> debugTrace) {
                       /*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::step() {
 }
 
 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::stepOver() {
 }
 
 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::stepOut() {
     }
 }
 
+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();
 }

src/sksl/tracing/SkVMDebugTracePlayer.h

@@ -59,6 +59,11 @@ private:
      */
     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 @@ private:
     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

tests/SkVMDebugTracePlayerTest.cpp

@@ -243,9 +243,9 @@ int main() {                    // Line 8
 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 @@ int main() {                          // Line 6
     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);

tools/viewer/SkSLDebuggerSlide.cpp

@@ -146,6 +146,8 @@ void SkSLDebuggerSlide::showVariableTable() {
     std::vector<SkSL::SkVMDebugTracePlayer::VariableData> vars;
     if (frame >= 0) {
         vars = fPlayer.getLocalVariables(frame);
+    } else {
+        vars = fPlayer.getGlobalVariables();
     }
     if (vars.empty()) {
         return;