*: add option to re-record recorded targets (#1702)

Adds a '-r' option to the 'restart' command (and to the Restart API)
that re-records the target when using rr.

Also moves the code to delete the trace directory inside the gdbserial
package.

Documentation/cli/starlark.md

@@ -49,7 +49,7 @@ threads() | Equivalent to API call [ListThreads](https://godoc.org/github.com/go
 types(Filter) | Equivalent to API call [ListTypes](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.ListTypes)
 process_pid() | Equivalent to API call [ProcessPid](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.ProcessPid)
 recorded() | Equivalent to API call [Recorded](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.Recorded)
-restart(Position, ResetArgs, NewArgs) | Equivalent to API call [Restart](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.Restart)
+restart(Position, ResetArgs, NewArgs, Rerecord) | Equivalent to API call [Restart](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.Restart)
 set_expr(Scope, Symbol, Value) | Equivalent to API call [Set](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.Set)
 stacktrace(Id, Depth, Full, Defers, Opts, Cfg) | Equivalent to API call [Stacktrace](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.Stacktrace)
 state(NonBlocking) | Equivalent to API call [State](https://godoc.org/github.com/go-delve/delve/service/rpc2#RPCServer.State)

_fixtures/testrerecord.go

+package main
+
+import (
+	"fmt"
+	"time"
+)
+
+func main() {
+	t := time.Now().Unix()
+	fmt.Println(t)
+}

cmd/dlv/cmds/commands.go

@@ -560,13 +560,6 @@ func connect(addr string, clientConn net.Conn, conf *config.Config, kind execute
 			}
 		}
 	}
-	if client.Recorded() && (kind == executingGeneratedFile || kind == executingGeneratedTest) {
-		// When using the rr backend remove the trace directory if we built the
-		// executable
-		if tracedir, err := client.TraceDirectory(); err == nil {
-			defer SafeRemoveAll(tracedir)
-		}
-	}
 	term := terminal.New(client, conf)
 	term.InitFile = InitFile
 	status, err := term.Run()
@@ -741,28 +734,3 @@ func gocommand(command string, args ...string) error {
 	goBuild.Stderr = os.Stderr
 	return goBuild.Run()
 }
-
-// SafeRemoveAll removes dir and its contents but only as long as dir does
-// not contain directories.
-func SafeRemoveAll(dir string) {
-	dh, err := os.Open(dir)
-	if err != nil {
-		return
-	}
-	defer dh.Close()
-	fis, err := dh.Readdir(-1)
-	if err != nil {
-		return
-	}
-	for _, fi := range fis {
-		if fi.IsDir() {
-			return
-		}
-	}
-	for _, fi := range fis {
-		if err := os.Remove(filepath.Join(dir, fi.Name())); err != nil {
-			return
-		}
-	}
-	os.Remove(dir)
-}

cmd/dlv/dlv_test.go

@@ -18,7 +18,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/go-delve/delve/cmd/dlv/cmds"
 	protest "github.com/go-delve/delve/pkg/proc/test"
 	"github.com/go-delve/delve/pkg/terminal"
 	"github.com/go-delve/delve/service/rpc2"
@@ -284,7 +283,7 @@ func TestGeneratedDoc(t *testing.T) {
 	// Checks gen-usage-docs.go
 	tempDir, err := ioutil.TempDir(os.TempDir(), "test-gen-doc")
 	assertNoError(err, t, "TempDir")
-	defer cmds.SafeRemoveAll(tempDir)
+	defer protest.SafeRemoveAll(tempDir)
 	cmd := exec.Command("go", "run", "scripts/gen-usage-docs.go", tempDir)
 	cmd.Dir = projectRoot()
 	cmd.Run()

pkg/proc/gdbserial/gdbserver.go

@@ -124,6 +124,8 @@ type Process struct {
 	process  *os.Process
 	waitChan chan *os.ProcessState
 
+	onDetach func() // called after a successful detach
+
 	common proc.CommonProcess
 }
 
@@ -866,6 +868,9 @@ func (p *Process) Detach(kill bool) error {
 		p.process = nil
 	}
 	p.detached = true
+	if p.onDetach != nil {
+		p.onDetach()
+	}
 	return p.bi.Close()
 }
 

pkg/proc/gdbserial/rr.go

@@ -8,6 +8,7 @@ import (
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"unicode"
@@ -53,7 +54,7 @@ func Record(cmd []string, wd string, quiet bool) (tracedir string, err error) {
 
 // Replay starts an instance of rr in replay mode, with the specified trace
 // directory, and connects to it.
-func Replay(tracedir string, quiet bool, debugInfoDirs []string) (*Process, error) {
+func Replay(tracedir string, quiet, deleteOnDetach bool, debugInfoDirs []string) (*Process, error) {
 	if err := checkRRAvailabe(); err != nil {
 		return nil, err
 	}
@@ -82,6 +83,11 @@ func Replay(tracedir string, quiet bool, debugInfoDirs []string) (*Process, erro
 
 	p := New(rrcmd.Process)
 	p.tracedir = tracedir
+	if deleteOnDetach {
+		p.onDetach = func() {
+			safeRemoveAll(p.tracedir)
+		}
+	}
 	err = p.Dial(init.port, init.exe, 0, debugInfoDirs)
 	if err != nil {
 		rrcmd.Process.Kill()
@@ -262,6 +268,31 @@ func RecordAndReplay(cmd []string, wd string, quiet bool, debugInfoDirs []string
 	if tracedir == "" {
 		return nil, "", err
 	}
-	p, err = Replay(tracedir, quiet, debugInfoDirs)
+	p, err = Replay(tracedir, quiet, true, debugInfoDirs)
 	return p, tracedir, err
 }
+
+// safeRemoveAll removes dir and its contents but only as long as dir does
+// not contain directories.
+func safeRemoveAll(dir string) {
+	dh, err := os.Open(dir)
+	if err != nil {
+		return
+	}
+	defer dh.Close()
+	fis, err := dh.Readdir(-1)
+	if err != nil {
+		return
+	}
+	for _, fi := range fis {
+		if fi.IsDir() {
+			return
+		}
+	}
+	for _, fi := range fis {
+		if err := os.Remove(filepath.Join(dir, fi.Name())); err != nil {
+			return
+		}
+	}
+	os.Remove(dir)
+}

pkg/proc/gdbserial/rr_test.go

@@ -38,9 +38,6 @@ func withTestRecording(name string, t testing.TB, fn func(p *gdbserial.Process,
 
 	defer func() {
 		p.Detach(true)
-		if tracedir != "" {
-			protest.SafeRemoveAll(tracedir)
-		}
 	}()
 
 	fn(p, fixture)

pkg/proc/proc_test.go

@@ -84,9 +84,6 @@ func withTestProcessArgs(name string, t testing.TB, wd string, args []string, bu
 
 	defer func() {
 		p.Detach(true)
-		if tracedir != "" {
-			protest.SafeRemoveAll(tracedir)
-		}
 	}()
 
 	fn(p, fixture)

pkg/terminal/command.go

@@ -128,12 +128,18 @@ A tracepoint is a breakpoint that does not stop the execution of the program, in
 See also: "help on", "help cond" and "help clear"`},
 		{aliases: []string{"restart", "r"}, cmdFn: restart, helpMsg: `Restart process.
 
-	restart [checkpoint]
-	restart [-noargs] newargv...
+For recorded targets the command takes the following forms:
 
-For recorded processes restarts from the start or from the specified
-checkpoint.  For normal processes restarts the process, optionally changing
-the arguments.  With -noargs, the process starts with an empty commandline.
+	restart				resets ot the start of the recording
+	restart [checkpoint]		resets the recording to the given checkpoint
+	restart -r [newargv...]		re-records the target process
+	
+For live targets the command takes the following forms:
+
+	restart [newargv...]		restarts the process
+
+If newargv is omitted the process is restarted (or re-recorded) with the same argument vector.
+If -noargs is specified instead, the argument vector is cleared.
 `},
 		{aliases: []string{"continue", "c"}, cmdFn: c.cont, helpMsg: "Run until breakpoint or program termination."},
 		{aliases: []string{"step", "s"}, cmdFn: c.step, helpMsg: "Single step through program."},
@@ -856,21 +862,51 @@ func writeGoroutineLong(w io.Writer, g *api.Goroutine, prefix string) {
 		prefix, formatLocation(g.StartLoc))
 }
 
-func parseArgs(args string) ([]string, error) {
-	if args == "" {
-		return nil, nil
+func restart(t *Term, ctx callContext, args string) error {
+	if t.client.Recorded() {
+		return restartRecorded(t, ctx, args)
 	}
-	v, err := argv.Argv([]rune(args), argv.ParseEnv(os.Environ()),
-		func(s []rune, _ map[string]string) ([]rune, error) {
-			return nil, fmt.Errorf("Backtick not supported in '%s'", string(s))
-		})
-	if err != nil {
-		return nil, err
+
+	return restartLive(t, ctx, args)
+}
+
+func restartRecorded(t *Term, ctx callContext, args string) error {
+	v := strings.SplitN(args, " ", 2)
+
+	rerecord := false
+	resetArgs := false
+	newArgv := []string{}
+	restartPos := ""
+
+	if len(v) > 0 {
+		if v[0] == "-r" {
+			rerecord = true
+			if len(v) == 2 {
+				var err error
+				resetArgs, newArgv, err = parseNewArgv(v[1])
+				if err != nil {
+					return err
+				}
+			}
+		} else {
+			if len(v) > 1 {
+				return fmt.Errorf("too many arguments to restart")
+			}
+			restartPos = v[0]
+		}
 	}
-	if len(v) != 1 {
-		return nil, fmt.Errorf("Illegal commandline '%s'", args)
+
+	if err := restartIntl(t, rerecord, restartPos, resetArgs, newArgv); err != nil {
+		return err
 	}
-	return v[0], nil
+
+	state, err := t.client.GetState()
+	if err != nil {
+		return err
+	}
+	printcontext(t, state)
+	printfile(t, state.CurrentThread.File, state.CurrentThread.Line, true)
+	return nil
 }
 
 // parseOptionalCount parses an optional count argument.
@@ -882,49 +918,56 @@ func parseOptionalCount(arg string) (int64, error) {
 	return strconv.ParseInt(arg, 0, 64)
 }
 
-func restart(t *Term, ctx callContext, args string) error {
-	v, err := parseArgs(args)
+func restartLive(t *Term, ctx callContext, args string) error {
+	resetArgs, newArgv, err := parseNewArgv(args)
 	if err != nil {
 		return err
 	}
-	var restartPos string
-	var resetArgs bool
-	if t.client.Recorded() {
-		if len(v) > 1 {
-			return fmt.Errorf("restart: illegal position '%v'", v)
-		}
-		if len(v) == 1 {
-			restartPos = v[0]
-			v = nil
-		}
-	} else if len(v) > 0 {
-		resetArgs = true
-		if v[0] == "-noargs" {
-			if len(v) > 1 {
-				return fmt.Errorf("restart: -noargs does not take any arg")
-			}
-			v = nil
-		}
+
+	if err := restartIntl(t, false, "", resetArgs, newArgv); err != nil {
+		return err
 	}
-	discarded, err := t.client.RestartFrom(restartPos, resetArgs, v)
+
+	fmt.Println("Process restarted with PID", t.client.ProcessPid())
+	return nil
+}
+
+func restartIntl(t *Term, rerecord bool, restartPos string, resetArgs bool, newArgv []string) error {
+	discarded, err := t.client.RestartFrom(rerecord, restartPos, resetArgs, newArgv)
 	if err != nil {
 		return err
 	}
-	if !t.client.Recorded() {
-		fmt.Println("Process restarted with PID", t.client.ProcessPid())
-	}
 	for i := range discarded {
 		fmt.Printf("Discarded %s at %s: %v\n", formatBreakpointName(discarded[i].Breakpoint, false), formatBreakpointLocation(discarded[i].Breakpoint), discarded[i].Reason)
 	}
-	if t.client.Recorded() {
-		state, err := t.client.GetState()
-		if err != nil {
-			return err
+	return nil
+}
+
+func parseNewArgv(args string) (resetArgs bool, newArgv []string, err error) {
+	if args == "" {
+		return false, nil, nil
+	}
+	v, err := argv.Argv([]rune(args), argv.ParseEnv(os.Environ()),
+		func(s []rune, _ map[string]string) ([]rune, error) {
+			return nil, fmt.Errorf("Backtick not supported in '%s'", string(s))
+		})
+	if err != nil {
+		return false, nil, err
+	}
+	if len(v) != 1 {
+		return false, nil, fmt.Errorf("Illegal commandline '%s'", args)
+	}
+	w := v[0]
+	if len(w) == 0 {
+		return false, nil, nil
+	}
+	if w[0] == "-noargs" {
+		if len(w) > 1 {
+			return false, nil, fmt.Errorf("Too many arguments to restart")
 		}
-		printcontext(t, state)
-		printfile(t, state.CurrentThread.File, state.CurrentThread.Line, true)
+		return true, nil, nil
 	}
-	return nil
+	return true, w, nil
 }
 
 func printcontextNoState(t *Term) {

pkg/terminal/command_test.go

@@ -161,11 +161,7 @@ func withTestTerminalBuildFlags(name string, t testing.TB, buildFlags test.Build
 	}
 	client := rpc2.NewClient(listener.Addr().String())
 	defer func() {
-		dir, _ := client.TraceDirectory()
 		client.Detach(true)
-		if dir != "" {
-			test.SafeRemoveAll(dir)
-		}
 	}()
 
 	ft := &FakeTerminal{

pkg/terminal/starbind/starlark_mapping.go

@@ -998,6 +998,12 @@ func (env *Env) starlarkPredeclare() starlark.StringDict {
 				return starlark.None, decorateError(thread, err)
 			}
 		}
+		if len(args) > 3 && args[3] != starlark.None {
+			err := unmarshalStarlarkValue(args[3], &rpcArgs.Rerecord, "Rerecord")
+			if err != nil {
+				return starlark.None, decorateError(thread, err)
+			}
+		}
 		for _, kv := range kwargs {
 			var err error
 			switch kv[0].(starlark.String) {
@@ -1007,6 +1013,8 @@ func (env *Env) starlarkPredeclare() starlark.StringDict {
 				err = unmarshalStarlarkValue(kv[1], &rpcArgs.ResetArgs, "ResetArgs")
 			case "NewArgs":
 				err = unmarshalStarlarkValue(kv[1], &rpcArgs.NewArgs, "NewArgs")
+			case "Rerecord":
+				err = unmarshalStarlarkValue(kv[1], &rpcArgs.Rerecord, "Rerecord")
 			default:
 				err = fmt.Errorf("unknown argument %q", kv[0])
 			}

service/client.go

@@ -21,7 +21,7 @@ type Client interface {
 	// Restarts program.
 	Restart() ([]api.DiscardedBreakpoint, error)
 	// Restarts program from the specified position.
-	RestartFrom(pos string, resetArgs bool, newArgs []string) ([]api.DiscardedBreakpoint, error)
+	RestartFrom(rerecord bool, pos string, resetArgs bool, newArgs []string) ([]api.DiscardedBreakpoint, error)
 
 	// GetState returns the current debugger state.
 	GetState() (*api.DebuggerState, error)

service/debugger/debugger.go

@@ -106,7 +106,7 @@ func New(config *Config, processArgs []string) (*Debugger, error) {
 		switch d.config.Backend {
 		case "rr":
 			d.log.Infof("opening trace %s", d.config.CoreFile)
-			p, err = gdbserial.Replay(d.config.CoreFile, false, d.config.DebugInfoDirectories)
+			p, err = gdbserial.Replay(d.config.CoreFile, false, false, d.config.DebugInfoDirectories)
 		default:
 			d.log.Infof("opening core file %s (executable %s)", d.config.CoreFile, d.processArgs[0])
 			p, err = core.OpenCore(d.config.CoreFile, d.processArgs[0], d.config.DebugInfoDirectories)
@@ -140,6 +140,18 @@ func New(config *Config, processArgs []string) (*Debugger, error) {
 	return d, nil
 }
 
+// canRestart returns true if the target was started with Launch and can be restarted
+func (d *Debugger) canRestart() bool {
+	switch {
+	case d.config.AttachPid > 0:
+		return false
+	case d.config.CoreFile != "":
+		return false
+	default:
+		return true
+	}
+}
+
 func (d *Debugger) checkGoVersion() error {
 	if !d.config.CheckGoVersion {
 		return nil
@@ -273,16 +285,19 @@ func (d *Debugger) detach(kill bool) error {
 	return d.target.Detach(kill)
 }
 
+var ErrCanNotRestart = errors.New("can not restart this target")
+
 // Restart will restart the target process, first killing
 // and then exec'ing it again.
 // If the target process is a recording it will restart it from the given
 // position. If pos starts with 'c' it's a checkpoint ID, otherwise it's an
 // event number. If resetArgs is true, newArgs will replace the process args.
-func (d *Debugger) Restart(pos string, resetArgs bool, newArgs []string) ([]api.DiscardedBreakpoint, error) {
+func (d *Debugger) Restart(rerecord bool, pos string, resetArgs bool, newArgs []string) ([]api.DiscardedBreakpoint, error) {
 	d.processMutex.Lock()
 	defer d.processMutex.Unlock()
 
-	if recorded, _ := d.target.Recorded(); recorded {
+	recorded, _ := d.target.Recorded()
+	if recorded && !rerecord {
 		return nil, d.target.Restart(pos)
 	}
 
@@ -290,7 +305,11 @@ func (d *Debugger) Restart(pos string, resetArgs bool, newArgs []string) ([]api.
 		return nil, proc.ErrNotRecorded
 	}
 
-	if valid, _ := d.target.Valid(); valid {
+	if !d.canRestart() {
+		return nil, ErrCanNotRestart
+	}
+
+	if valid, _ := d.target.Valid(); valid && !recorded {
 		// Ensure the process is in a PTRACE_STOP.
 		if err := stopProcess(d.ProcessPid()); err != nil {
 			return nil, err

service/rpc1/server.go

@@ -40,7 +40,7 @@ func (s *RPCServer) Restart(arg1 interface{}, arg2 *int) error {
 	if s.config.AttachPid != 0 {
 		return errors.New("cannot restart process Delve did not create")
 	}
-	_, err := s.debugger.Restart("", false, nil)
+	_, err := s.debugger.Restart(false, "", false, nil)
 	return err
 }
 

service/rpc2/client.go

@@ -62,13 +62,13 @@ func (c *RPCClient) Detach(kill bool) error {
 
 func (c *RPCClient) Restart() ([]api.DiscardedBreakpoint, error) {
 	out := new(RestartOut)
-	err := c.call("Restart", RestartIn{"", false, nil}, out)
+	err := c.call("Restart", RestartIn{"", false, nil, false}, out)
 	return out.DiscardedBreakpoints, err
 }
 
-func (c *RPCClient) RestartFrom(pos string, resetArgs bool, newArgs []string) ([]api.DiscardedBreakpoint, error) {
+func (c *RPCClient) RestartFrom(rerecord bool, pos string, resetArgs bool, newArgs []string) ([]api.DiscardedBreakpoint, error) {
 	out := new(RestartOut)
-	err := c.call("Restart", RestartIn{pos, resetArgs, newArgs}, out)
+	err := c.call("Restart", RestartIn{pos, resetArgs, newArgs, rerecord}, out)
 	return out.DiscardedBreakpoints, err
 }
 

service/rpc2/server.go

@@ -73,6 +73,9 @@ type RestartIn struct {
 	// NewArgs are arguments to launch a new process.  They replace only the
 	// argv[1] and later. Argv[0] cannot be changed.
 	NewArgs []string
+
+	// When Rerecord is set the target will be rerecorded
+	Rerecord bool
 }
 
 type RestartOut struct {
@@ -85,7 +88,7 @@ func (s *RPCServer) Restart(arg RestartIn, out *RestartOut) error {
 		return errors.New("cannot restart process Delve did not create")
 	}
 	var err error
-	out.DiscardedBreakpoints, err = s.debugger.Restart(arg.Position, arg.ResetArgs, arg.NewArgs)
+	out.DiscardedBreakpoints, err = s.debugger.Restart(arg.Rerecord, arg.Position, arg.ResetArgs, arg.NewArgs)
 	return err
 }
 

service/test/integration2_test.go

@@ -77,11 +77,7 @@ func withTestClient2Extended(name string, t *testing.T, fn func(c service.Client
 	clientConn, fixture := startServer(name, t)
 	client := rpc2.NewClientFromConn(clientConn)
 	defer func() {
-		dir, _ := client.TraceDirectory()
 		client.Detach(true)
-		if dir != "" {
-			protest.SafeRemoveAll(dir)
-		}
 	}()
 
 	fn(client, fixture)
@@ -1147,17 +1143,22 @@ func TestSkipPrologue2(t *testing.T) {
 func TestIssue419(t *testing.T) {
 	// Calling service/rpc.(*Client).Halt could cause a crash because both Halt and Continue simultaneously
 	// try to read 'runtime.g' and debug/dwarf.Data.Type is not thread safe
+	finish := make(chan struct{})
 	withTestClient2("issue419", t, func(c service.Client) {
 		go func() {
+			defer close(finish)
 			rand.Seed(time.Now().Unix())
 			d := time.Duration(rand.Intn(4) + 1)
 			time.Sleep(d * time.Second)
+			t.Logf("halt")
 			_, err := c.Halt()
 			assertNoError(err, t, "RequestManualStop()")
 		}()
 		statech := c.Continue()
 		state := <-statech
 		assertNoError(state.Err, t, "Continue()")
+		t.Logf("done")
+		<-finish
 	})
 }
 
@@ -1730,3 +1731,54 @@ func TestIssue1703(t *testing.T) {
 		t.Logf("text: %#v\n", text)
 	})
 }
+
+func TestRerecord(t *testing.T) {
+	protest.AllowRecording(t)
+	if testBackend != "rr" {
+		t.Skip("only valid for recorded targets")
+	}
+	withTestClient2("testrerecord", t, func(c service.Client) {
+		fp := testProgPath(t, "testrerecord")
+		_, err := c.CreateBreakpoint(&api.Breakpoint{File: fp, Line: 10})
+		assertNoError(err, t, "CreateBreakpoin")
+
+		gett := func() int {
+			state := <-c.Continue()
+			if state.Err != nil {
+				t.Fatalf("Unexpected error: %v, state: %#v", state.Err, state)
+			}
+
+			vart, err := c.EvalVariable(api.EvalScope{-1, 0, 0}, "t", normalLoadConfig)
+			assertNoError(err, t, "EvalVariable")
+			if vart.Unreadable != "" {
+				t.Fatalf("Could not read variable 't': %s\n", vart.Unreadable)
+			}
+
+			t.Logf("Value of t is %s\n", vart.Value)
+
+			vartval, err := strconv.Atoi(vart.Value)
+			assertNoError(err, t, "Parsing value of variable t")
+			return vartval
+		}
+
+		t0 := gett()
+
+		_, err = c.RestartFrom(false, "", false, nil)
+		assertNoError(err, t, "First restart")
+		t1 := gett()
+
+		if t0 != t1 {
+			t.Fatalf("Expected same value for t after restarting (without rerecording) %d %d", t0, t1)
+		}
+
+		time.Sleep(2 * time.Second) // make sure that we're not running inside the same second
+
+		_, err = c.RestartFrom(true, "", false, nil)
+		assertNoError(err, t, "Second restart")
+		t2 := gett()
+
+		if t0 == t2 {
+			t.Fatalf("Expected new value for t after restarting (with rerecording) %d %d", t0, t2)
+		}
+	})
+}

service/test/variables_test.go

@@ -138,9 +138,6 @@ func withTestProcessArgs(name string, t *testing.T, wd string, args []string, bu
 
 	defer func() {
 		p.Detach(true)
-		if tracedir != "" {
-			protest.SafeRemoveAll(tracedir)
-		}
 	}()
 
 	fn(p, fixture)