cmd/dlv: Improve dlv trace output

This patch improves the `dlv trace` subcommand output by reducing the
noise that is generated and providing clearer more concise information.

Also adds new tests closing a gap in our testing (we previously never
really tested this subcommand).

Fixes go-delve#2027

cmd/dlv/cmds/commands.go

@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"strconv"
+	"strings"
 	"syscall"
 
 	"github.com/go-delve/delve/pkg/config"
@@ -536,7 +537,7 @@ func traceCmd(cmd *cobra.Command, args []string) {
 				Stacktrace:   traceStackDepth,
 				LoadArgs:     &terminal.ShortLoadConfig,
 			})
-			if err != nil {
+			if err != nil && !isBreakpointExistsErr(err) {
 				fmt.Fprintln(os.Stderr, err)
 				return 1
 			}
@@ -549,10 +550,11 @@ func traceCmd(cmd *cobra.Command, args []string) {
 				_, err = client.CreateBreakpoint(&api.Breakpoint{
 					Addr:        addrs[i],
 					TraceReturn: true,
+					Stacktrace:  traceStackDepth,
 					Line:        -1,
 					LoadArgs:    &terminal.ShortLoadConfig,
 				})
-				if err != nil {
+				if err != nil && !isBreakpointExistsErr(err) {
 					fmt.Fprintln(os.Stderr, err)
 					return 1
 				}
@@ -561,16 +563,16 @@ func traceCmd(cmd *cobra.Command, args []string) {
 		cmds := terminal.DebugCommands(client)
 		t := terminal.New(client, nil)
 		defer t.Close()
-		err = cmds.Call("continue", t)
-		if err != nil {
-			fmt.Fprintln(os.Stderr, err)
-			return 1
-		}
+		cmds.Call("continue", t)
 		return 0
 	}()
 	os.Exit(status)
 }
 
+func isBreakpointExistsErr(err error) bool {
+	return strings.Contains(err.Error(), "Breakpoint exists")
+}
+
 func testCmd(cmd *cobra.Command, args []string) {
 	status := func() int {
 		debugname, err := filepath.Abs(cmd.Flag("output").Value.String())

cmd/dlv/dlv_test.go

@@ -23,7 +23,6 @@ import (
 	"github.com/go-delve/delve/pkg/terminal"
 	"github.com/go-delve/delve/service/dap/daptest"
 	"github.com/go-delve/delve/service/rpc2"
-
 	"golang.org/x/tools/go/packages"
 )
 
@@ -556,3 +555,82 @@ func TestDap(t *testing.T) {
 	client.Close()
 	cmd.Wait()
 }
+
+func TestTrace(t *testing.T) {
+	dlvbin, tmpdir := getDlvBin(t)
+	defer os.RemoveAll(tmpdir)
+
+	expected := "> goroutine(1): main.foo(99, 9801) => (9900)\n"
+
+	fixtures := protest.FindFixturesDir()
+	cmd := exec.Command(dlvbin, "trace", filepath.Join(fixtures, "issue573.go"), "foo")
+	rdr, err := cmd.StderrPipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = cmd.Start()
+	if err != nil {
+		t.Fatalf("error running trace: %v", err)
+	}
+	output, err := ioutil.ReadAll(rdr)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(output) != expected {
+		t.Fatalf("expected:\n%s\ngot:\n%s", expected, string(output))
+	}
+	cmd.Wait()
+}
+
+func TestTraceBreakpointExists(t *testing.T) {
+	dlvbin, tmpdir := getDlvBin(t)
+	defer os.RemoveAll(tmpdir)
+
+	fixtures := protest.FindFixturesDir()
+	// We always set breakpoints on some runtime functions at startup, so this would return with
+	// a breakpoints exists error.
+	// TODO: Perhaps we shouldn't be setting these default breakpoints in trace mode, however.
+	cmd := exec.Command(dlvbin, "trace", filepath.Join(fixtures, "issue573.go"), "runtime.*")
+	rdr, err := cmd.StderrPipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = cmd.Start()
+	if err != nil {
+		t.Fatalf("error running trace: %v", err)
+	}
+	defer cmd.Wait()
+
+	output, err := ioutil.ReadAll(rdr)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if bytes.Contains(output, []byte("Breakpoint exists")) {
+		t.Fatal("Breakpoint exists errors should be ignored")
+	}
+}
+
+func TestTracePrintStack(t *testing.T) {
+	dlvbin, tmpdir := getDlvBin(t)
+	defer os.RemoveAll(tmpdir)
+
+	fixtures := protest.FindFixturesDir()
+	cmd := exec.Command(dlvbin, "trace", "--stack", "2", filepath.Join(fixtures, "issue573.go"), "foo")
+	rdr, err := cmd.StderrPipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	err = cmd.Start()
+	if err != nil {
+		t.Fatalf("error running trace: %v", err)
+	}
+	defer cmd.Wait()
+
+	output, err := ioutil.ReadAll(rdr)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !bytes.Contains(output, []byte("Stack:")) && !bytes.Contains(output, []byte("main.main")) {
+		t.Fatal("stacktrace not printed")
+	}
+}

pkg/terminal/command.go

@@ -2124,6 +2124,7 @@ func printcontextThread(t *Term, th *api.Thread) {
 	}
 
 	args := ""
+	var hasReturnValue bool
 	if th.BreakpointInfo != nil && th.Breakpoint.LoadArgs != nil && *th.Breakpoint.LoadArgs == ShortLoadConfig {
 		var arg []string
 		for _, ar := range th.BreakpointInfo.Arguments {
@@ -2135,6 +2136,9 @@ func printcontextThread(t *Term, th *api.Thread) {
 			if (ar.Flags & api.VariableArgument) != 0 {
 				arg = append(arg, ar.SinglelineString())
 			}
+			if (ar.Flags & api.VariableReturnArgument) != 0 {
+				hasReturnValue = true
+			}
 		}
 		args = strings.Join(arg, ", ")
 	}
@@ -2144,6 +2148,11 @@ func printcontextThread(t *Term, th *api.Thread) {
 		bpname = fmt.Sprintf("[%s] ", th.Breakpoint.Name)
 	}
 
+	if th.Breakpoint.Tracepoint || th.Breakpoint.TraceReturn {
+		printTracepoint(th, bpname, fn, args, hasReturnValue)
+		return
+	}
+
 	if hitCount, ok := th.Breakpoint.HitCount[strconv.Itoa(th.GoroutineID)]; ok {
 		fmt.Printf("> %s%s(%s) %s:%d (hits goroutine(%d):%d total:%d) (PC: %#v)\n",
 			bpname,
@@ -2204,6 +2213,28 @@ func printcontextThread(t *Term, th *api.Thread) {
 	}
 }
 
+func printTracepoint(th *api.Thread, bpname string, fn *api.Function, args string, hasReturnValue bool) {
+	if th.Breakpoint.Tracepoint {
+		fmt.Fprintf(os.Stderr, "> goroutine(%d): %s%s(%s)", th.GoroutineID, bpname, fn.Name(), args)
+		if !hasReturnValue {
+			fmt.Println()
+		}
+	}
+	if th.Breakpoint.TraceReturn {
+		retVals := make([]string, 0, len(th.ReturnValues))
+		for _, v := range th.ReturnValues {
+			retVals = append(retVals, v.SinglelineString())
+		}
+		fmt.Fprintf(os.Stderr, " => (%s)\n", strings.Join(retVals, ","))
+	}
+	if th.Breakpoint.TraceReturn || !hasReturnValue {
+		if th.BreakpointInfo.Stacktrace != nil {
+			fmt.Fprintf(os.Stderr, "\tStack:\n")
+			printStack(th.BreakpointInfo.Stacktrace, "\t\t", false)
+		}
+	}
+}
+
 func printfile(t *Term, filename string, line int, showArrow bool) error {
 	if filename == "" {
 		return nil

service/config.go

@@ -36,8 +36,4 @@ type Config struct {
 
 	// DisconnectChan will be closed by the server when the client disconnects
 	DisconnectChan chan<- struct{}
-
-	// TTY is passed along to the target process on creation. Used to specify a
-	// TTY for that process.
-	TTY string
 }

service/debugger/debugger.go

@@ -1105,7 +1105,7 @@ func regexFilterFuncs(filter string, allFuncs []proc.Function) ([]string, error)
 
 	funcs := []string{}
 	for _, f := range allFuncs {
-		if regex.Match([]byte(f.Name)) {
+		if regex.MatchString(f.Name) {
 			funcs = append(funcs, f.Name)
 		}
 	}