From b370e20cd5e8a8b1fc110b41f30a21315c8f78cc Mon Sep 17 00:00:00 2001 From: aarzilli Date: Sun, 31 Jan 2016 13:03:53 +0100 Subject: [PATCH] proc: bugs setting next breakpoints 1. A running goroutine is by definition not parked waiting for a chan recv 2. The FDE end address is intended to be exclusive, the code interpreted as inclusive and sometimes ended up setting a breakpoint on a function other than the current one. --- _fixtures/issue332.go | 15 ++++++++++ proc/proc_test.go | 64 +++++++++++++++++++++++++++++++++++++++++++ proc/threads.go | 2 +- proc/variables.go | 2 +- terminal/command.go | 12 ++++---- 5 files changed, 88 insertions(+), 7 deletions(-) create mode 100644 _fixtures/issue332.go diff --git a/_fixtures/issue332.go b/_fixtures/issue332.go new file mode 100644 index 00000000..fed22850 --- /dev/null +++ b/_fixtures/issue332.go @@ -0,0 +1,15 @@ +package main + +import "fmt" + +func main() { + m := make([]string, 1, 25) + fmt.Println(m) // [ ] + changeMe(m) + fmt.Println(m) // [Todd] +} + +func changeMe(z []string) { + z[0] = "Todd" + fmt.Println(z) // [Todd] +} diff --git a/proc/proc_test.go b/proc/proc_test.go index 31c59b2d..b3883580 100644 --- a/proc/proc_test.go +++ b/proc/proc_test.go @@ -19,6 +19,8 @@ import ( protest "github.com/derekparker/delve/proc/test" ) +const disableFailingTests = true + func init() { runtime.GOMAXPROCS(4) os.Setenv("GOMAXPROCS", "4") @@ -1475,3 +1477,65 @@ func TestIssue384(t *testing.T) { assertNoError(err, t, "EvalVariable()") }) } + +func TestIssue332_Part1(t *testing.T) { + // Next shouldn't step inside a function call + withTestProcess("issue332", t, func(p *Process, fixture protest.Fixture) { + start, _, err := p.goSymTable.LineToPC(fixture.Source, 8) + assertNoError(err, t, "LineToPC()") + _, err = p.SetBreakpoint(start) + assertNoError(err, t, "SetBreakpoint()") + assertNoError(p.Continue(), t, "Continue()") + assertNoError(p.Next(), t, "first Next()") + locations, err := p.CurrentThread.Stacktrace(2) + assertNoError(err, t, "Stacktrace()") + if locations[0].Call.Fn == nil { + t.Fatalf("Not on a function") + } + if locations[0].Call.Fn.Name != "main.main" { + t.Fatalf("Not on main.main after Next: %s (%s:%d)", locations[0].Call.Fn.Name, locations[0].Call.File, locations[0].Call.Line) + } + if locations[0].Call.Line != 9 { + t.Fatalf("Not on line 9 after Next: %s (%s:%d)", locations[0].Call.Fn.Name, locations[0].Call.File, locations[0].Call.Line) + } + }) +} + +func TestIssue332_Part2(t *testing.T) { + // Step should skip a function's prologue + // In some parts of the prologue, for some functions, the FDE data is incorrect + // which leads to 'next' and 'stack' failing with error "could not find FDE for PC: " + // because the incorrect FDE data leads to reading the wrong stack address as the return address + // TODO: the incorrect FDE data problem is fixed in go 1.6, reenable this test when 1.6 is released. + if disableFailingTests { + return + } + withTestProcess("issue332", t, func(p *Process, fixture protest.Fixture) { + start, _, err := p.goSymTable.LineToPC(fixture.Source, 8) + assertNoError(err, t, "LineToPC()") + _, err = p.SetBreakpoint(start) + assertNoError(err, t, "SetBreakpoint()") + assertNoError(p.Continue(), t, "Continue()") + + // step until we enter changeMe + for { + assertNoError(p.Step(), t, "Step()") + locations, err := p.CurrentThread.Stacktrace(2) + assertNoError(err, t, "Stacktrace()") + if locations[0].Call.Fn == nil { + t.Fatalf("Not on a function") + } + if locations[0].Call.Fn.Name == "main.changeMe" { + break + } + } + + assertNoError(p.Next(), t, "first Next()") + assertNoError(p.Next(), t, "second Next()") + assertNoError(p.Next(), t, "third Next()") + err = p.Continue() + if _, exited := err.(ProcessExitedError); !exited { + assertNoError(err, t, "final Continue()") + } + }) +} diff --git a/proc/threads.go b/proc/threads.go index 797633f4..ff46d146 100644 --- a/proc/threads.go +++ b/proc/threads.go @@ -164,7 +164,7 @@ func (ge GoroutineExitingError) Error() string { // Set breakpoints at every line, and the return address. Also look for // a deferred function and set a breakpoint there too. func (thread *Thread) next(curpc uint64, fde *frame.FrameDescriptionEntry, file string, line int) error { - pcs := thread.dbp.lineInfo.AllPCsBetween(fde.Begin(), fde.End(), file) + pcs := thread.dbp.lineInfo.AllPCsBetween(fde.Begin(), fde.End()-1, file) g, err := thread.GetG() if err != nil { diff --git a/proc/variables.go b/proc/variables.go index 0d1c3ed3..1e0a0669 100644 --- a/proc/variables.go +++ b/proc/variables.go @@ -311,7 +311,7 @@ func (scope *EvalScope) PtrSize() int { // ChanRecvBlocked returns whether the goroutine is blocked on // a channel read operation. func (g *G) ChanRecvBlocked() bool { - return g.WaitReason == chanRecv + return (g.thread == nil) && (g.WaitReason == chanRecv) } // chanRecvReturnAddr returns the address of the return from a channel read. diff --git a/terminal/command.go b/terminal/command.go index 3d301240..539f07d8 100644 --- a/terminal/command.go +++ b/terminal/command.go @@ -882,7 +882,7 @@ func printcontextThread(t *Term, th *api.Thread) { fn := th.Function if th.Breakpoint == nil { - fmt.Printf("> %s() %s:%d\n", fn.Name, ShortenFilePath(th.File), th.Line) + fmt.Printf("> %s() %s:%d (PC: %#v)\n", fn.Name, ShortenFilePath(th.File), th.Line, th.PC) return } @@ -896,21 +896,23 @@ func printcontextThread(t *Term, th *api.Thread) { } if hitCount, ok := th.Breakpoint.HitCount[strconv.Itoa(th.GoroutineID)]; ok { - fmt.Printf("> %s(%s) %s:%d (hits goroutine(%d):%d total:%d)\n", + fmt.Printf("> %s(%s) %s:%d (hits goroutine(%d):%d total:%d) (PC: %#v)\n", fn.Name, args, ShortenFilePath(th.File), th.Line, th.GoroutineID, hitCount, - th.Breakpoint.TotalHitCount) + th.Breakpoint.TotalHitCount, + th.PC) } else { - fmt.Printf("> %s(%s) %s:%d (hits total:%d)\n", + fmt.Printf("> %s(%s) %s:%d (hits total:%d) (PC: %#v)\n", fn.Name, args, ShortenFilePath(th.File), th.Line, - th.Breakpoint.TotalHitCount) + th.Breakpoint.TotalHitCount, + th.PC) } if th.BreakpointInfo != nil {