Repository: go-delve/delve
Pull request #2093 (2020-06-26 16:52:32)

service/dap: Support stackTrace request

derekparker: > @derekparker: it's bothering me somewhat that dap is calling debugger.go which converts the result into a service/api type that then dap converts into a dap type. I think that debugger.go should return proc.StackFrame and then service/rpc* and service/dap should do the conversion starting from that.
> 
> Same thing for debugger.(*Debugger).ListGoroutines.
> 
> I'm not saying we should do it with this PR but what do you think?

I agree it's definitely not ideal. Let's not do anything in this PR but it's certainly worth looking into how to simplify the various conversions. It may be that the API layer directly converts to the type it needs to return from the raw proc types. The downside to this is it kind of breaks the contract we set up where the API layer doesn't really know about the lower-level proc types.

Patch:
diff --git a/service/dap/error_ids.go b/service/dap/error_ids.go
index 6fbd0b6c..e8034a84 100644
--- a/service/dap/error_ids.go
+++ b/service/dap/error_ids.go
@@ -12,7 +12,8 @@ const (
 	// TODO(polina): confirm if the extension expects specific ids
 	// for specific cases, and we must match the existing adaptor
 	// or if these codes can evolve.
-	FailedToContinue       = 3000
-	UnableToDisplayThreads = 2003
+	FailedToContinue          = 3000
+	UnableToDisplayThreads    = 2003
+	UnableToProduceStackTrace = 2004
 	// Add more codes as we support more requests
 )
diff --git a/service/dap/handles.go b/service/dap/handles.go
new file mode 100644
index 00000000..05bfd3dd
--- /dev/null
+++ b/service/dap/handles.go
@@ -0,0 +1,34 @@
+package dap
+
+const startHandle = 1000
+
+// handlesMap maps arbitrary values to unique sequential ids.
+// This provides convenient abstraction of references, offering
+// opacity and allowing simplification of complex identifiers.
+// Based on
+// https://github.com/microsoft/vscode-debugadapter-node/blob/master/adapter/src/handles.ts
+type handlesMap struct {
+	nextHandle  int
+	handleToVal map[int]interface{}
+}
+
+func newHandlesMap() *handlesMap {
+	return &handlesMap{startHandle, make(map[int]interface{})}
+}
+
+func (hs *handlesMap) reset() {
+	hs.nextHandle = startHandle
+	hs.handleToVal = make(map[int]interface{})
+}
+
+func (hs *handlesMap) create(value interface{}) int {
+	next := hs.nextHandle
+	hs.nextHandle++
+	hs.handleToVal[next] = value
+	return next
+}
+
+func (hs *handlesMap) get(handle int) (interface{}, bool) {
+	v, ok := hs.handleToVal[handle]
+	return v, ok
+}
diff --git a/service/dap/server.go b/service/dap/server.go
index c2fd0f20..8cac9d32 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -56,6 +56,8 @@ type Server struct {
 	stopOnEntry bool
 	// binaryToRemove is the compiled binary to be removed on disconnect.
 	binaryToRemove string
+	// stackFrameHandles keep track of unique ids created across all goroutines.
+	stackFrameHandles *handlesMap
 }
 
 // NewServer creates a new DAP Server. It takes an opened Listener
@@ -67,10 +69,11 @@ func NewServer(config *service.Config) *Server {
 	logflags.WriteDAPListeningMessage(config.Listener.Addr().String())
 	logger.Debug("DAP server pid = ", os.Getpid())
 	return &Server{
-		config:   config,
-		listener: config.Listener,
-		stopChan: make(chan struct{}),
-		log:      logger,
+		config:            config,
+		listener:          config.Listener,
+		stopChan:          make(chan struct{}),
+		log:               logger,
+		stackFrameHandles: newHandlesMap(),
 	}
 }
 
