From 06053a7e4b34e16aac9c124e4111ee3b2ba9dd51 Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Wed, 12 Jun 2024 21:37:04 +0200 Subject: [PATCH] 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 | 16 +++++----- _fixtures/stackwatchbug.go | 20 ++++++++++++ pkg/proc/amd64util/debugregs.go | 2 +- pkg/proc/proc_test.go | 48 ++++++++++++++++++++++++++++ pkg/proc/stackwatch.go | 16 +--------- pkg/proc/target_exec.go | 8 +++++ 6 files changed, 86 insertions(+), 24 deletions(-) create mode 100644 _fixtures/stackwatchbug.go diff --git a/Documentation/backend_test_health.md b/Documentation/backend_test_health.md index 4aa80a9564..ec4e9f01de 100644 --- a/Documentation/backend_test_health.md +++ b/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 diff --git a/_fixtures/stackwatchbug.go b/_fixtures/stackwatchbug.go new file mode 100644 index 0000000000..efb18814a2 --- /dev/null +++ b/_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 +} diff --git a/pkg/proc/amd64util/debugregs.go b/pkg/proc/amd64util/debugregs.go index 23de122bbb..290ee16321 100644 --- a/pkg/proc/amd64util/debugregs.go +++ b/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 diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go index 2e317208ae..ea385b38b0 100644 --- a/pkg/proc/proc_test.go +++ b/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") + } + }) +} diff --git a/pkg/proc/stackwatch.go b/pkg/proc/stackwatch.go index 99ee6db760..ff9b94901e 100644 --- a/pkg/proc/stackwatch.go +++ b/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. diff --git a/pkg/proc/target_exec.go b/pkg/proc/target_exec.go index da798738ae..d341173010 100644 --- a/pkg/proc/target_exec.go +++ b/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 {