From 1908691bd69036cdb829aeffbcbaf74f0b3ff868 Mon Sep 17 00:00:00 2001
From: aarzilli <alessandro.arzilli@gmail.com>
Date: Thu, 4 Jan 2024 12:49:12 +0100
Subject: [PATCH] proc: fix TestCondBreakpointWithFrame flakes on 1.22rc1

The flake manifests as an error where the variable i can not be found in
frame 1 and happens in go1.22rc1 between 0.1% and 0.5% of the time (it is highly dependent on CPU contention)
This problem is caused by the new code in evalop.PushLocal referencing the
stale value of SelectedGoroutine. This happens because:

-  evalop.PushLocal calls ConvertEvalScope
- ConvertEvalScope calls FindGoroutine
- FindGoroutine checks the value of selectedGoroutine

When breakpoint conditions are evaluated both the value of selectedGoroutine
and currentThread are stale because we can only set their definitive value
*after* all breakpoint conditions have been evaluated.

The fact that it only happens in 1.22rc1 is coincidental, it's probably
caused by the fact that 1.22rc1 migrates goroutines between threads more in
this particular circumstance.

This commit fixes the problem in two ways:

1. selectedGoroutine and currentThread are given temprorary non-stale values
   before breakpoint conditions are evaluated
2. evalop.PushLocal is changed so it takes a stack trace of the current
   thread rather than resolving it through the selected goroutine.

Either one would suffice however I think we should do both, (2) ensures that
the runtime.frame(n).var will work even if the current thread is not running
any goroutine and (1) ensures that we don't accidentally reference a stale
selectedGoroutine in the future.
---
 pkg/proc/eval.go        | 7 +++----
 pkg/proc/target_exec.go | 7 +++++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/pkg/proc/eval.go b/pkg/proc/eval.go
index 8baae9ea1c..f3e83e9ede 100644
--- a/pkg/proc/eval.go
+++ b/pkg/proc/eval.go
@@ -966,10 +966,9 @@ func (stack *evalStack) executeOp() {
 		var vars []*Variable
 		var err error
 		if op.Frame != 0 {
-			var frameScope *EvalScope
-			frameScope, err = ConvertEvalScope(scope.target, scope.g.ID, int(op.Frame), 0)
-			if err != nil {
-				stack.err = err
+			frameScope, err2 := ConvertEvalScope(scope.target, -1, int(op.Frame), 0)
+			if err2 != nil {
+				stack.err = err2
 				return
 			}
 			vars, err = frameScope.Locals(0)
diff --git a/pkg/proc/target_exec.go b/pkg/proc/target_exec.go
index 2e3f4748d4..29fc6e50c6 100644
--- a/pkg/proc/target_exec.go
+++ b/pkg/proc/target_exec.go
@@ -97,11 +97,18 @@ func (grp *TargetGroup) Continue() error {
 
 		it := ValidTargets{Group: grp}
 		for it.Next() {
+			// Both selectedGoroutine and current thread are stale here, since we can
+			// only set their definitive value *after* evaluating breakpoint
+			// conditions here we give them temporary non-stale values.
+			it.selectedGoroutine = nil
+			curthread := it.currentThread
 			for _, thread := range it.ThreadList() {
 				if thread.Breakpoint().Breakpoint != nil {
+					it.currentThread = thread
 					thread.Breakpoint().Breakpoint.checkCondition(it.Target, thread, thread.Breakpoint())
 				}
 			}
+			it.currentThread = curthread
 		}
 
 		if contOnceErr != nil {