@@ -263,7 +266,6 @@ func (s *Server) handleRequest(request dap.Message) {
 		s.onPauseRequest(request)
 	case *dap.StackTraceRequest:
 		// Required
-		// TODO: implement this request in V0
 		s.onStackTraceRequest(request)
 	case *dap.ScopesRequest:
 		// Required
@@ -623,10 +625,48 @@ func (s *Server) onPauseRequest(request *dap.PauseRequest) { // TODO V0
 	s.sendNotYetImplementedErrorResponse(request.Request)
 }
 
-// onStackTraceRequest sends a not-yet-implemented error response.
+// stackFrame represents the index of a frame within
+// the context of a stack of a specific goroutine.
+type stackFrame struct {
+	goroutineID int
+	frameIndex  int
+}
+
+// onStackTraceRequest handles ‘stackTrace’ requests.
 // This is a mandatory request to support.
-func (s *Server) onStackTraceRequest(request *dap.StackTraceRequest) { // TODO V0
-	s.sendNotYetImplementedErrorResponse(request.Request)
+func (s *Server) onStackTraceRequest(request *dap.StackTraceRequest) {
+	goroutineID := request.Arguments.ThreadId
+	// Borrow the defaults for the arguments from the original vscode-go adapter.
+	// TODO(polina): is 'startFrame'+'levels' a better alternative for depth?
+	// TODO(polina): support setting depth via launch/attach args
+	depth := 50 // length of the returned list of Stackframes
+	locs, err := s.debugger.Stacktrace(goroutineID, depth, 0, nil /*skip locals & args*/)
+	if err != nil {
+		s.sendErrorResponse(request.Request, UnableToProduceStackTrace, "Unable to produce stack trace", err.Error())
+		return
+	}
+
+	stackFrames := make([]dap.StackFrame, len(locs))
+	for i, loc := range locs {
+		uniqueStackFrameID := s.stackFrameHandles.create(stackFrame{goroutineID, i})
+		stackFrames[i] = dap.StackFrame{Id: uniqueStackFrameID, Line: loc.Line}
+		stackFrames[i].Name = loc.Function.Name()
+		if loc.File != "<autogenerated>" {
+			stackFrames[i].Source = dap.Source{Name: filepath.Base(loc.File), Path: loc.File}
+		}
+		stackFrames[i].Column = 0
+	}
+	if request.Arguments.StartFrame > 0 {
+		stackFrames = stackFrames[min(request.Arguments.StartFrame, len(stackFrames)):]
+	}
+	if request.Arguments.Levels > 0 {
+		stackFrames = stackFrames[:min(request.Arguments.Levels, len(stackFrames))]
+	}
+	response := &dap.StackTraceResponse{
+		Response: *newResponse(request.Request),
+		Body:     dap.StackTraceResponseBody{StackFrames: stackFrames, TotalFrames: len(locs)},
+	}
+	s.send(response)
 }
 
 // onScopesRequest sends a not-yet-implemented error response.
@@ -788,6 +828,7 @@ func (s *Server) doContinue() {
 		}
 		return
 	}
+	s.stackFrameHandles.reset()
 	if state.Exited {
 		e := &dap.TerminatedEvent{Event: *newEvent("terminated")}
 		s.send(e)
diff --git a/service/dap/utils.go b/service/dap/utils.go
new file mode 100644
index 00000000..5b4ff3b4
--- /dev/null
+++ b/service/dap/utils.go
@@ -0,0 +1,10 @@
+package dap
+
+// min returns the lowest-valued integer
+// between the two passed into it.
+func min(i, j int) int {
+	if i < j {
+		return i
+	}
+	return j
+}


Test patch:
diff --git a/service/dap/daptest/client.go b/service/dap/daptest/client.go
index 67a70966..6b8fce3e 100644
--- a/service/dap/daptest/client.go
+++ b/service/dap/daptest/client.go
@@ -368,8 +368,11 @@ func (c *Client) ThreadsRequest() {
 }
 
 // StackTraceRequest sends a 'stackTrace' request.
-func (c *Client) StackTraceRequest() {
+func (c *Client) StackTraceRequest(threadID, startFrame, levels int) {
 	request := &dap.StackTraceRequest{Request: *c.newRequest("stackTrace")}
+	request.Arguments.ThreadId = threadID
+	request.Arguments.StartFrame = startFrame
+	request.Arguments.Levels = levels
 	c.send(request)
 }
 
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index ff86808b..3ed2b120 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -166,17 +166,17 @@ func TestStopOnEntry(t *testing.T) {
 		}
 
 		// 8 >> stackTrace, << stackTrace
-		client.StackTraceRequest()
+		client.StackTraceRequest(1, 0, 20)
 		stResp := client.ExpectErrorResponse(t)
-		if stResp.Seq != 0 || stResp.RequestSeq != 8 || stResp.Message != "Not yet implemented" {
-			t.Errorf("\ngot %#v\nwant Seq=0, RequestSeq=8 Message=\"Not yet implemented\"", stResp)
+		if stResp.Seq != 0 || stResp.RequestSeq != 8 || stResp.Body.Error.Format != "Unable to produce stack trace: unknown goroutine 1" {
+			t.Errorf("\ngot %#v\nwant Seq=0, RequestSeq=8 Format=\"Unable to produce stack trace: unknown goroutine 1\"", stResp)
 		}
 
 		// 9 >> stackTrace, << stackTrace
-		client.StackTraceRequest()
+		client.StackTraceRequest(1, 0, 20)
 		stResp = client.ExpectErrorResponse(t)
-		if stResp.Seq != 0 || stResp.RequestSeq != 9 || stResp.Message != "Not yet implemented" {
-			t.Errorf("\ngot %#v\nwant Seq=0, RequestSeq=9 Message=\"Not yet implemented\"", stResp)
+		if stResp.Seq != 0 || stResp.RequestSeq != 9 || stResp.Body.Error.Id != 2004 {
+			t.Errorf("\ngot %#v\nwant Seq=0, RequestSeq=9 Id=2004", stResp)
 		}
 
 		// 10 >> continue, << continue, << terminated
@@ -306,8 +306,139 @@ func TestSetBreakpoint(t *testing.T) {
 			}
 		}
 
+		client.StackTraceRequest(1, 0, 20)
+		stResp := client.ExpectStackTraceResponse(t)
+
+		if stResp.Body.TotalFrames != 6 {
+			t.Errorf("\ngot %#v\nwant TotalFrames=6", stResp.Body.TotalFrames)
+		}
+		if len(stResp.Body.StackFrames) != 6 {
+			t.Errorf("\ngot %#v\nwant len(StackFrames)=6", stResp.Body.StackFrames)
+		} else {
+			expectStackFrame := func(got dap.StackFrame, id int, name string, sourceName string, line int) {
+				t.Helper()
+				if got.Id != id || got.Name != name {
+					t.Errorf("\ngot  %#v\nwant id=%d name=%s", got, id, name)
+				}
+				if (sourceName != "" && got.Source.Name != sourceName) || (line > 0 && got.Line != line) {
+					t.Errorf("\ngot  %#v\nwant source.name=%s line=%d", got, sourceName, line)
+				}
+			}
+			expectStackFrame(stResp.Body.StackFrames[0], 1000, "main.Increment", "increment.go", 8)
+			expectStackFrame(stResp.Body.StackFrames[1], 1001, "main.Increment", "increment.go", 11)
+			expectStackFrame(stResp.Body.StackFrames[2], 1002, "main.Increment", "increment.go", 11)
+			expectStackFrame(stResp.Body.StackFrames[3], 1003, "main.main", "increment.go", 17)
+			expectStackFrame(stResp.Body.StackFrames[4], 1004, "runtime.main", "proc.go", -1)
+			expectStackFrame(stResp.Body.StackFrames[5], 1005, "runtime.goexit", "", -1)
+		}
+
 		// TODO(polina): add other status checking requests
-		// that are not yet supported (stackTrace, scopes, variables)
+		// that are not yet supported (scopes, variables)
+
+		client.ContinueRequest(1)
+		client.ExpectContinueResponse(t)
+		// "Continue" is triggered after the response is sent
+
+		client.ExpectTerminatedEvent(t)
+		client.DisconnectRequest()
+		client.ExpectDisconnectResponse(t)
+	})
+}
+
+// TestStackTraceRequest executes to a breakpoint (similarly to TestSetBreakpoint
+// that includes more thorough checking of that sequence) and tests different
+// good and bad configurations of 'stackTrace' requests.
+func TestStackTraceRequest(t *testing.T) {
+	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
+		client.InitializeRequest()
+		client.ExpectInitializeResponse(t)
+
+		client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+		client.ExpectInitializedEvent(t)
+		client.ExpectLaunchResponse(t)
+
+		client.SetBreakpointsRequest(fixture.Source, []int{8, 18})
+		client.ExpectSetBreakpointsResponse(t)
+
+		client.ConfigurationDoneRequest()
+		client.ExpectConfigurationDoneResponse(t)
+		// This triggers "continue"
+
+		var stResp *dap.StackTraceResponse
+		var expectFrames = func(wantStart int, wantFrames int, wantTotal int, wantLine int) {
+			t.Helper()
+			if stResp.Body.TotalFrames != wantTotal {
+				t.Errorf("\ngot  %#v\nwant TotalFrames=%d", stResp.Body.TotalFrames, wantTotal)
+			}
+			if len(stResp.Body.StackFrames) != wantFrames {
+				t.Errorf("\ngot  len=%d\nwant len=%d", len(stResp.Body.StackFrames), wantFrames)
+			} else {
+				for i := 0; i < wantFrames; i++ {
+					if stResp.Body.StackFrames[i].Id != wantStart+i {
+						t.Errorf("\ngot  %#v\nwant id=%d", stResp.Body.StackFrames[i], wantStart+i)
+					}
+				}
+				if wantFrames > 0 && wantLine > 0 && stResp.Body.StackFrames[0].Line != wantLine {
+					t.Errorf("\ngot  line=%d\nwant line=%d", stResp.Body.StackFrames[0].Line, wantLine)
+				}
+			}
+		}
+
+		client.ExpectStoppedEvent(t) // at line 8
+
+		client.StackTraceRequest(1, 0, 0)
+		stResp = client.ExpectStackTraceResponse(t)
+		expectFrames(1000, 6, 6, 8)
+
+		// Even though the stack frames are the same,
+		// repeated requests at the same breakpoint,
+		// would assign unique ids to them each time.
+		client.StackTraceRequest(1, -1, 0)
+		stResp = client.ExpectStackTraceResponse(t) // Or should this return an ErrorResponse?
+		expectFrames(1006, 6, 6, 8)
+
+		client.StackTraceRequest(1, 3, 0)
+		stResp = client.ExpectStackTraceResponse(t)
+		expectFrames(1015, 3, 6, 17)
+
+		client.StackTraceRequest(1, 6, 0)
+		stResp = client.ExpectStackTraceResponse(t)
+		expectFrames(0, 0, 6, 0)
+
+		client.StackTraceRequest(1, 7, 0)
+		stResp = client.ExpectStackTraceResponse(t) // Or should this return an ErrorResponse?
+		expectFrames(0, 0, 6, 0)
+
+		client.ContinueRequest(1)
+		client.ExpectContinueResponse(t)
+		// "Continue" is triggered after the response is sent
+
+		client.ExpectStoppedEvent(t) // at line 18
+
+		// Frame ids get reset at each breakpoint.
+		client.StackTraceRequest(1, 0, 0)
+		stResp = client.ExpectStackTraceResponse(t)
+		expectFrames(1000, 3, 3, 18)
+
+		client.StackTraceRequest(1, 0, -1) // Or should this return an ErrorResponse?
+		stResp = client.ExpectStackTraceResponse(t)
+		expectFrames(1003, 3, 3, 18)
+
+		client.StackTraceRequest(1, 0, 2)
+		stResp = client.ExpectStackTraceResponse(t)
+		expectFrames(1006, 2, 3, 18)
+
+		client.StackTraceRequest(1, 0, 3)
+		stResp = client.ExpectStackTraceResponse(t)
+		expectFrames(1009, 3, 3, 18)
+
+		client.StackTraceRequest(1, 0, 4)
+		stResp = client.ExpectStackTraceResponse(t)
+		expectFrames(1012, 3, 3, 18)
+
+		client.StackTraceRequest(1, 1, 2)
+		stResp = client.ExpectStackTraceResponse(t)
+		expectFrames(1016, 2, 3, -1) // Don't test for runtime line we don't control.
 
 		client.ContinueRequest(1)
 		client.ExpectContinueResponse(t)
@@ -477,9 +608,6 @@ func TestRequiredNotYetImplementedResponses(t *testing.T) {
 		client.PauseRequest()
 		expectNotYetImplemented("pause")
 
-		client.StackTraceRequest()
-		expectNotYetImplemented("stackTrace")
-
 		client.ScopesRequest()
 		expectNotYetImplemented("scopes")