proc: next should not skip lines with conditional bps

Conditional breakpoints with unmet conditions would cause next and step
to skip the line.

This breakpoint changes the Kind field of proc.Breakpoint from a single
value to a bit field, each breakpoint object can represent
simultaneously a user breakpoint and one internal breakpoint (of which
we have several different kinds).

The breakpoint condition for internal breakpoints is stored in the new
internalCond field of proc.Breakpoint so that it will not conflict with
user specified conditions.

The breakpoint setting code is changed to allow overlapping one
internal breakpoint on a user breakpoint, or a user breakpoint on an
existing internal breakpoint. All other combinations are rejected. The
breakpoint clearing code is changed to clear the UserBreakpoint bit and
only remove the phisical breakpoint if no other bits are set in the
Kind field. ClearInternalBreakpoints does the same thing but clearing
all bits that aren't the UserBreakpoint bit.

Fixes #844

_fixtures/nextcond.go

+package main
+
+var n = 10
+
+func f1(x int) {
+}
+
+func main() {
+	f1(n)
+	f1(n)
+	f1(n)
+}

pkg/proc/breakpoints.go

@@ -21,7 +21,13 @@ type Breakpoint struct {
 	OriginalData []byte // If software breakpoint, the data we replace with breakpoint instruction.
 	Name         string // User defined name of the breakpoint
 	ID           int    // Monotonically increasing ID.
-	Kind         BreakpointKind // Whether this is an internal breakpoint (for next'ing or stepping).
+
+	// Kind describes whether this is an internal breakpoint (for next'ing or
+	// stepping).
+	// A single breakpoint can be both a UserBreakpoint and some kind of
+	// internal breakpoint, but it can not be two different kinds of internal
+	// breakpoint.
+	Kind BreakpointKind
 
 	// Breakpoint information
 	Tracepoint    bool     // Tracepoint flag
@@ -45,15 +51,17 @@ type Breakpoint struct {
 	DeferReturns []uint64
 	// Cond: if not nil the breakpoint will be triggered only if evaluating Cond returns true
 	Cond ast.Expr
+	// internalCond is the same as Cond but used for the condition of internal breakpoints
+	internalCond ast.Expr
 }
 
 // Breakpoint Kind determines the behavior of delve when the
 // breakpoint is reached.
-type BreakpointKind int
+type BreakpointKind uint16
 
 const (
 	// UserBreakpoint is a user set breakpoint
-	UserBreakpoint BreakpointKind = iota
+	UserBreakpoint BreakpointKind = (1 << iota)
 	// NextBreakpoint is a breakpoint set by Next, Continue
 	// will stop on it and delete it
 	NextBreakpoint
@@ -95,11 +103,15 @@ func (iae InvalidAddressError) Error() string {
 }
 
 // CheckCondition evaluates bp's condition on thread.
-func (bp *Breakpoint) CheckCondition(thread Thread) (bool, error) {
-	if bp.Cond == nil {
-		return true, nil
+func (bp *Breakpoint) CheckCondition(thread Thread) BreakpointState {
+	bpstate := BreakpointState{Breakpoint: bp, Active: false, Internal: false, CondError: nil}
+	if bp.Cond == nil && bp.internalCond == nil {
+		bpstate.Active = true
+		bpstate.Internal = bp.Kind != UserBreakpoint
+		return bpstate
 	}
-	if bp.Kind == NextDeferBreakpoint {
+	nextDeferOk := true
+	if bp.Kind&NextDeferBreakpoint != 0 {
 		frames, err := ThreadStacktrace(thread, 2)
 		if err == nil {
 			ispanic := len(frames) >= 3 && frames[2].Current.Fn != nil && frames[2].Current.Fn.Name == "runtime.gopanic"
@@ -112,15 +124,43 @@ func (bp *Breakpoint) CheckCondition(thread Thread) (bool, error) {
 					}
 				}
 			}
-			if !ispanic && !isdeferreturn {
-				return false, nil
+			nextDeferOk = ispanic || isdeferreturn
+		}
 	}
+	if bp.Kind != UserBreakpoint {
+		// Check internalCondition if this is also an internal breakpoint
+		bpstate.Active, bpstate.CondError = evalBreakpointCondition(thread, bp.internalCond)
+		bpstate.Active = bpstate.Active && nextDeferOk
+		if bpstate.Active || bpstate.CondError != nil {
+			bpstate.Internal = true
+			return bpstate
 		}
 	}
-	return evalBreakpointCondition(thread, bp.Cond)
+	if bp.Kind&UserBreakpoint != 0 {
+		// Check normal condition if this is also a user breakpoint
+		bpstate.Active, bpstate.CondError = evalBreakpointCondition(thread, bp.Cond)
+	}
+	return bpstate
+}
+
+// IsInternal returns true if bp is an internal breakpoint.
+// User-set breakpoints can overlap with internal breakpoints, in that case
+// both IsUser and IsInternal will be true.
+func (bp *Breakpoint) IsInternal() bool {
+	return bp.Kind != UserBreakpoint
+}
+
+// IsUser returns true if bp is a user-set breakpoint.
+// User-set breakpoints can overlap with internal breakpoints, in that case
+// both IsUser and IsInternal will be true.
+func (bp *Breakpoint) IsUser() bool {
+	return bp.Kind&UserBreakpoint != 0
 }
 
 func evalBreakpointCondition(thread Thread, cond ast.Expr) (bool, error) {
+	if cond == nil {
+		return true, nil
+	}
 	scope, err := GoroutineScope(thread)
 	if err != nil {
 		return true, err
@@ -138,11 +178,6 @@ func evalBreakpointCondition(thread Thread, cond ast.Expr) (bool, error) {
 	return constant.BoolVal(v.Value), nil
 }
 
-// Internal returns true for breakpoints not set directly by the user.
-func (bp *Breakpoint) Internal() bool {
-	return bp.Kind != UserBreakpoint
-}
-
 // NoBreakpointError is returned when trying to
 // clear a breakpoint that does not exist.
 type NoBreakpointError struct {
@@ -153,6 +188,7 @@ func (nbp NoBreakpointError) Error() string {
 	return fmt.Sprintf("no breakpoint at %#v", nbp.Addr)
 }
 
+// BreakpointMap represents an (address, breakpoint) map.
 type BreakpointMap struct {
 	M map[uint64]*Breakpoint
 
@@ -160,12 +196,14 @@ type BreakpointMap struct {
 	internalBreakpointIDCounter int
 }
 
+// NewBreakpointMap creates a new BreakpointMap.
 func NewBreakpointMap() BreakpointMap {
 	return BreakpointMap{
 		M: make(map[uint64]*Breakpoint),
 	}
 }
 
+// ResetBreakpointIDCounter resets the breakpoint ID counter of bpmap.
 func (bpmap *BreakpointMap) ResetBreakpointIDCounter() {
 	bpmap.breakpointIDCounter = 0
 }
@@ -178,8 +216,20 @@ type clearBreakpointFn func(*Breakpoint) error
 // to implement proc.Process.SetBreakpoint.
 func (bpmap *BreakpointMap) Set(addr uint64, kind BreakpointKind, cond ast.Expr, writeBreakpoint writeBreakpointFn) (*Breakpoint, error) {
 	if bp, ok := bpmap.M[addr]; ok {
+		// We can overlap one internal breakpoint with one user breakpoint, we
+		// need to support this otherwise a conditional breakpoint can mask a
+		// breakpoint set by next or step.
+		if (kind != UserBreakpoint && bp.Kind != UserBreakpoint) || (kind == UserBreakpoint && bp.Kind&UserBreakpoint != 0) {
 			return bp, BreakpointExistsError{bp.File, bp.Line, bp.Addr}
 		}
+		bp.Kind |= kind
+		if kind != UserBreakpoint {
+			bp.internalCond = cond
+		} else {
+			bp.Cond = cond
+		}
+		return bp, nil
+	}
 
 	f, l, fn, originalData, err := writeBreakpoint(addr)
 	if err != nil {
@@ -192,7 +242,6 @@ func (bpmap *BreakpointMap) Set(addr uint64, kind BreakpointKind, cond ast.Expr,
 		Line:         l,
 		Addr:         addr,
 		Kind:         kind,
-		Cond:         cond,
 		OriginalData: originalData,
 		HitCount:     map[int]uint64{},
 	}
@@ -200,9 +249,11 @@ func (bpmap *BreakpointMap) Set(addr uint64, kind BreakpointKind, cond ast.Expr,
 	if kind != UserBreakpoint {
 		bpmap.internalBreakpointIDCounter++
 		newBreakpoint.ID = bpmap.internalBreakpointIDCounter
+		newBreakpoint.internalCond = cond
 	} else {
 		bpmap.breakpointIDCounter++
 		newBreakpoint.ID = bpmap.breakpointIDCounter
+		newBreakpoint.Cond = cond
 	}
 
 	bpmap.M[addr] = newBreakpoint
@@ -228,6 +279,12 @@ func (bpmap *BreakpointMap) Clear(addr uint64, clearBreakpoint clearBreakpointFn
 		return nil, NoBreakpointError{Addr: addr}
 	}
 
+	bp.Kind &= ^UserBreakpoint
+	bp.Cond = nil
+	if bp.Kind != 0 {
+		return bp, nil
+	}
+
 	if err := clearBreakpoint(bp); err != nil {
 		return nil, err
 	}
@@ -243,7 +300,9 @@ func (bpmap *BreakpointMap) Clear(addr uint64, clearBreakpoint clearBreakpointFn
 // instead, this function is used to implement that.
 func (bpmap *BreakpointMap) ClearInternalBreakpoints(clearBreakpoint clearBreakpointFn) error {
 	for addr, bp := range bpmap.M {
-		if !bp.Internal() {
+		bp.Kind = bp.Kind & UserBreakpoint
+		bp.internalCond = nil
+		if bp.Kind != 0 {
 			continue
 		}
 		if err := clearBreakpoint(bp); err != nil {
@@ -253,3 +312,45 @@ func (bpmap *BreakpointMap) ClearInternalBreakpoints(clearBreakpoint clearBreakp
 	}
 	return nil
 }
+
+// HasInternalBreakpoints returns true if bpmap has at least one internal
+// breakpoint set.
+func (bpmap *BreakpointMap) HasInternalBreakpoints() bool {
+	for _, bp := range bpmap.M {
+		if bp.Kind != UserBreakpoint {
+			return true
+		}
+	}
+	return false
+}
+
+// BreakpointState describes the state of a breakpoint in a thread.
+type BreakpointState struct {
+	*Breakpoint
+	// Active is true if the breakpoint condition was met.
+	Active bool
+	// Internal is true if the breakpoint was matched as an internal
+	// breakpoint.
+	Internal bool
+	// CondError contains any error encountered while evaluating the
+	// breakpoint's condition.
+	CondError error
+}
+
+func (bpstate *BreakpointState) Clear() {
+	bpstate.Breakpoint = nil
+	bpstate.Active = false
+	bpstate.Internal = false
+	bpstate.CondError = nil
+}
+
+func (bpstate *BreakpointState) String() string {
+	s := bpstate.Breakpoint.String()
+	if bpstate.Active {
+		s += " active"
+	}
+	if bpstate.Internal {
+		s += " internal"
+	}
+	return s
+}

pkg/proc/core/core.go

@@ -222,8 +222,8 @@ func (t *Thread) Location() (*proc.Location, error) {
 	return &proc.Location{PC: t.th.Reg.Rip, File: f, Line: l, Fn: fn}, nil
 }
 
-func (t *Thread) Breakpoint() (*proc.Breakpoint, bool, error) {
-	return nil, false, nil
+func (t *Thread) Breakpoint() proc.BreakpointState {
+	return proc.BreakpointState{}
 }
 
 func (t *Thread) ThreadID() int {

pkg/proc/gdbserial/gdbserver.go

@@ -130,9 +130,7 @@ type Thread struct {
 	ID                int
 	strID             string
 	regs              gdbRegisters
-	CurrentBreakpoint        *proc.Breakpoint
-	BreakpointConditionMet   bool
-	BreakpointConditionError error
+	CurrentBreakpoint proc.BreakpointState
 	p                 *Process
 	setbp             bool // thread was stopped because of a breakpoint
 }
@@ -557,7 +555,7 @@ func (p *Process) ContinueOnce() (proc.Thread, error) {
 	if p.conn.direction == proc.Forward {
 		// step threads stopped at any breakpoint over their breakpoint
 		for _, thread := range p.threads {
-			if thread.CurrentBreakpoint != nil {
+			if thread.CurrentBreakpoint.Breakpoint != nil {
 				if err := thread.stepInstruction(&threadUpdater{p: p}); err != nil {
 					return nil, err
 				}
@@ -914,11 +912,9 @@ func (p *Process) Direction(dir proc.Direction) error {
 	if p.conn.direction == dir {
 		return nil
 	}
-	for _, bp := range p.Breakpoints().M {
-		if bp.Internal() {
+	if p.Breakpoints().HasInternalBreakpoints() {
 		return ErrDirChange
 	}
-	}
 	p.conn.direction = dir
 	return nil
 }
@@ -971,8 +967,8 @@ func (p *Process) ClearInternalBreakpoints() error {
 			return err
 		}
 		for _, thread := range p.threads {
-			if thread.CurrentBreakpoint == bp {
-				thread.CurrentBreakpoint = nil
+			if thread.CurrentBreakpoint.Breakpoint == bp {
+				thread.clearBreakpointState()
 			}
 		}
 		return nil
@@ -1098,7 +1094,7 @@ func (p *Process) setCurrentBreakpoints() error {
 	}
 	if !p.threadStopInfo {
 		for _, th := range p.threads {
-			if th.CurrentBreakpoint == nil {
+			if th.CurrentBreakpoint.Breakpoint == nil {
 				err := th.SetCurrentBreakpoint()
 				if err != nil {
 					return err
@@ -1131,8 +1127,8 @@ func (t *Thread) Location() (*proc.Location, error) {
 	return &proc.Location{PC: pc, File: f, Line: l, Fn: fn}, nil
 }
 
-func (t *Thread) Breakpoint() (breakpoint *proc.Breakpoint, active bool, condErr error) {
-	return t.CurrentBreakpoint, (t.CurrentBreakpoint != nil && t.BreakpointConditionMet), t.BreakpointConditionError
+func (t *Thread) Breakpoint() proc.BreakpointState {
+	return t.CurrentBreakpoint
 }
 
 func (t *Thread) ThreadID() int {
@@ -1431,13 +1427,11 @@ func (t *Thread) reloadGAlloc() error {
 
 func (t *Thread) clearBreakpointState() {
 	t.setbp = false
-	t.CurrentBreakpoint = nil
-	t.BreakpointConditionMet = false
-	t.BreakpointConditionError = nil
+	t.CurrentBreakpoint.Clear()
 }
 
 func (thread *Thread) SetCurrentBreakpoint() error {
-	thread.CurrentBreakpoint = nil
+	thread.clearBreakpointState()
 	regs, err := thread.Registers(false)
 	if err != nil {
 		return err
@@ -1449,9 +1443,8 @@ func (thread *Thread) SetCurrentBreakpoint() error {
 				return err
 			}
 		}
-		thread.CurrentBreakpoint = bp
-		thread.BreakpointConditionMet, thread.BreakpointConditionError = bp.CheckCondition(thread)
-		if thread.CurrentBreakpoint != nil && thread.BreakpointConditionMet {
+		thread.CurrentBreakpoint = bp.CheckCondition(thread)
+		if thread.CurrentBreakpoint.Breakpoint != nil && thread.CurrentBreakpoint.Active {
 			if g, err := proc.GetG(thread); err == nil {
 				thread.CurrentBreakpoint.HitCount[g.ID]++
 			}

pkg/proc/native/proc.go

@@ -210,8 +210,7 @@ func (dbp *Process) writeBreakpoint(addr uint64) (string, int, *proc.Function, [
 }
 
 // SetBreakpoint sets a breakpoint at addr, and stores it in the process wide
-// break point table. Setting a break point must be thread specific due to
-// ptrace actions needing the thread to be in a signal-delivery-stop.
+// break point table.
 func (dbp *Process) SetBreakpoint(addr uint64, kind proc.BreakpointKind, cond ast.Expr) (*proc.Breakpoint, error) {
 	return dbp.breakpoints.Set(addr, kind, cond, dbp.writeBreakpoint)
 }
@@ -235,7 +234,7 @@ func (dbp *Process) ContinueOnce() (proc.Thread, error) {
 
 	dbp.allGCache = nil
 	for _, th := range dbp.threads {
-		th.clearBreakpointState()
+		th.CurrentBreakpoint.Clear()
 	}
 
 	if dbp.resumeChan != nil {
@@ -276,7 +275,7 @@ func (dbp *Process) StepInstruction() (err error) {
 	if dbp.exited {
 		return &proc.ProcessExitedError{Pid: dbp.Pid()}
 	}
-	thread.clearBreakpointState()
+	thread.CurrentBreakpoint.Clear()
 	err = thread.StepInstruction()
 	if err != nil {
 		return err
@@ -386,8 +385,8 @@ func (dbp *Process) ClearInternalBreakpoints() error {
 			return err
 		}
 		for _, thread := range dbp.threads {
-			if thread.CurrentBreakpoint == bp {
-				thread.CurrentBreakpoint = nil
+			if thread.CurrentBreakpoint.Breakpoint == bp {
+				thread.CurrentBreakpoint.Clear()
 			}
 		}
 		return nil

pkg/proc/native/proc_darwin.go

@@ -99,7 +99,7 @@ func Launch(cmd []string, wd string) (*Process, error) {
 
 	dbp.allGCache = nil
 	for _, th := range dbp.threads {
-		th.clearBreakpointState()
+		th.CurrentBreakpoint.Clear()
 	}
 
 	trapthread, err := dbp.trapWait(-1)
@@ -425,11 +425,11 @@ func (dbp *Process) exitGuard(err error) error {
 func (dbp *Process) resume() error {
 	// all threads stopped over a breakpoint are made to step over it
 	for _, thread := range dbp.threads {
-		if thread.CurrentBreakpoint != nil {
+		if thread.CurrentBreakpoint.Breakpoint != nil {
 			if err := thread.StepInstruction(); err != nil {
 				return err
 			}
-			thread.CurrentBreakpoint = nil
+			thread.CurrentBreakpoint.Clear()
 		}
 	}
 	// everything is resumed

pkg/proc/native/proc_linux.go

@@ -369,7 +369,7 @@ func (dbp *Process) wait(pid, options int) (int, *sys.WaitStatus, error) {
 
 func (dbp *Process) setCurrentBreakpoints(trapthread *Thread) error {
 	for _, th := range dbp.threads {
-		if th.CurrentBreakpoint == nil {
+		if th.CurrentBreakpoint.Breakpoint == nil {
 			if err := th.SetCurrentBreakpoint(); err != nil {
 				if err == sys.ESRCH {
 					// This thread quit between the point where we received the breakpoint and
@@ -399,11 +399,11 @@ func (dbp *Process) exitGuard(err error) error {
 func (dbp *Process) resume() error {
 	// all threads stopped over a breakpoint are made to step over it
 	for _, thread := range dbp.threads {
-		if thread.CurrentBreakpoint != nil {
+		if thread.CurrentBreakpoint.Breakpoint != nil {
 			if err := thread.StepInstruction(); err != nil {
 				return err
 			}
-			thread.CurrentBreakpoint = nil
+			thread.CurrentBreakpoint.Clear()
 		}
 	}
 	// everything is resumed

pkg/proc/native/proc_windows.go

@@ -447,11 +447,11 @@ func (dbp *Process) exitGuard(err error) error {
 
 func (dbp *Process) resume() error {
 	for _, thread := range dbp.threads {
-		if thread.CurrentBreakpoint != nil {
+		if thread.CurrentBreakpoint.Breakpoint != nil {
 			if err := thread.StepInstruction(); err != nil {
 				return err
 			}
-			thread.CurrentBreakpoint = nil
+			thread.CurrentBreakpoint.Clear()
 		}
 	}
 

pkg/proc/native/threads.go

@@ -14,9 +14,7 @@ import (
 type Thread struct {
 	ID                int                  // Thread ID or mach port
 	Status            *WaitStatus          // Status returned from last wait call
-	CurrentBreakpoint        *proc.Breakpoint // Breakpoint thread is currently stopped at
-	BreakpointConditionMet   bool             // Output of evaluating the breakpoint's condition
-	BreakpointConditionError error            // Error evaluating the breakpoint's condition
+	CurrentBreakpoint proc.BreakpointState // Breakpoint thread is currently stopped at
 
 	dbp            *Process
 	singleStepping bool
@@ -141,18 +139,17 @@ func (thread *Thread) Halt() (err error) {
 // SetCurrentBreakpoint sets the current breakpoint that this
 // thread is stopped at as CurrentBreakpoint on the thread struct.
 func (thread *Thread) SetCurrentBreakpoint() error {
-	thread.CurrentBreakpoint = nil
+	thread.CurrentBreakpoint.Clear()
 	pc, err := thread.PC()
 	if err != nil {
 		return err
 	}
 	if bp, ok := thread.dbp.FindBreakpoint(pc); ok {
-		thread.CurrentBreakpoint = bp
 		if err = thread.SetPC(bp.Addr); err != nil {
 			return err
 		}
-		thread.BreakpointConditionMet, thread.BreakpointConditionError = bp.CheckCondition(thread)
-		if thread.CurrentBreakpoint != nil && thread.BreakpointConditionMet {
+		thread.CurrentBreakpoint = bp.CheckCondition(thread)
+		if thread.CurrentBreakpoint.Breakpoint != nil && thread.CurrentBreakpoint.Active {
 			if g, err := proc.GetG(thread); err == nil {
 				thread.CurrentBreakpoint.HitCount[g.ID]++
 			}
@@ -162,14 +159,8 @@ func (thread *Thread) SetCurrentBreakpoint() error {
 	return nil
 }
 
-func (thread *Thread) clearBreakpointState() {
-	thread.CurrentBreakpoint = nil
-	thread.BreakpointConditionMet = false
-	thread.BreakpointConditionError = nil
-}
-
-func (th *Thread) Breakpoint() (*proc.Breakpoint, bool, error) {
-	return th.CurrentBreakpoint, th.CurrentBreakpoint != nil && th.BreakpointConditionMet, th.BreakpointConditionError
+func (th *Thread) Breakpoint() proc.BreakpointState {
+	return th.CurrentBreakpoint
 }
 
 func (th *Thread) ThreadID() int {

pkg/proc/proc.go

@@ -68,11 +68,9 @@ func Next(dbp Process) (err error) {
 	if dbp.Exited() {
 		return &ProcessExitedError{Pid: dbp.Pid()}
 	}
-	for _, bp := range dbp.Breakpoints().M {
-		if bp.Internal() {
+	if dbp.Breakpoints().HasInternalBreakpoints() {
 		return fmt.Errorf("next while nexting")
 	}
-	}
 
 	if err = next(dbp, false); err != nil {
 		dbp.ClearInternalBreakpoints()
@@ -106,10 +104,10 @@ func Continue(dbp Process) error {
 		}
 
 		curthread := dbp.CurrentThread()
-		curbp, curbpActive, _ := curthread.Breakpoint()
+		curbp := curthread.Breakpoint()
 
 		switch {
-		case curbp == nil:
+		case curbp.Breakpoint == nil:
 			// runtime.Breakpoint or manual stop
 			if recorded, _ := dbp.Recorded(); onRuntimeBreakpoint(curthread) && !recorded {
 				// Single-step current thread until we exit runtime.breakpoint and
@@ -127,7 +125,7 @@ func Continue(dbp Process) error {
 				}
 			}
 			return conditionErrors(threads)
-		case curbpActive && curbp.Internal():
+		case curbp.Active && curbp.Internal:
 			if curbp.Kind == StepBreakpoint {
 				// See description of proc.(*Process).next for the meaning of StepBreakpoints
 				if err := conditionErrors(threads); err != nil {
@@ -154,7 +152,7 @@ func Continue(dbp Process) error {
 				}
 				return conditionErrors(threads)
 			}
-		case curbpActive:
+		case curbp.Active:
 			onNextGoroutine, err := onNextGoroutine(curthread, dbp.Breakpoints())
 			if err != nil {
 				return err
@@ -178,9 +176,9 @@ func Continue(dbp Process) error {
 func conditionErrors(threads []Thread) error {
 	var condErr error
 	for _, th := range threads {
-		if bp, _, bperr := th.Breakpoint(); bp != nil && bperr != nil {
+		if bp := th.Breakpoint(); bp.Breakpoint != nil && bp.CondError != nil {
 			if condErr == nil {
-				condErr = bperr
+				condErr = bp.CondError
 			} else {
 				return fmt.Errorf("multiple errors evaluating conditions")
 			}
@@ -195,15 +193,15 @@ func conditionErrors(threads []Thread) error {
 // 	- trapthread
 func pickCurrentThread(dbp Process, trapthread Thread, threads []Thread) error {
 	for _, th := range threads {
-		if bp, active, _ := th.Breakpoint(); active && bp.Internal() {
+		if bp := th.Breakpoint(); bp.Active && bp.Internal {
 			return dbp.SwitchThread(th.ThreadID())
 		}
 	}
-	if _, active, _ := trapthread.Breakpoint(); active {
+	if bp := trapthread.Breakpoint(); bp.Active {
 		return dbp.SwitchThread(trapthread.ThreadID())
 	}
 	for _, th := range threads {
-		if _, active, _ := th.Breakpoint(); active {
+		if bp := th.Breakpoint(); bp.Active {
 			return dbp.SwitchThread(th.ThreadID())
 		}
 	}
@@ -216,11 +214,9 @@ func Step(dbp Process) (err error) {
 	if dbp.Exited() {
 		return &ProcessExitedError{Pid: dbp.Pid()}
 	}
-	for _, bp := range dbp.Breakpoints().M {
-		if bp.Internal() {
+	if dbp.Breakpoints().HasInternalBreakpoints() {
 		return fmt.Errorf("next while nexting")
 	}
-	}
 
 	if err = next(dbp, true); err != nil {
 		switch err.(type) {
@@ -336,7 +332,7 @@ func StepOut(dbp Process) error {
 		}
 	}
 
-	if bp, _, _ := curthread.Breakpoint(); bp == nil {
+	if bp := curthread.Breakpoint(); bp.Breakpoint == nil {
 		curthread.SetCurrentBreakpoint()
 	}
 

pkg/proc/proc_test.go

@@ -550,7 +550,7 @@ func TestNextConcurrentVariant2(t *testing.T) {
 				}
 				assertNoError(err, t, "EvalVariable")
 				vval, _ = constant.Int64Val(v.Value)
-				if bp, _, _ := p.CurrentThread().Breakpoint(); bp == nil {
+				if bpstate := p.CurrentThread().Breakpoint(); bpstate.Breakpoint == nil {
 					if vval != initVval {
 						t.Fatal("Did not end up on same goroutine")
 					}
@@ -1038,11 +1038,11 @@ func TestContinueMulti(t *testing.T) {
 			}
 			assertNoError(err, t, "Continue()")
 
-			if bp, _, _ := p.CurrentThread().Breakpoint(); bp.ID == bp1.ID {
+			if bp := p.CurrentThread().Breakpoint(); bp.ID == bp1.ID {
 				mainCount++
 			}
 
-			if bp, _, _ := p.CurrentThread().Breakpoint(); bp.ID == bp2.ID {
+			if bp := p.CurrentThread().Breakpoint(); bp.ID == bp2.ID {
 				sayhiCount++
 			}
 		}
@@ -1447,7 +1447,7 @@ func TestBreakpointCountsWithDetection(t *testing.T) {
 				assertNoError(err, t, "Continue()")
 			}
 			for _, th := range p.ThreadList() {
-				if bp, _, _ := th.Breakpoint(); bp == nil {
+				if bp := th.Breakpoint(); bp.Breakpoint == nil {
 					continue
 				}
 				scope, err := proc.GoroutineScope(th)
@@ -1864,8 +1864,8 @@ func TestPanicBreakpoint(t *testing.T) {
 	protest.AllowRecording(t)
 	withTestProcess("panic", t, func(p proc.Process, fixture protest.Fixture) {
 		assertNoError(proc.Continue(p), t, "Continue()")
-		bp, _, _ := p.CurrentThread().Breakpoint()
-		if bp == nil || bp.Name != proc.UnrecoveredPanic {
+		bp := p.CurrentThread().Breakpoint()
+		if bp.Breakpoint == nil || bp.Name != proc.UnrecoveredPanic {
 			t.Fatalf("not on unrecovered-panic breakpoint: %v", bp)
 		}
 	})
@@ -1874,8 +1874,8 @@ func TestPanicBreakpoint(t *testing.T) {
 func TestCmdLineArgs(t *testing.T) {
 	expectSuccess := func(p proc.Process, fixture protest.Fixture) {
 		err := proc.Continue(p)
-		bp, _, _ := p.CurrentThread().Breakpoint()
-		if bp != nil && bp.Name == proc.UnrecoveredPanic {
+		bp := p.CurrentThread().Breakpoint()
+		if bp.Breakpoint != nil && bp.Name == proc.UnrecoveredPanic {
 			t.Fatalf("testing args failed on unrecovered-panic breakpoint: %v", bp)
 		}
 		exit, exited := err.(proc.ProcessExitedError)
@@ -1890,8 +1890,8 @@ func TestCmdLineArgs(t *testing.T) {
 
 	expectPanic := func(p proc.Process, fixture protest.Fixture) {
 		proc.Continue(p)
-		bp, _, _ := p.CurrentThread().Breakpoint()
-		if bp == nil || bp.Name != proc.UnrecoveredPanic {
+		bp := p.CurrentThread().Breakpoint()
+		if bp.Breakpoint == nil || bp.Name != proc.UnrecoveredPanic {
 			t.Fatalf("not on unrecovered-panic breakpoint: %v", bp)
 		}
 	}
@@ -2326,15 +2326,6 @@ func TestStepConcurrentDirect(t *testing.T) {
 	})
 }
 
-func nextInProgress(p proc.Process) bool {
-	for _, bp := range p.Breakpoints().M {
-		if bp.Internal() {
-			return true
-		}
-	}
-	return false
-}
-
 func TestStepConcurrentPtr(t *testing.T) {
 	protest.AllowRecording(t)
 	withTestProcess("teststepconcurrent", t, func(p proc.Process, fixture protest.Fixture) {
@@ -2364,10 +2355,9 @@ func TestStepConcurrentPtr(t *testing.T) {
 			f, ln := currentLineNumber(p, t)
 			if ln != 24 {
 				for _, th := range p.ThreadList() {
-					bp, bpactive, bperr := th.Breakpoint()
-					t.Logf("thread %d stopped on breakpoint %v %v %v", th.ThreadID(), bp, bpactive, bperr)
+					t.Logf("thread %d stopped on breakpoint %v", th.ThreadID(), th.Breakpoint())
 				}
-				curbp, _, _ := p.CurrentThread().Breakpoint()
+				curbp := p.CurrentThread().Breakpoint()
 				t.Fatalf("Program did not continue at expected location (24): %s:%d %#x [%v] (gid %d count %d)", f, ln, currentPC(p, t), curbp, p.SelectedGoroutine().ID, count)
 			}
 
@@ -2385,7 +2375,7 @@ func TestStepConcurrentPtr(t *testing.T) {
 			kvals[gid] = k
 
 			assertNoError(proc.Step(p), t, "Step()")
-			for nextInProgress(p) {
+			for p.Breakpoints().HasInternalBreakpoints() {
 				if p.SelectedGoroutine().ID == gid {
 					t.Fatalf("step did not step into function call (but internal breakpoints still active?) (%d %d)", gid, p.SelectedGoroutine().ID)
 				}
@@ -2705,7 +2695,7 @@ func TestStacktraceWithBarriers(t *testing.T) {
 			gs, err := proc.GoroutinesInfo(p)
 			assertNoError(err, t, "GoroutinesInfo()")
 			for _, th := range p.ThreadList() {
-				if bp, _, _ := th.Breakpoint(); bp == nil {
+				if bp := th.Breakpoint(); bp.Breakpoint == nil {
 					continue
 				}
 
@@ -3140,3 +3130,23 @@ func TestAttachStripped(t *testing.T) {
 	}
 	os.Remove(fixture.Path)
 }
+
+func TestIssue844(t *testing.T) {
+	// Conditional breakpoints should not prevent next from working if their
+	// condition isn't met.
+	withTestProcess("nextcond", t, func(p proc.Process, fixture protest.Fixture) {
+		setFileBreakpoint(p, t, fixture, 9)
+		condbp := setFileBreakpoint(p, t, fixture, 10)
+		condbp.Cond = &ast.BinaryExpr{
+			Op: token.EQL,
+			X:  &ast.Ident{Name: "n"},
+			Y:  &ast.BasicLit{Kind: token.INT, Value: "11"},
+		}
+		assertNoError(proc.Continue(p), t, "Continue")
+		assertNoError(proc.Next(p), t, "Next")
+		f, l := currentLineNumber(p, t)
+		if l != 10 {
+			t.Fatalf("continued to wrong location %s:%d (expected 10)", f, l)
+		}
+	})
+}

pkg/proc/scope_test.go

@@ -86,7 +86,7 @@ func TestScope(t *testing.T) {
 				}
 				assertNoError(err, t, "Continue()")
 			}
-			bp, _, _ := p.CurrentThread().Breakpoint()
+			bp := p.CurrentThread().Breakpoint()
 
 			scopeCheck := findScopeCheck(scopeChecks, bp.Line)
 			if scopeCheck == nil {

pkg/proc/threads.go

@@ -19,11 +19,7 @@ type Thread interface {
 	Location() (*Location, error)
 	// Breakpoint will return the breakpoint that this thread is stopped at or
 	// nil if the thread is not stopped at any breakpoint.
-	// Active will be true if the thread is stopped at a breakpoint and the
-	// breakpoint's condition is met.
-	// If there was an error evaluating the breakpoint's condition it will be
-	// returned as condErr
-	Breakpoint() (breakpoint *Breakpoint, active bool, condErr error)
+	Breakpoint() BreakpointState
 	ThreadID() int
 	Registers(floatingPoint bool) (Registers, error)
 	Arch() Arch
@@ -261,7 +257,7 @@ func next(dbp Process, stepInto bool) error {
 		// there it's ok.
 	}
 
-	if bp, _, _ := curthread.Breakpoint(); bp == nil {
+	if bp := curthread.Breakpoint(); bp.Breakpoint == nil {
 		curthread.SetCurrentBreakpoint()
 	}
 	success = true
@@ -414,7 +410,7 @@ func onRuntimeBreakpoint(thread Thread) bool {
 func onNextGoroutine(thread Thread, breakpoints *BreakpointMap) (bool, error) {
 	var bp *Breakpoint
 	for i := range breakpoints.M {
-		if breakpoints.M[i].Internal() && breakpoints.M[i].Cond != nil {
+		if breakpoints.M[i].Kind != UserBreakpoint && breakpoints.M[i].internalCond != nil {
 			bp = breakpoints.M[i]
 			break
 		}
@@ -432,7 +428,7 @@ func onNextGoroutine(thread Thread, breakpoints *BreakpointMap) (bool, error) {
 	//   runtime.curg.goid == X && (runtime.frameoff == Y || runtime.frameoff == Z)
 	// Here we are only interested in testing the runtime.curg.goid clause.
 	w := onNextGoroutineWalker{thread: thread}
-	ast.Walk(&w, bp.Cond)
+	ast.Walk(&w, bp.internalCond)
 	return w.ret, w.err
 }
 

service/api/conversions.go

@@ -64,8 +64,8 @@ func ConvertThread(th proc.Thread) *Thread {
 
 	var bp *Breakpoint
 
-	if b, active, _ := th.Breakpoint(); active {
-		bp = ConvertBreakpoint(b)
+	if b := th.Breakpoint(); b.Active {
+		bp = ConvertBreakpoint(b.Breakpoint)
 	}
 
 	if g, _ := proc.GetG(th); g != nil {

service/debugger/debugger.go

@@ -264,12 +264,7 @@ func (d *Debugger) state() (*api.DebuggerState, error) {
 		}
 	}
 
-	for _, bp := range d.target.Breakpoints().M {
-		if bp.Internal() {
-			state.NextInProgress = true
-			break
-		}
-	}
+	state.NextInProgress = d.target.Breakpoints().HasInternalBreakpoints()
 
 	if recorded, _ := d.target.Recorded(); recorded {
 		state.When, _ = d.target.When()
@@ -399,11 +394,10 @@ func (d *Debugger) Breakpoints() []*api.Breakpoint {
 func (d *Debugger) breakpoints() []*api.Breakpoint {
 	bps := []*api.Breakpoint{}
 	for _, bp := range d.target.Breakpoints().M {
-		if bp.Internal() {
-			continue
-		}
+		if bp.IsUser() {
 			bps = append(bps, api.ConvertBreakpoint(bp))
 		}
+	}
 	return bps
 }