pkg/proc/internal/ebpf: Fix handling of entry / return (#3081)

This patch removes the old error-prone way of tracking
whether the tracepoint is for a function entry or
return. Instead of trying to guess, let the data structure
simply tell us directly.

_fixtures/ebpf_trace.go

@@ -0,0 +1,14 @@
+package main
+
+import "fmt"
+
+func callme(i int) int {
+	if i == 0 {
+		return 100
+	}
+	return callme(i - 1)
+}
+
+func main() {
+	fmt.Println(callme(10))
+}

cmd/dlv/cmds/commands.go

@@ -691,7 +691,6 @@ func traceCmd(cmd *cobra.Command, args []string) {
 			done := make(chan struct{})
 			defer close(done)
 			go func() {
-				gFnEntrySeen := map[int]struct{}{}
 				for {
 					select {
 					case <-done:
@@ -713,14 +712,11 @@ func traceCmd(cmd *cobra.Command, args []string) {
 									params.WriteString(p.Value)
 								}
 							}
-							_, seen := gFnEntrySeen[t.GoroutineID]
-							if seen {
+							if t.IsRet {
 								for _, p := range t.ReturnParams {
 									fmt.Fprintf(os.Stderr, "=> %#v\n", p.Value)
 								}
-								delete(gFnEntrySeen, t.GoroutineID)
 							} else {
-								gFnEntrySeen[t.GoroutineID] = struct{}{}
 								fmt.Fprintf(os.Stderr, "> (%d) %s(%s)\n", t.GoroutineID, t.FunctionName, params.String())
 							}
 						}

cmd/dlv/dlv_test.go

@@ -1146,6 +1146,67 @@ func TestTraceEBPF(t *testing.T) {
 	cmd.Wait()
 }
 
+func TestTraceEBPF2(t *testing.T) {
+	if os.Getenv("CI") == "true" {
+		t.Skip("cannot run test in CI, requires kernel compiled with btf support")
+	}
+	if runtime.GOOS != "linux" || runtime.GOARCH != "amd64" {
+		t.Skip("not implemented on non linux/amd64 systems")
+	}
+	if !goversion.VersionAfterOrEqual(runtime.Version(), 1, 16) {
+		t.Skip("requires at least Go 1.16 to run test")
+	}
+	usr, err := user.Current()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if usr.Uid != "0" {
+		t.Skip("test must be run as root")
+	}
+
+	dlvbin, tmpdir := getDlvBinEBPF(t)
+	defer os.RemoveAll(tmpdir)
+
+	expected := []byte(`> (1) main.callme(10)
+> (1) main.callme(9)
+> (1) main.callme(8)
+> (1) main.callme(7)
+> (1) main.callme(6)
+> (1) main.callme(5)
+> (1) main.callme(4)
+> (1) main.callme(3)
+> (1) main.callme(2)
+> (1) main.callme(1)
+> (1) main.callme(0)
+=> "100"
+=> "100"
+=> "100"
+=> "100"
+=> "100"
+=> "100"
+=> "100"
+=> "100"
+=> "100"
+=> "100"
+=> "100"`)
+
+	fixtures := protest.FindFixturesDir()
+	cmd := exec.Command(dlvbin, "trace", "--ebpf", "--output", filepath.Join(tmpdir, "__debug"), filepath.Join(fixtures, "ebpf_trace.go"), "main.callme")
+	rdr, err := cmd.StderrPipe()
+	assertNoError(err, t, "stderr pipe")
+	defer rdr.Close()
+
+	assertNoError(cmd.Start(), t, "running trace")
+
+	output, err := ioutil.ReadAll(rdr)
+	assertNoError(err, t, "ReadAll")
+
+	if !bytes.Contains(output, expected) {
+		t.Fatalf("expected:\n%s\ngot:\n%s", string(expected), string(output))
+	}
+	cmd.Wait()
+}
+
 func TestDlvTestChdir(t *testing.T) {
 	dlvbin, tmpdir := getDlvBin(t)
 	defer os.RemoveAll(tmpdir)

pkg/proc/internal/ebpf/bpf/trace.bpf.c

@@ -219,6 +219,7 @@ int uprobe__dlv_trace(struct pt_regs *ctx) {
     parsed_args->fn_addr = args->fn_addr;
     parsed_args->n_parameters = args->n_parameters;
     parsed_args->n_ret_parameters = args->n_ret_parameters;
+    parsed_args->is_ret = args->is_ret;
     memcpy(parsed_args->params, args->params, sizeof(args->params));
     memcpy(parsed_args->ret_params, args->ret_params, sizeof(args->ret_params));
 

pkg/proc/internal/ebpf/context.go

@@ -29,6 +29,7 @@ type RawUProbeParam struct {
 type RawUProbeParams struct {
 	FnAddr       int
 	GoroutineID  int
+	IsRet        bool
 	InputParams  []*RawUProbeParam
 	ReturnParams []*RawUProbeParam
 }

pkg/proc/internal/ebpf/helpers.go

@@ -153,6 +153,7 @@ func parseFunctionParameterList(rawParamBytes []byte) RawUProbeParams {
 	var rawParams RawUProbeParams
 	rawParams.FnAddr = int(params.fn_addr)
 	rawParams.GoroutineID = int(params.goroutine_id)
+	rawParams.IsRet = params.is_ret
 
 	parseParam := func(param function_parameter_t) *RawUProbeParam {
 		iparam := &RawUProbeParam{}

pkg/proc/target.go

@@ -436,6 +436,7 @@ func (t *Target) CurrentThread() Thread {
 type UProbeTraceResult struct {
 	FnAddr       int
 	GoroutineID  int
+	IsRet        bool
 	InputParams  []*Variable
 	ReturnParams []*Variable
 }
@@ -465,6 +466,7 @@ func (t *Target) GetBufferedTracepoints() []*UProbeTraceResult {
 		r := &UProbeTraceResult{}
 		r.FnAddr = tp.FnAddr
 		r.GoroutineID = tp.GoroutineID
+		r.IsRet = tp.IsRet
 		for _, ip := range tp.InputParams {
 			v := convertInputParamToVariable(ip)
 			r.InputParams = append(r.InputParams, v)

service/api/types.go

@@ -58,6 +58,7 @@ type TracepointResult struct {
 	File string `json:"file"`
 	// Line is a line in File for the breakpoint.
 	Line  int  `json:"line"`
+	IsRet bool `json:"is_ret"`
 	// FunctionName is the name of the function at the current breakpoint, and
 	// may not always be available.
 	FunctionName string `json:"functionName,omitempty"`

service/debugger/debugger.go

@@ -2272,6 +2272,8 @@ func (d *Debugger) GetBufferedTracepoints() []api.TracepointResult {
 	}
 	results := make([]api.TracepointResult, len(traces))
 	for i, trace := range traces {
+		results[i].IsRet = trace.IsRet
+
 		f, l, fn := d.target.Selected.BinInfo().PCToLine(uint64(trace.FnAddr))
 
 		results[i].FunctionName = fn.Name