Skip to content

Commit

Permalink
Miscellaneous fixes for Windows native backend (#2736)
Browse files Browse the repository at this point in the history 
* proc/native: always stop after RequestManualStop on Windows

On Windows RequestManualStop will generate an exception on a special
DbgUiRemoteBreakin thread, sometimes this thread will die before we
finish stopping the process. We need to account for that and still stop
even if the thread is gone and no other thread hit a breakpoint.

Fixes flakiness of TestIssue419.

* proc/native: fix watchpoints with new threads on Windows

When a new thread is created we must reapply all watchpoints to it,
like we do on linux.

* tests: be lenient on goroutinestackprog tests on Windows

We can not guarantee that we find all goroutines stopped in a good
place and sometimes the stacktrace fails on Windows.
  • Loading branch information
@aarzilli
aarzilli committed Oct 13, 2021
1 parent d1c888f commit 1893c97
Show file tree
Hide file tree
Showing 6 changed files with 99 additions and 17 deletions.
14 changes: 14 additions & 0 deletions _fixtures/databpeasy.go
View file
Expand Up @@ -3,6 +3,8 @@ package main
import (
"fmt"
"runtime"
"sync"
"time"
)

var globalvar1 = 0
Expand All @@ -23,6 +25,12 @@ func main() { // Position 0
fmt.Printf("%d %d\n", globalvar1, globalvar2)

done := make(chan struct{}) // Position 4
var wg sync.WaitGroup
for i := 0; i < 20; i++ {
wg.Add(1)
go waitfunc(i, &wg)
}
wg.Wait()
go f(done)
<-done
}
Expand All @@ -32,3 +40,9 @@ func f(done chan struct{}) {
globalvar1 = globalvar2 + 2
close(done) // Position 5
}

func waitfunc(i int, wg *sync.WaitGroup) {
runtime.LockOSThread()
wg.Done()
time.Sleep(50 * time.Second)
}
18 changes: 18 additions & 0 deletions pkg/proc/native/proc_windows.go
View file
Expand Up @@ -222,6 +222,16 @@ func (dbp *nativeProcess) addThread(hThread syscall.Handle, threadID int, attach
return nil, err
}
}

for _, bp := range dbp.Breakpoints().M {
if bp.WatchType != 0 {
err := thread.writeHardwareBreakpoint(bp.Addr, bp.WatchType, bp.HWBreakIndex)
if err != nil {
return nil, err
}
}
}

return thread, nil
}

Expand Down Expand Up @@ -489,6 +499,7 @@ func (dbp *nativeProcess) stop(trapthread *nativeThread) (*nativeThread, error)
}

if !trapthreadFound {
wasDbgUiRemoteBreakIn := trapthread.os.dbgUiRemoteBreakIn
// trapthread exited during stop, pick another one
trapthread = nil
for _, thread := range dbp.threads {
Expand All @@ -497,6 +508,13 @@ func (dbp *nativeProcess) stop(trapthread *nativeThread) (*nativeThread, error)
break
}
}
if trapthread == nil && wasDbgUiRemoteBreakIn {
// If this was triggered by a manual stop request we should stop
// regardless, pick a thread.
for _, thread := range dbp.threads {
return thread, nil
}
}
}

return trapthread, nil
Expand Down
37 changes: 25 additions & 12 deletions pkg/proc/proc_test.go
View file
Expand Up @@ -957,6 +957,11 @@ func TestStacktraceGoroutine(t *testing.T) {
{{10, "main.agoroutine"}},
}

lenient := 0
if runtime.GOOS == "windows" {
lenient = 1
}

