proc/*: move Set/Clear Breakpoint methods to Target (#2064)

pkg/proc/breakpoints.go

@@ -237,21 +237,13 @@ func NewBreakpointMap() BreakpointMap {
 	}
 }
 
-// ResetBreakpointIDCounter resets the breakpoint ID counter of bpmap.
+// SetBreakpoint sets a breakpoint at addr, and stores it in the process wide
-func (bpmap *BreakpointMap) ResetBreakpointIDCounter() {
+// break point table.
-	bpmap.breakpointIDCounter = 0
+func (t *Target) SetBreakpoint(addr uint64, kind BreakpointKind, cond ast.Expr) (*Breakpoint, error) {
-}
+	if valid, err := t.Valid(); !valid {
-
+		return nil, err
-// WriteBreakpointFn is a type that represents a function to be used for
+	}
-// writting breakpoings into the target.
+	bpmap := t.Breakpoints()
-type WriteBreakpointFn func(addr uint64) (file string, line int, fn *Function, originalData []byte, err error)
-
-type clearBreakpointFn func(*Breakpoint) error
-
-// Set creates a breakpoint at addr calling writeBreakpoint. Do not call this
-// function, call proc.Process.SetBreakpoint instead, this function exists
-// 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
@@ -268,7 +260,7 @@ func (bpmap *BreakpointMap) Set(addr uint64, kind BreakpointKind, cond ast.Expr,
 		return bp, nil
 	}
 
-	f, l, fn, originalData, err := writeBreakpoint(addr)
+	f, l, fn, originalData, err := t.proc.WriteBreakpoint(addr)
 	if err != nil {
 		return nil, err
 	}
@@ -303,9 +295,10 @@ func (bpmap *BreakpointMap) Set(addr uint64, kind BreakpointKind, cond ast.Expr,
 	return newBreakpoint, nil
 }
 
-// SetWithID creates a breakpoint at addr, with the specified logical ID.
+// setBreakpointWithID creates a breakpoint at addr, with the specified logical ID.
-func (bpmap *BreakpointMap) SetWithID(id int, addr uint64, writeBreakpoint WriteBreakpointFn) (*Breakpoint, error) {
+func (t *Target) setBreakpointWithID(id int, addr uint64) (*Breakpoint, error) {
-	bp, err := bpmap.Set(addr, UserBreakpoint, nil, writeBreakpoint)
+	bpmap := t.Breakpoints()
+	bp, err := t.SetBreakpoint(addr, UserBreakpoint, nil)
 	if err == nil {
 		bp.LogicalID = id
 		bpmap.breakpointIDCounter--
@@ -313,9 +306,12 @@ func (bpmap *BreakpointMap) SetWithID(id int, addr uint64, writeBreakpoint Write
 	return bp, err
 }
 
-// Clear clears the breakpoint at addr.
+// ClearBreakpoint clears the breakpoint at addr.
-// Do not call this function call proc.Process.ClearBreakpoint instead.
+func (t *Target) ClearBreakpoint(addr uint64) (*Breakpoint, error) {
-func (bpmap *BreakpointMap) Clear(addr uint64, clearBreakpoint clearBreakpointFn) (*Breakpoint, error) {
+	if valid, err := t.Valid(); !valid {
+		return nil, err
+	}
+	bpmap := t.Breakpoints()
 	bp, ok := bpmap.M[addr]
 	if !ok {
 		return nil, NoBreakpointError{Addr: addr}
@@ -327,7 +323,7 @@ func (bpmap *BreakpointMap) Clear(addr uint64, clearBreakpoint clearBreakpointFn
 		return bp, nil
 	}
 
-	if err := clearBreakpoint(bp); err != nil {
+	if err := t.proc.EraseBreakpoint(bp); err != nil {
 		return nil, err
 	}
 
@@ -338,9 +334,9 @@ func (bpmap *BreakpointMap) Clear(addr uint64, clearBreakpoint clearBreakpointFn
 
 // ClearInternalBreakpoints removes all internal breakpoints from the map,
 // calling clearBreakpoint on each one.
-// Do not call this function, call proc.Process.ClearInternalBreakpoints
+func (t *Target) ClearInternalBreakpoints() error {
-// instead, this function is used to implement that.
+	bpmap := t.Breakpoints()
-func (bpmap *BreakpointMap) ClearInternalBreakpoints(clearBreakpoint clearBreakpointFn) error {
+	threads := t.ThreadList()
 	for addr, bp := range bpmap.M {
 		bp.Kind = bp.Kind & UserBreakpoint
 		bp.internalCond = nil
@@ -348,9 +344,14 @@ func (bpmap *BreakpointMap) ClearInternalBreakpoints(clearBreakpoint clearBreakp
 		if bp.Kind != 0 {
 			continue
 		}
-		if err := clearBreakpoint(bp); err != nil {
+		if err := t.proc.EraseBreakpoint(bp); err != nil {
 			return err
 		}
+		for _, thread := range threads {
+			if thread.Breakpoint().Breakpoint == bp {
+				thread.Breakpoint().Clear()
+			}
+		}
 		delete(bpmap.M, addr)
 	}
 	return nil

pkg/proc/core/core.go

@@ -3,7 +3,6 @@ package core
 import (
 	"errors"
 	"fmt"
-	"go/ast"
 	"io"
 
 	"github.com/go-delve/delve/pkg/proc"
@@ -216,7 +215,6 @@ func OpenCore(corePath, exePath string, debugInfoDirs []string) (*proc.Target, e
 	return proc.NewTarget(p, proc.NewTargetConfig{
 		Path:                exePath,
 		DebugInfoDirs:       debugInfoDirs,
-		WriteBreakpoint:     p.writeBreakpoint,
 		DisableAsyncPreempt: false,
 		StopReason:          proc.StopAttached})
 }
@@ -231,9 +229,9 @@ func (p *process) EntryPoint() (uint64, error) {
 	return p.entryPoint, nil
 }
 
-// writeBreakpoint is a noop function since you
+// WriteBreakpoint is a noop function since you
 // cannot write breakpoints into core files.
-func (p *process) writeBreakpoint(addr uint64) (file string, line int, fn *proc.Function, originalData []byte, err error) {
+func (p *process) WriteBreakpoint(addr uint64) (file string, line int, fn *proc.Function, originalData []byte, err error) {
 	return "", 0, nil, nil, errors.New("cannot write a breakpoint to a core file")
 }
 
@@ -365,10 +363,10 @@ func (p *process) Breakpoints() *proc.BreakpointMap {
 	return &p.breakpoints
 }
 
-// ClearBreakpoint will always return an error as you cannot set or clear
+// EraseBreakpoint will always return an error as you cannot set or clear
 // breakpoints on core files.
-func (p *process) ClearBreakpoint(addr uint64) (*proc.Breakpoint, error) {
+func (p *process) EraseBreakpoint(bp *proc.Breakpoint) error {
-	return nil, proc.NoBreakpointError{Addr: addr}
+	return proc.NoBreakpointError{Addr: bp.Addr}
 }
 
 // ClearInternalBreakpoints will always return nil and have no
@@ -430,11 +428,6 @@ func (p *process) Pid() int {
 func (p *process) ResumeNotify(chan<- struct{}) {
 }
 
-// SetBreakpoint will always return an error for core files as you cannot write memory or control execution.
-func (p *process) SetBreakpoint(addr uint64, kind proc.BreakpointKind, cond ast.Expr) (*proc.Breakpoint, error) {
-	return nil, ErrWriteCore
-}
-
 // ThreadList will return a list of all threads currently in the process.
 func (p *process) ThreadList() []proc.Thread {
 	r := make([]proc.Thread, 0, len(p.Threads))

pkg/proc/gdbserial/gdbserver.go

@@ -67,7 +67,6 @@ import (
 	"encoding/binary"
 	"errors"
 	"fmt"
-	"go/ast"
 	"net"
 	"os"
 	"os/exec"
@@ -555,7 +554,6 @@ func (p *gdbProcess) initialize(path string, debugInfoDirs []string, stopReason
 	tgt, err := proc.NewTarget(p, proc.NewTargetConfig{
 		Path:                path,
 		DebugInfoDirs:       debugInfoDirs,
-		WriteBreakpoint:     p.writeBreakpoint,
 		DisableAsyncPreempt: runtime.GOOS == "darwin",
 		StopReason:          stopReason})
 	if err != nil {
@@ -1083,7 +1081,7 @@ func (p *gdbProcess) FindBreakpoint(pc uint64) (*proc.Breakpoint, bool) {
 	return nil, false
 }
 
-func (p *gdbProcess) writeBreakpoint(addr uint64) (string, int, *proc.Function, []byte, error) {
+func (p *gdbProcess) WriteBreakpoint(addr uint64) (string, int, *proc.Function, []byte, error) {
 	f, l, fn := p.bi.PCToLine(uint64(addr))
 
 	if err := p.conn.setBreakpoint(addr); err != nil {
@@ -1093,37 +1091,8 @@ func (p *gdbProcess) writeBreakpoint(addr uint64) (string, int, *proc.Function,
 	return f, l, fn, nil, nil
 }
 
-// SetBreakpoint creates a new breakpoint.
+func (p *gdbProcess) EraseBreakpoint(bp *proc.Breakpoint) error {
-func (p *gdbProcess) SetBreakpoint(addr uint64, kind proc.BreakpointKind, cond ast.Expr) (*proc.Breakpoint, error) {
+	return p.conn.clearBreakpoint(bp.Addr)
-	if p.exited {
-		return nil, &proc.ErrProcessExited{Pid: p.conn.pid}
-	}
-	return p.breakpoints.Set(addr, kind, cond, p.writeBreakpoint)
-}
-
-// ClearBreakpoint clears a breakpoint at the given address.
-func (p *gdbProcess) ClearBreakpoint(addr uint64) (*proc.Breakpoint, error) {
-	if p.exited {
-		return nil, &proc.ErrProcessExited{Pid: p.conn.pid}
-	}
-	return p.breakpoints.Clear(addr, func(bp *proc.Breakpoint) error {
-		return p.conn.clearBreakpoint(bp.Addr)
-	})
-}
-
-// ClearInternalBreakpoints clear all internal use breakpoints like those set by 'next'.
-func (p *gdbProcess) ClearInternalBreakpoints() error {
-	return p.breakpoints.ClearInternalBreakpoints(func(bp *proc.Breakpoint) error {
-		if err := p.conn.clearBreakpoint(bp.Addr); err != nil {
-			return err
-		}
-		for _, thread := range p.threads {
-			if thread.CurrentBreakpoint.Breakpoint == bp {
-				thread.clearBreakpointState()
-			}
-		}
-		return nil
-	})
 }
 
 type threadUpdater struct {

pkg/proc/gdbserial/rr_test.go

@@ -49,7 +49,7 @@ func assertNoError(err error, t testing.TB, s string) {
 	}
 }
 
-func setFunctionBreakpoint(p proc.Process, t *testing.T, fname string) *proc.Breakpoint {
+func setFunctionBreakpoint(p *proc.Target, t *testing.T, fname string) *proc.Breakpoint {
 	_, f, l, _ := runtime.Caller(1)
 	f = filepath.Base(f)
 
@@ -117,7 +117,7 @@ func TestRestartDuringStop(t *testing.T) {
 	})
 }
 
-func setFileBreakpoint(p proc.Process, t *testing.T, fixture protest.Fixture, lineno int) *proc.Breakpoint {
+func setFileBreakpoint(p *proc.Target, t *testing.T, fixture protest.Fixture, lineno int) *proc.Breakpoint {
 	_, f, l, _ := runtime.Caller(1)
 	f = filepath.Base(f)
 

pkg/proc/interface.go

@@ -1,9 +1,5 @@
 package proc
 
-import (
-	"go/ast"
-)
-
 // Process represents the target of the debugger. This
 // target could be a system process, core file, etc.
 //
@@ -14,8 +10,9 @@ import (
 type Process interface {
 	Info
 	ProcessManipulation
-	BreakpointManipulation
 	RecordingManipulation
+
+	Breakpoints() *BreakpointMap
 }
 
 // ProcessInternal holds a set of methods that are not meant to be called by
@@ -32,6 +29,9 @@ type ProcessInternal interface {
 	Restart(pos string) error
 	Detach(bool) error
 	ContinueOnce() (trapthread Thread, stopReason StopReason, err error)
+
+	WriteBreakpoint(addr uint64) (file string, line int, fn *Function, originalData []byte, err error)
+	EraseBreakpoint(*Breakpoint) error
 }
 
 // RecordingManipulation is an interface for manipulating process recordings.
@@ -101,11 +101,3 @@ type ProcessManipulation interface {
 	// after a call to RequestManualStop.
 	CheckAndClearManualStopRequest() bool
 }
-
-// BreakpointManipulation is an interface for managing breakpoints.
-type BreakpointManipulation interface {
-	Breakpoints() *BreakpointMap
-	SetBreakpoint(addr uint64, kind BreakpointKind, cond ast.Expr) (*Breakpoint, error)
-	ClearBreakpoint(addr uint64) (*Breakpoint, error)
-	ClearInternalBreakpoints() error
-}

pkg/proc/native/proc.go

@@ -1,7 +1,6 @@
 package native
 
 import (
-	"go/ast"
 	"os"
 	"runtime"
 	"sync"
@@ -115,15 +114,6 @@ func (dbp *nativeProcess) Detach(kill bool) (err error) {
 		dbp.bi.Close()
 		return nil
 	}
-	// Clean up any breakpoints we've set.
-	for _, bp := range dbp.breakpoints.M {
-		if bp != nil {
-			_, err := dbp.ClearBreakpoint(bp.Addr)
-			if err != nil {
-				return err
-			}
-		}
-	}
 	dbp.execPtraceFunc(func() {
 		err = dbp.detach(kill)
 		if err != nil {
@@ -215,7 +205,7 @@ func (dbp *nativeProcess) CheckAndClearManualStopRequest() bool {
 	return msr
 }
 
-func (dbp *nativeProcess) writeBreakpoint(addr uint64) (string, int, *proc.Function, []byte, error) {
+func (dbp *nativeProcess) WriteBreakpoint(addr uint64) (string, int, *proc.Function, []byte, error) {
 	f, l, fn := dbp.bi.PCToLine(uint64(addr))
 
 	originalData := make([]byte, dbp.bi.Arch.BreakpointSize())
@@ -230,18 +220,8 @@ func (dbp *nativeProcess) writeBreakpoint(addr uint64) (string, int, *proc.Funct
 	return f, l, fn, originalData, nil
 }
 
-// SetBreakpoint sets a breakpoint at addr, and stores it in the process wide
+func (dbp *nativeProcess) EraseBreakpoint(bp *proc.Breakpoint) error {
-// break point table.
+	return dbp.currentThread.ClearBreakpoint(bp)
-func (dbp *nativeProcess) SetBreakpoint(addr uint64, kind proc.BreakpointKind, cond ast.Expr) (*proc.Breakpoint, error) {
-	return dbp.breakpoints.Set(addr, kind, cond, dbp.writeBreakpoint)
-}
-
-// ClearBreakpoint clears the breakpoint at addr.
-func (dbp *nativeProcess) ClearBreakpoint(addr uint64) (*proc.Breakpoint, error) {
-	if dbp.exited {
-		return nil, &proc.ErrProcessExited{Pid: dbp.Pid()}
-	}
-	return dbp.breakpoints.Clear(addr, dbp.currentThread.ClearBreakpoint)
 }
 
 // ContinueOnce will continue the target until it stops.
@@ -305,27 +285,10 @@ func (dbp *nativeProcess) initialize(path string, debugInfoDirs []string) (*proc
 	return proc.NewTarget(dbp, proc.NewTargetConfig{
 		Path:                path,
 		DebugInfoDirs:       debugInfoDirs,
-		WriteBreakpoint:     dbp.writeBreakpoint,
 		DisableAsyncPreempt: runtime.GOOS == "windows" || runtime.GOOS == "freebsd",
 		StopReason:          stopReason})
 }
 
-// ClearInternalBreakpoints will clear all non-user set breakpoints. These
-// breakpoints are set for internal operations such as 'next'.
-func (dbp *nativeProcess) ClearInternalBreakpoints() error {
-	return dbp.breakpoints.ClearInternalBreakpoints(func(bp *proc.Breakpoint) error {
-		if err := dbp.currentThread.ClearBreakpoint(bp); err != nil {
-			return err
-		}
-		for _, thread := range dbp.threads {
-			if thread.CurrentBreakpoint.Breakpoint == bp {
-				thread.CurrentBreakpoint.Clear()
-			}
-		}
-		return nil
-	})
-}
-
 func (dbp *nativeProcess) handlePtraceFuncs() {
 	// We must ensure here that we are running on the same thread during
 	// while invoking the ptrace(2) syscall. This is due to the fact that ptrace(2) expects

pkg/proc/target.go

@@ -80,11 +80,10 @@ const (
 
 // NewTargetConfig contains the configuration for a new Target object,
 type NewTargetConfig struct {
-	Path                string            // path of the main executable
+	Path                string     // path of the main executable
-	DebugInfoDirs       []string          // Directories to search for split debug info
+	DebugInfoDirs       []string   // Directories to search for split debug info
-	WriteBreakpoint     WriteBreakpointFn // Function to write a breakpoint to the target process
+	DisableAsyncPreempt bool       // Go 1.14 asynchronous preemption should be disabled
-	DisableAsyncPreempt bool              // Go 1.14 asynchronous preemption should be disabled
+	StopReason          StopReason // Initial stop reason
-	StopReason          StopReason        // Initial stop reason
 }
 
 // DisableAsyncPreemptEnv returns a process environment (like os.Environ)
@@ -128,8 +127,8 @@ func NewTarget(p Process, cfg NewTargetConfig) (*Target, error) {
 	g, _ := GetG(p.CurrentThread())
 	t.selectedGoroutine = g
 
-	createUnrecoveredPanicBreakpoint(p, cfg.WriteBreakpoint)
+	t.createUnrecoveredPanicBreakpoint()
-	createFatalThrowBreakpoint(p, cfg.WriteBreakpoint)
+	t.createFatalThrowBreakpoint()
 
 	t.gcache.init(p.BinInfo())
 
@@ -216,8 +215,18 @@ func (p *Target) SwitchThread(tid int) error {
 // we were previously debugging.
 // If kill is true then the process will be killed when we detach.
 func (t *Target) Detach(kill bool) error {
-	if !kill && t.asyncPreemptChanged {
+	if !kill {
-		setAsyncPreemptOff(t, t.asyncPreemptOff)
+		if t.asyncPreemptChanged {
+			setAsyncPreemptOff(t, t.asyncPreemptOff)
+		}
+		for _, bp := range t.Breakpoints().M {
+			if bp != nil {
+				_, err := t.ClearBreakpoint(bp.Addr)
+				if err != nil {
+					return err
+				}
+			}
+		}
 	}
 	t.StopReason = StopUnknown
 	return t.proc.Detach(kill)
@@ -255,13 +264,13 @@ func setAsyncPreemptOff(p *Target, v int64) {
 }
 
 // createUnrecoveredPanicBreakpoint creates the unrecoverable-panic breakpoint.
-func createUnrecoveredPanicBreakpoint(p Process, writeBreakpoint WriteBreakpointFn) {
+func (t *Target) createUnrecoveredPanicBreakpoint() {
-	panicpcs, err := FindFunctionLocation(p, "runtime.startpanic", 0)
+	panicpcs, err := FindFunctionLocation(t.Process, "runtime.startpanic", 0)
 	if _, isFnNotFound := err.(*ErrFunctionNotFound); isFnNotFound {
-		panicpcs, err = FindFunctionLocation(p, "runtime.fatalpanic", 0)
+		panicpcs, err = FindFunctionLocation(t.Process, "runtime.fatalpanic", 0)
 	}
 	if err == nil {
-		bp, err := p.Breakpoints().SetWithID(unrecoveredPanicID, panicpcs[0], writeBreakpoint)
+		bp, err := t.setBreakpointWithID(unrecoveredPanicID, panicpcs[0])
 		if err == nil {
 			bp.Name = UnrecoveredPanic
 			bp.Variables = []string{"runtime.curg._panic.arg"}
@@ -270,10 +279,10 @@ func createUnrecoveredPanicBreakpoint(p Process, writeBreakpoint WriteBreakpoint
 }
 
 // createFatalThrowBreakpoint creates the a breakpoint as runtime.fatalthrow.
-func createFatalThrowBreakpoint(p Process, writeBreakpoint WriteBreakpointFn) {
+func (t *Target) createFatalThrowBreakpoint() {
-	fatalpcs, err := FindFunctionLocation(p, "runtime.fatalthrow", 0)
+	fatalpcs, err := FindFunctionLocation(t.Process, "runtime.fatalthrow", 0)
 	if err == nil {
-		bp, err := p.Breakpoints().SetWithID(fatalThrowID, fatalpcs[0], writeBreakpoint)
+		bp, err := t.setBreakpointWithID(fatalThrowID, fatalpcs[0])
 		if err == nil {
 			bp.Name = FatalThrow
 		}

pkg/proc/target_exec.go

@@ -625,7 +625,7 @@ func next(dbp *Target, stepInto, inlinedStepOut bool) error {
 	return nil
 }
 
-func setStepIntoBreakpoints(dbp Process, text []AsmInstruction, topframe Stackframe, sameGCond ast.Expr) error {
+func setStepIntoBreakpoints(dbp *Target, text []AsmInstruction, topframe Stackframe, sameGCond ast.Expr) error {
 	for _, instr := range text {
 		if instr.Loc.File != topframe.Current.File || instr.Loc.Line != topframe.Current.Line || !instr.IsCall() {
 			continue
@@ -645,7 +645,7 @@ func setStepIntoBreakpoints(dbp Process, text []AsmInstruction, topframe Stackfr
 	return nil
 }
 
-func setStepIntoBreakpointsReverse(dbp Process, text []AsmInstruction, topframe Stackframe, sameGCond ast.Expr) error {
+func setStepIntoBreakpointsReverse(dbp *Target, text []AsmInstruction, topframe Stackframe, sameGCond ast.Expr) error {
 	// Set a breakpoint after every CALL instruction
 	for i, instr := range text {
 		if instr.Loc.File != topframe.Current.File || !instr.IsCall() || instr.DestLoc == nil || instr.DestLoc.Fn == nil {
@@ -708,7 +708,7 @@ func removePCsBetween(pcs []uint64, start, end uint64) []uint64 {
 	return out
 }
 
-func setStepIntoBreakpoint(dbp Process, text []AsmInstruction, cond ast.Expr) error {
+func setStepIntoBreakpoint(dbp *Target, text []AsmInstruction, cond ast.Expr) error {
 	if len(text) <= 0 {
 		return nil
 	}
@@ -873,7 +873,7 @@ func skipAutogeneratedWrappersOut(g *G, thread Thread, startTopframe, startRetfr
 
 // setDeferBreakpoint is a helper function used by next and StepOut to set a
 // breakpoint on the first deferred function.
-func setDeferBreakpoint(p Process, text []AsmInstruction, topframe Stackframe, sameGCond ast.Expr, stepInto bool) (uint64, error) {
+func setDeferBreakpoint(p *Target, text []AsmInstruction, topframe Stackframe, sameGCond ast.Expr, stepInto bool) (uint64, error) {
 	// Set breakpoint on the most recently deferred function (if any)
 	var deferpc uint64
 	if topframe.TopmostDefer != nil && topframe.TopmostDefer.DeferredPC != 0 {

service/debugger/debugger.go

@@ -581,7 +581,7 @@ func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoin
 
 // createLogicalBreakpoint creates one physical breakpoint for each address
 // in addrs and associates all of them with the same logical breakpoint.
-func createLogicalBreakpoint(p proc.Process, addrs []uint64, requestedBp *api.Breakpoint) (*api.Breakpoint, error) {
+func createLogicalBreakpoint(p *proc.Target, addrs []uint64, requestedBp *api.Breakpoint) (*api.Breakpoint, error) {
 	bps := make([]*proc.Breakpoint, len(addrs))
 	var err error
 	for i := range addrs {