From d17c7c3a614d785181ffb830f90efe40a964f810 Mon Sep 17 00:00:00 2001 From: aarzilli Date: Mon, 11 Apr 2016 13:50:01 +0200 Subject: [PATCH] proc, terminal: next on parked goroutines Previously Next would step through the goroutine associated with CurrentThread if SelectedGoroutine was parked Also fixes a bug with proc.(*Process).StepInto where StepInto could switch to a different goroutine. --- proc/proc.go | 64 +++++++++++------------- proc/proc_test.go | 40 ++++++++++++++- proc/threads.go | 115 +++++++++++++++++--------------------------- terminal/command.go | 13 ++++- 4 files changed, 124 insertions(+), 108 deletions(-) diff --git a/proc/proc.go b/proc/proc.go index 18a70910..cec72405 100644 --- a/proc/proc.go +++ b/proc/proc.go @@ -225,8 +225,13 @@ func (dbp *Process) SetBreakpoint(addr uint64) (*Breakpoint, error) { } // SetTempBreakpoint sets a temp breakpoint. Used during 'next' operations. -func (dbp *Process) SetTempBreakpoint(addr uint64) (*Breakpoint, error) { - return dbp.setBreakpoint(dbp.CurrentThread.ID, addr, true) +func (dbp *Process) SetTempBreakpoint(addr uint64, cond ast.Expr) (*Breakpoint, error) { + bp, err := dbp.setBreakpoint(dbp.CurrentThread.ID, addr, true) + if err != nil { + return nil, err + } + bp.Cond = cond + return bp, nil } // ClearBreakpoint clears the breakpoint at addr. @@ -264,14 +269,6 @@ func (dbp *Process) Next() (err error) { } } - // Get the goroutine for the current thread. We will - // use it later in order to ensure we are on the same - // goroutine. - g, err := dbp.CurrentThread.GetG() - if err != nil { - return err - } - // Set breakpoints for any goroutine that is currently // blocked trying to read from a channel. This is so that // if control flow switches to that goroutine, we end up @@ -280,36 +277,15 @@ func (dbp *Process) Next() (err error) { return } - var goroutineExiting bool - if err = dbp.CurrentThread.setNextBreakpoints(); err != nil { - switch t := err.(type) { + if err = dbp.setNextBreakpoints(); err != nil { + switch err.(type) { case ThreadBlockedError, NoReturnAddr: // Noop - case GoroutineExitingError: - goroutineExiting = t.goid == g.ID default: dbp.ClearTempBreakpoints() return } } - if !goroutineExiting { - for i := range dbp.Breakpoints { - if dbp.Breakpoints[i].Temp { - dbp.Breakpoints[i].Cond = &ast.BinaryExpr{ - Op: token.EQL, - X: &ast.SelectorExpr{ - X: &ast.SelectorExpr{ - X: &ast.Ident{Name: "runtime"}, - Sel: &ast.Ident{Name: "curg"}, - }, - Sel: &ast.Ident{Name: "goid"}, - }, - Y: &ast.BasicLit{Kind: token.INT, Value: strconv.Itoa(g.ID)}, - } - } - } - } - return dbp.Continue() } @@ -329,7 +305,7 @@ func (dbp *Process) setChanRecvBreakpoints() (int, error) { } return 0, err } - if _, err = dbp.SetTempBreakpoint(ret); err != nil { + if _, err = dbp.SetTempBreakpoint(ret, nil); err != nil { if _, ok := err.(BreakpointExistsError); ok { // Ignore duplicate breakpoints in case if multiple // goroutines wait on the same channel @@ -492,12 +468,30 @@ func (dbp *Process) StepInto(fn *gosym.Func) error { } } pc, _ := dbp.FirstPCAfterPrologue(fn, false) - if _, err := dbp.SetTempBreakpoint(pc); err != nil { + if _, err := dbp.SetTempBreakpoint(pc, sameGoroutineCondition(dbp.SelectedGoroutine)); err != nil { return err } return dbp.Continue() } +// Returns an expression that evaluates to true when the current goroutine is g +func sameGoroutineCondition(g *G) ast.Expr { + if g == nil { + return nil + } + return &ast.BinaryExpr{ + Op: token.EQL, + X: &ast.SelectorExpr{ + X: &ast.SelectorExpr{ + X: &ast.Ident{Name: "runtime"}, + Sel: &ast.Ident{Name: "curg"}, + }, + Sel: &ast.Ident{Name: "goid"}, + }, + Y: &ast.BasicLit{Kind: token.INT, Value: strconv.Itoa(g.ID)}, + } +} + // StepInstruction will continue the current thread for exactly // one instruction. This method affects only the thread // asssociated with the selected goroutine. All other diff --git a/proc/proc_test.go b/proc/proc_test.go index 753f4ca3..90ce5b4e 100644 --- a/proc/proc_test.go +++ b/proc/proc_test.go @@ -485,7 +485,8 @@ func TestNextFunctionReturnDefer(t *testing.T) { {5, 8}, {8, 9}, {9, 10}, - {10, 7}, + {10, 6}, + {6, 7}, {7, 8}, } testnext("testnextdefer", testcases, "main.main", t) @@ -1751,3 +1752,40 @@ func TestIssue554(t *testing.T) { t.Fatalf("should be false") } } + +func TestNextParked(t *testing.T) { + withTestProcess("parallel_next", t, func(p *Process, fixture protest.Fixture) { + bp, err := setFunctionBreakpoint(p, "main.sayhi") + assertNoError(err, t, "SetBreakpoint()") + + // continue until a parked goroutine exists + var parkedg *G + LookForParkedG: + for { + err := p.Continue() + if _, exited := err.(ProcessExitedError); exited { + t.Log("could not find parked goroutine") + return + } + assertNoError(err, t, "Continue()") + + gs, err := p.GoroutinesInfo() + assertNoError(err, t, "GoroutinesInfo()") + + for _, g := range gs { + if g.thread == nil { + parkedg = g + break LookForParkedG + } + } + } + + assertNoError(p.SwitchGoroutine(parkedg.ID), t, "SwitchGoroutine()") + p.ClearBreakpoint(bp.Addr) + assertNoError(p.Next(), t, "Next()") + + if p.SelectedGoroutine.ID != parkedg.ID { + t.Fatalf("Next did not continue on the selected goroutine, expected %d got %d", parkedg.ID, p.SelectedGoroutine.ID) + } + }) +} diff --git a/proc/threads.go b/proc/threads.go index 9debb23b..910b18a1 100644 --- a/proc/threads.go +++ b/proc/threads.go @@ -5,6 +5,7 @@ import ( "encoding/binary" "errors" "fmt" + "go/ast" "path/filepath" "reflect" "runtime" @@ -124,79 +125,54 @@ func (tbe ThreadBlockedError) Error() string { } // Set breakpoints for potential next lines. -func (thread *Thread) setNextBreakpoints() (err error) { - if thread.blocked() { - return ThreadBlockedError{} +func (dbp *Process) setNextBreakpoints() (err error) { + var frames []Stackframe + + if dbp.SelectedGoroutine == nil { + if dbp.CurrentThread.blocked() { + return ThreadBlockedError{} + } + frames, err = dbp.CurrentThread.Stacktrace(0) + } else { + frames, err = dbp.SelectedGoroutine.Stacktrace(0) } - curpc, err := thread.PC() if err != nil { return err } + if len(frames) < 1 { + return errors.New("empty stack trace") + } // Grab info on our current stack frame. Used to determine // whether we may be stepping outside of the current function. - fde, err := thread.dbp.frameEntries.FDEForPC(curpc) + fde, err := dbp.frameEntries.FDEForPC(frames[0].Current.PC) if err != nil { return err } - // Get current file/line. - loc, err := thread.Location() - if err != nil { - return err + if filepath.Ext(frames[0].Current.File) != ".go" { + return dbp.cnext(frames[0], fde) } - if filepath.Ext(loc.File) == ".go" { - err = thread.next(curpc, fde, loc.File, loc.Line) - } else { - err = thread.cnext(curpc, fde, loc.File) - } - return err -} -// GoroutineExitingError is returned when the -// goroutine specified by `goid` is in the process -// of exiting. -type GoroutineExitingError struct { - goid int -} - -func (ge GoroutineExitingError) Error() string { - return fmt.Sprintf("goroutine %d is exiting", ge.goid) + return dbp.next(dbp.SelectedGoroutine, frames[0], fde) } // 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()-1, file) +// The first return value is set to true if the goroutine is in the process of exiting +func (dbp *Process) next(g *G, topframe Stackframe, fde *frame.FrameDescriptionEntry) error { + pcs := dbp.lineInfo.AllPCsBetween(fde.Begin(), fde.End()-1, topframe.Current.File) - g, err := thread.GetG() - if err != nil { - return err - } - if g.DeferPC != 0 { - f, lineno, _ := thread.dbp.goSymTable.PCToLine(g.DeferPC) - for { - lineno++ - dpc, _, err := thread.dbp.goSymTable.LineToPC(f, lineno) - if err == nil { - // We want to avoid setting an actual breakpoint on the - // entry point of the deferred function so instead create - // a fake breakpoint which will be cleaned up later. - thread.dbp.Breakpoints[g.DeferPC] = new(Breakpoint) - defer func() { delete(thread.dbp.Breakpoints, g.DeferPC) }() - if _, err = thread.dbp.SetTempBreakpoint(dpc); err != nil { - return err - } - break - } + var deferpc uint64 = 0 + if g != nil && g.DeferPC != 0 { + _, _, deferfn := dbp.goSymTable.PCToLine(g.DeferPC) + var err error + deferpc, err = dbp.FirstPCAfterPrologue(deferfn, false) + if err != nil { + return err } } - ret, err := thread.ReturnAddress() - if err != nil { - return err - } - var covered bool for i := range pcs { if fde.Cover(pcs[i]) { @@ -206,38 +182,35 @@ func (thread *Thread) next(curpc uint64, fde *frame.FrameDescriptionEntry, file } if !covered { - fn := thread.dbp.goSymTable.PCToFunc(ret) - if fn != nil && fn.Name == "runtime.goexit" { - g, err := thread.GetG() - if err != nil { - return err - } - return GoroutineExitingError{goid: g.ID} + fn := dbp.goSymTable.PCToFunc(topframe.Ret) + if g != nil && fn != nil && fn.Name == "runtime.goexit" { + return nil } } - pcs = append(pcs, ret) - return thread.setNextTempBreakpoints(curpc, pcs) + if deferpc != 0 { + pcs = append(pcs, deferpc) + } + pcs = append(pcs, topframe.Ret) + return dbp.setTempBreakpoints(topframe.Current.PC, pcs, sameGoroutineCondition(dbp.SelectedGoroutine)) } // Set a breakpoint at every reachable location, as well as the return address. Without // the benefit of an AST we can't be sure we're not at a branching statement and thus // cannot accurately predict where we may end up. -func (thread *Thread) cnext(curpc uint64, fde *frame.FrameDescriptionEntry, file string) error { - pcs := thread.dbp.lineInfo.AllPCsBetween(fde.Begin(), fde.End(), file) - ret, err := thread.ReturnAddress() - if err != nil { - return err - } - pcs = append(pcs, ret) - return thread.setNextTempBreakpoints(curpc, pcs) +func (dbp *Process) cnext(topframe Stackframe, fde *frame.FrameDescriptionEntry) error { + pcs := dbp.lineInfo.AllPCsBetween(fde.Begin(), fde.End(), topframe.Current.File) + pcs = append(pcs, topframe.Ret) + return dbp.setTempBreakpoints(topframe.Current.PC, pcs, sameGoroutineCondition(dbp.SelectedGoroutine)) } -func (thread *Thread) setNextTempBreakpoints(curpc uint64, pcs []uint64) error { +// setTempBreakpoints sets a breakpoint to all addresses specified in pcs +// skipping over curpc and curpc-1 +func (dbp *Process) setTempBreakpoints(curpc uint64, pcs []uint64, cond ast.Expr) error { for i := range pcs { if pcs[i] == curpc || pcs[i] == curpc-1 { continue } - if _, err := thread.dbp.SetTempBreakpoint(pcs[i]); err != nil { + if _, err := dbp.SetTempBreakpoint(pcs[i], cond); err != nil { if _, ok := err.(BreakpointExistsError); !ok { return err } diff --git a/terminal/command.go b/terminal/command.go index a1de1073..e4c6a849 100644 --- a/terminal/command.go +++ b/terminal/command.go @@ -101,7 +101,7 @@ See also: "help on", "help cond" and "help clear"`}, {aliases: []string{"continue", "c"}, cmdFn: cont, helpMsg: "Run until breakpoint or program termination."}, {aliases: []string{"step", "s"}, cmdFn: step, helpMsg: "Single step through program."}, {aliases: []string{"step-instruction", "si"}, cmdFn: stepInstruction, helpMsg: "Single step a single cpu instruction."}, - {aliases: []string{"next", "n"}, cmdFn: next, helpMsg: "Step over to next source line."}, + {aliases: []string{"next", "n"}, allowedPrefixes: scopePrefix, cmdFn: next, helpMsg: "Step over to next source line."}, {aliases: []string{"threads"}, cmdFn: threads, helpMsg: "Print out info for every traced thread."}, {aliases: []string{"thread", "tr"}, cmdFn: thread, helpMsg: `Switch to the specified thread. @@ -625,6 +625,17 @@ func stepInstruction(t *Term, ctx callContext, args string) error { } func next(t *Term, ctx callContext, args string) error { + if ctx.Prefix == scopePrefix { + if ctx.Scope.Frame != 0 { + return errors.New("can not prefix next with frame") + } + if ctx.Scope.GoroutineID > 0 { + _, err := t.client.SwitchGoroutine(ctx.Scope.GoroutineID) + if err != nil { + return err + } + } + } state, err := t.client.Next() if err != nil { return err