protest.AllowRecording(t)
withTestProcess("goroutinestackprog", t, func(p *proc.Target, fixture protest.Fixture) {
bp := setFunctionBreakpoint(p, t, "main.stacktraceme")
Expand Down Expand Up @@ -1006,7 +1011,7 @@ func TestStacktraceGoroutine(t *testing.T) {
t.Fatalf("Main goroutine stack not found %d", mainCount)
}

if agoroutineCount < 10 {
if agoroutineCount < 10-lenient {
t.Fatalf("Goroutine stacks not found (%d)", agoroutineCount)
}

Expand Down Expand Up @@ -1266,6 +1271,10 @@ func TestVariableEvaluation(t *testing.T) {

func TestFrameEvaluation(t *testing.T) {
protest.AllowRecording(t)
lenient := false
if runtime.GOOS == "windows" {
lenient = true
}
withTestProcess("goroutinestackprog", t, func(p *proc.Target, fixture protest.Fixture) {
setFunctionBreakpoint(p, t, "main.stacktraceme")
assertNoError(p.Continue(), t, "Continue()")
Expand Down Expand Up @@ -1312,7 +1321,11 @@ func TestFrameEvaluation(t *testing.T) {

for i := range found {
if !found[i] {
t.Fatalf("Goroutine %d not found\n", i)
if lenient {
lenient = false
} else {
t.Fatalf("Goroutine %d not found\n", i)
}
}
}

Expand Down Expand Up @@ -5368,20 +5381,20 @@ func TestWatchpointsBasic(t *testing.T) {
skipOn(t, "not implemented", "linux", "arm64")
protest.AllowRecording(t)

position1 := 17
position5 := 33
position1 := 19
position5 := 41

if runtime.GOARCH == "arm64" {
position1 = 16
position5 = 32
position1 = 18
position5 = 40
}

withTestProcess("databpeasy", t, func(p *proc.Target, fixture protest.Fixture) {
setFunctionBreakpoint(p, t, "main.main")
setFileBreakpoint(p, t, fixture.Source, 19) // Position 2 breakpoint
setFileBreakpoint(p, t, fixture.Source, 25) // Position 4 breakpoint
setFileBreakpoint(p, t, fixture.Source, 21) // Position 2 breakpoint
setFileBreakpoint(p, t, fixture.Source, 27) // Position 4 breakpoint
assertNoError(p.Continue(), t, "Continue 0")
assertLineNumber(p, t, 11, "Continue 0") // Position 0
assertLineNumber(p, t, 13, "Continue 0") // Position 0

scope, err := proc.GoroutineScope(p, p.CurrentThread())
assertNoError(err, t, "GoroutineScope")
Expand All @@ -5399,18 +5412,18 @@ func TestWatchpointsBasic(t *testing.T) {
p.ClearBreakpoint(bp.Addr)

assertNoError(p.Continue(), t, "Continue 2")
assertLineNumber(p, t, 19, "Continue 2") // Position 2
assertLineNumber(p, t, 21, "Continue 2") // Position 2

_, err = p.SetWatchpoint(scope, "globalvar1", proc.WatchWrite|proc.WatchRead, nil)
assertNoError(err, t, "SetDataBreakpoint(read-write)")

assertNoError(p.Continue(), t, "Continue 3")
assertLineNumber(p, t, 20, "Continue 3") // Position 3
assertLineNumber(p, t, 22, "Continue 3") // Position 3

p.ClearBreakpoint(bp.Addr)

assertNoError(p.Continue(), t, "Continue 4")
assertLineNumber(p, t, 25, "Continue 4") // Position 4
assertLineNumber(p, t, 27, "Continue 4") // Position 4

t.Logf("setting final breakpoint")
_, err = p.SetWatchpoint(scope, "globalvar1", proc.WatchWrite, nil)
Expand Down
23 changes: 20 additions & 3 deletions pkg/terminal/command_test.go
View file
Expand Up @@ -331,6 +331,11 @@ func TestScopePrefix(t *testing.T) {
const goroutinesCurLinePrefix = "* Goroutine "
test.AllowRecording(t)

lenient := 0
if runtime.GOOS == "windows" {
lenient = 1
}

withTestTerminal("goroutinestackprog", t, func(term *FakeTerminal) {
term.MustExec("b stacktraceme")
term.MustExec("continue")
Expand Down Expand Up @@ -383,7 +388,7 @@ func TestScopePrefix(t *testing.T) {
}
}
}
if len(agoroutines)+extraAgoroutines < 10 {
if len(agoroutines)+extraAgoroutines < 10-lenient {
t.Fatalf("Output of goroutines did not have 10 goroutines stopped on main.agoroutine (%d+%d found): %q", len(agoroutines), extraAgoroutines, goroutinesOut)
}
}
Expand Down Expand Up @@ -429,7 +434,11 @@ func TestScopePrefix(t *testing.T) {

for i := range seen {
if !seen[i] {
t.Fatalf("goroutine %d not found", i)
if lenient > 0 {
lenient--
} else {
t.Fatalf("goroutine %d not found", i)
}
}
}

Expand Down Expand Up @@ -474,6 +483,10 @@ func TestOnPrefix(t *testing.T) {
}
const prefix = "\ti: "
test.AllowRecording(t)
lenient := false
if runtime.GOOS == "windows" {
lenient = true
}
withTestTerminal("goroutinestackprog", t, func(term *FakeTerminal) {
term.MustExec("b agobp main.agoroutine")
term.MustExec("on agobp print i")
Expand Down Expand Up @@ -507,7 +520,11 @@ func TestOnPrefix(t *testing.T) {

for i := range seen {
if !seen[i] {
t.Fatalf("Goroutine %d not seen\n", i)
if lenient {
lenient = false
} else {
t.Fatalf("Goroutine %d not seen\n", i)
}
}
}
})
Expand Down
12 changes: 11 additions & 1 deletion service/test/integration1_test.go
View file
Expand Up @@ -719,6 +719,12 @@ func Test1ClientServer_FullStacktrace(t *testing.T) {
if runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" {
t.Skip("cgo doesn't work on darwin/arm64")
}

lenient := false
if runtime.GOOS == "windows" {
lenient = true
}

withTestClient1("goroutinestackprog", t, func(c *rpc1.RPCClient) {
_, err := c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.stacktraceme", Line: -1})
assertNoError(err, t, "CreateBreakpoint()")
Expand Down Expand Up @@ -757,7 +763,11 @@ func Test1ClientServer_FullStacktrace(t *testing.T) {

for i := range found {
if !found[i] {
t.Fatalf("Goroutine %d not found", i)
if lenient {
lenient = false
} else {
t.Fatalf("Goroutine %d not found", i)
}
}
}

Expand Down
12 changes: 11 additions & 1 deletion service/test/integration2_test.go
View file
Expand Up @@ -1022,6 +1022,12 @@ func TestClientServer_FullStacktrace(t *testing.T) {
if runtime.GOOS == "darwin" && runtime.GOARCH == "arm64" {
t.Skip("cgo doesn't work on darwin/arm64")
}

lenient := false
if runtime.GOOS == "windows" {
lenient = true
}

withTestClient2("goroutinestackprog", t, func(c service.Client) {
_, err := c.CreateBreakpoint(&api.Breakpoint{FunctionName: "main.stacktraceme", Line: -1})
assertNoError(err, t, "CreateBreakpoint()")
Expand Down Expand Up @@ -1060,7 +1066,11 @@ func TestClientServer_FullStacktrace(t *testing.T) {

for i := range found {
if !found[i] {
t.Fatalf("Goroutine %d not found", i)
if lenient {
lenient = false
} else {
t.Fatalf("Goroutine %d not found", i)
}
}
}

Expand Down

0 comments on commit 1893c97

Please sign in to comment.