proc: fix bug with stack watchpoints going out of scope (#3742)

When stack watchpoints go out of scope simultaneously they can hide (or
duplicate the effect) of other breakpoints (including other watchpoints
going out of scope) that are placed on the same physical memory
location.

This happens because we delete the watchpoint-out-of-scope breakpoint
while we are evaluating hit breakpoints, mangling the breaklets list.

This commit moves breakpoint deletion out of the
watchpoint-out-of-scope condition, delaying it until all hit
breakpoints have been evaluated.

Also fix bug where on amd64 if all four watchpoints are in use the last
one is not checked.

Fixes #3739

Documentation/backend_test_health.md

@@ -1,9 +1,9 @@
 Tests skipped by each supported backend:
 
-* 386 skipped = 7
+* 386 skipped = 8
 	* 1 broken
 	* 3 broken - cgo stacktraces
-	* 3 not implemented
+	* 4 not implemented
 * arm64 skipped = 1
 	* 1 broken - global variable symbolication
 * darwin skipped = 3
@@ -13,10 +13,10 @@ Tests skipped by each supported backend:
 	* 1 broken - cgo stacktraces
 * darwin/lldb skipped = 1
 	* 1 upstream issue
-* freebsd skipped = 10
+* freebsd skipped = 11
 	* 2 flaky
 	* 2 follow exec not implemented on freebsd
-	* 4 not implemented
+	* 5 not implemented
 	* 2 not working on freebsd
 * linux/386 skipped = 2
 	* 2 not working on linux/386
@@ -31,14 +31,14 @@ Tests skipped by each supported backend:
 	* 3 broken - pie mode
 * pie skipped = 2
 	* 2 upstream issue - https://github.com/golang/go/issues/29322
-* ppc64le skipped = 11
+* ppc64le skipped = 12
 	* 6 broken
 	* 1 broken - global variable symbolication
-	* 4 not implemented
-* windows skipped = 6
+	* 5 not implemented
+* windows skipped = 7
 	* 1 broken
 	* 2 not working on windows
-	* 3 see https://github.com/go-delve/delve/issues/2768
+	* 4 see https://github.com/go-delve/delve/issues/2768
 * windows/arm64 skipped = 5
 	* 3 broken
 	* 1 broken - cgo stacktraces

_fixtures/stackwatchbug.go

@@ -0,0 +1,20 @@
+package main
+
+import (
+	"fmt"
+)
+
+func multiRound() {
+	vars := []int{0, 1, 2, 3, 4, 5}
+	for i := range vars { // line 9: set watchpoints
+		if i > 0 {
+			vars[i] = vars[i-1]
+			fmt.Println() // line 12: watchpoints hit
+		}
+	}
+}
+
+func main() {
+	multiRound() // line 18: after restart, last watchpoint out of scope
+	return       // line 19: all watchpoints should go out of scope
+}

pkg/proc/amd64util/debugregs.go

@@ -115,7 +115,7 @@ func (drs *DebugRegisters) ClearBreakpoint(idx uint8) {
 // GetActiveBreakpoint returns the active hardware breakpoint and resets the
 // condition flags.
 func (drs *DebugRegisters) GetActiveBreakpoint() (ok bool, idx uint8) {
-	for idx := uint8(0); idx < 3; idx++ {
+	for idx := uint8(0); idx <= 3; idx++ {
 		enable := *(drs.pDR7) & (1 << enableBitOffset(idx))
 		if enable == 0 {
 			continue

pkg/proc/proc_test.go

@@ -6680,3 +6680,51 @@ func TestRangeOverFuncStepOut(t *testing.T) {
 		{contStepout, 237},
 	})
 }
+
+func TestStackwatchClearBug(t *testing.T) {
+	skipOn(t, "not implemented", "freebsd")
+	skipOn(t, "not implemented", "386")
+	skipOn(t, "not implemented", "ppc64le")
+	skipOn(t, "see https://github.com/go-delve/delve/issues/2768", "windows")
+
+	showbps := func(bps *proc.BreakpointMap) {
+		for _, bp := range bps.M {
+			t.Logf("\t%s\n", bp)
+		}
+	}
+
+	withTestProcess("stackwatchbug", t, func(p *proc.Target, grp *proc.TargetGroup, fixture protest.Fixture) {
+		setFileBreakpoint(p, t, fixture.Source, 9)
+		bpsBefore := p.Breakpoints()
+
+		assertNoError(grp.Continue(), t, "Continue 0")
+
+		scope, err := proc.GoroutineScope(p, p.CurrentThread())
+		assertNoError(err, t, "GoroutineScope")
+
+		for _, s := range []string{"vars[0]", "vars[3]", "vars[2]", "vars[1]"} {
+			_, err := p.SetWatchpoint(0, scope, s, proc.WatchWrite, nil)
+			assertNoError(err, t, "SetWatchpoint(write-only)")
+		}
+
+		t.Logf("After setting watchpoints:")
+		showbps(p.Breakpoints())
+
+		assertNoError(grp.Continue(), t, "Continue 1")
+		assertNoError(grp.Continue(), t, "Continue 2")
+		assertNoError(grp.Continue(), t, "Continue 3")
+		assertNoError(grp.Continue(), t, "Continue 4")
+		f, l := currentLineNumber(p, t)
+		t.Logf("at %s:%d", f, l)
+		if l != 19 {
+			t.Error("Wrong position after fourth continue")
+		}
+
+		bpsAfter := p.Breakpoints()
+		t.Logf("After watchpoint goes out of scope:")
+		showbps(bpsAfter)
+		if len(bpsBefore.M) != len(bpsAfter.M) {
+			t.Errorf("wrong number of breakpoints")
+		}
+	})
+}

pkg/proc/stackwatch.go

@@ -29,7 +29,7 @@ func (t *Target) setStackWatchBreakpoints(scope *EvalScope, watchpoint *Breakpoi
 	// Watchpoint Out-of-scope Sentinel
 
 	woos := func(_ Thread, _ *Target) (bool, error) {
-		watchpointOutOfScope(t, watchpoint)
+		t.Breakpoints().WatchOutOfScope = append(t.Breakpoints().WatchOutOfScope, watchpoint)
 		return true, nil
 	}
 
@@ -141,20 +141,6 @@ func (t *Target) clearStackWatchBreakpoints(watchpoint *Breakpoint) error {
 	return nil
 }
 
-// watchpointOutOfScope is called when the watchpoint goes out of scope. It
-// is used as a breaklet callback function.
-// Its responsibility is to delete the watchpoint and make sure that the
-// user is notified of the watchpoint going out of scope.
-func watchpointOutOfScope(t *Target, watchpoint *Breakpoint) {
-	t.Breakpoints().WatchOutOfScope = append(t.Breakpoints().WatchOutOfScope, watchpoint)
-	err := t.ClearBreakpoint(watchpoint.Addr)
-	if err != nil {
-		log := logflags.DebuggerLogger()
-		log.Errorf("could not clear out-of-scope watchpoint: %v", err)
-	}
-	delete(t.Breakpoints().Logical, watchpoint.LogicalID())
-}
-
 // adjustStackWatchpoint is called when the goroutine of watchpoint resizes
 // its stack. It is used as a breaklet callback function.
 // Its responsibility is to move the watchpoint to a its new address.

pkg/proc/target_exec.go

@@ -110,6 +110,14 @@ func (grp *TargetGroup) Continue() error {
 				}
 			}
 			it.currentThread = curthread
+			// Clear watchpoints that have gone out of scope
+			for _, watchpoint := range it.Breakpoints().WatchOutOfScope {
+				err := it.ClearBreakpoint(watchpoint.Addr)
+				if err != nil {
+					logflags.DebuggerLogger().Errorf("could not clear out-of-scope watchpoint: %v", err)
+				}
+				delete(it.Breakpoints().Logical, watchpoint.LogicalID())
+			}
 		}
 
 		if contOnceErr != nil {