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

pkg/proc/breakpoints.go

@@ -237,21 +237,13 @@ func NewBreakpointMap() BreakpointMap {
 	}
 }
 
-// ResetBreakpointIDCounter resets the breakpoint ID counter of bpmap.
-func (bpmap *BreakpointMap) ResetBreakpointIDCounter() {
-	bpmap.breakpointIDCounter = 0
-}
-
-// WriteBreakpointFn is a type that represents a function to be used for
-// writting breakpoings into the target.
-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) {
+// SetBreakpoint sets a breakpoint at addr, and stores it in the process wide
+// break point table.
+func (t *Target) SetBreakpoint(addr uint64, kind BreakpointKind, cond ast.Expr) (*Breakpoint, error) {
+	if valid, err := t.Valid(); !valid {
+		return nil, err
+	}
+	bpmap := t.Breakpoints()
 	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,19 +295,23 @@ 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.
-func (bpmap *BreakpointMap) SetWithID(id int, addr uint64, writeBreakpoint WriteBreakpointFn) (*Breakpoint, error) {
-	bp, err := bpmap.Set(addr, UserBreakpoint, nil, writeBreakpoint)
+// setBreakpointWithID creates a breakpoint at addr, with the specified logical ID.
+func (t *Target) setBreakpointWithID(id int, addr uint64) (*Breakpoint, error) {
+	bpmap := t.Breakpoints()
+	bp, err := t.SetBreakpoint(addr, UserBreakpoint, nil)
 	if err == nil {
 		bp.LogicalID = id
 		bpmap.breakpointIDCounter--
 	}
 	return bp, err
 }
 
-// Clear clears the breakpoint at addr.
-// Do not call this function call proc.Process.ClearBreakpoint instead.
-func (bpmap *BreakpointMap) Clear(addr uint64, clearBreakpoint clearBreakpointFn) (*Breakpoint, error) {
+// ClearBreakpoint clears the breakpoint at addr.
+func (t *Target) ClearBreakpoint(addr uint64) (*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,19 +334,24 @@ 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
-// instead, this function is used to implement that.
-func (bpmap *BreakpointMap) ClearInternalBreakpoints(clearBreakpoint clearBreakpointFn) error {
+func (t *Target) ClearInternalBreakpoints() error {
+	bpmap := t.Breakpoints()
+	threads := t.ThreadList()
 	for addr, bp := range bpmap.M {
 		bp.Kind = bp.Kind & UserBreakpoint
 		bp.internalCond = nil
 		bp.returnInfo = nil
 		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) {
-	return nil, proc.NoBreakpointError{Addr: addr}
+func (p *process) EraseBreakpoint(bp *proc.Breakpoint) error {
+	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) SetBreakpoint(addr uint64, kind proc.BreakpointKind, cond ast.Expr) (*proc.Breakpoint, error) {
-	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
-	})
+func (p *gdbProcess) EraseBreakpoint(bp *proc.Breakpoint) error {
+	return p.conn.clearBreakpoint(bp.Addr)
 }
 
 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
-// break point table.
-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)
+func (dbp *nativeProcess) EraseBreakpoint(bp *proc.Breakpoint) error {
+	return dbp.currentThread.ClearBreakpoint(bp)
 }
 
 // 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