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

@@ -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

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)
+		}
+	})
+}

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
 			}

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