diff --git a/service/dap/daptest/client.go b/service/dap/daptest/client.go index 52221adc..fc3d7dcc 100644 --- a/service/dap/daptest/client.go +++ b/service/dap/daptest/client.go @@ -101,6 +101,7 @@ func (c *Client) ExpectInitializeResponseAndCapabilities(t *testing.T) *dap.Init SupportsDelayedStackTraceLoading: true, SupportTerminateDebuggee: true, SupportsExceptionInfoRequest: true, + SupportsFunctionBreakpoints: true, } if !reflect.DeepEqual(initResp.Body, wantCapabilities) { t.Errorf("capabilities in initializeResponse: got %+v, want %v", pretty(initResp.Body), pretty(wantCapabilities)) @@ -331,8 +332,13 @@ func (c *Client) RestartRequest() { } // SetFunctionBreakpointsRequest sends a 'setFunctionBreakpoints' request. -func (c *Client) SetFunctionBreakpointsRequest() { - c.send(&dap.SetFunctionBreakpointsRequest{Request: *c.newRequest("setFunctionBreakpoints")}) +func (c *Client) SetFunctionBreakpointsRequest(breakpoints []dap.FunctionBreakpoint) { + c.send(&dap.SetFunctionBreakpointsRequest{ + Request: *c.newRequest("setFunctionBreakpoints"), + Arguments: dap.SetFunctionBreakpointsArguments{ + Breakpoints: breakpoints, + }, + }) } // StepBackRequest sends a 'stepBack' request. diff --git a/service/dap/server.go b/service/dap/server.go index 2e108cd8..40843093 100644 --- a/service/dap/server.go +++ b/service/dap/server.go @@ -667,13 +667,13 @@ func (s *Server) onInitializeRequest(request *dap.InitializeRequest) { response.Body.SupportsConditionalBreakpoints = true response.Body.SupportsDelayedStackTraceLoading = true response.Body.SupportTerminateDebuggee = true + response.Body.SupportsFunctionBreakpoints = true response.Body.SupportsExceptionInfoRequest = true // TODO(polina): support this to match vscode-go functionality response.Body.SupportsSetVariable = false // TODO(polina): support these requests in addition to vscode-go feature parity response.Body.SupportsTerminateRequest = false response.Body.SupportsRestartRequest = false - response.Body.SupportsFunctionBreakpoints = false response.Body.SupportsStepBack = false response.Body.SupportsSetExpression = false response.Body.SupportsLoadedSourcesRequest = false @@ -1020,23 +1020,10 @@ func (s *Server) onSetBreakpointsRequest(request *dap.SetBreakpointsRequest) { // -- exists and in request => AmendBreakpoint // -- doesn't exist and in request => SetBreakpoint - // Clear all existing breakpoints in the file. - existing := s.debugger.Breakpoints() - for _, bp := range existing { - // Skip special breakpoints such as for panic. - if bp.ID < 0 { - continue - } - // Skip other source files. - // TODO(polina): should this be normalized because of different OSes? - if bp.File != serverPath { - continue - } - _, err := s.debugger.ClearBreakpoint(bp) - if err != nil { - s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", err.Error()) - return - } + // Clear all existing breakpoints in the file that are not function breakpoints. + if err := s.clearMatchingBreakpoints(func(bp *api.Breakpoint) bool { return bp.File == serverPath && bp.Name == "" }); err != nil { + s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", err.Error()) + return } // Set all requested breakpoints. @@ -1883,10 +1870,101 @@ func (s *Server) onRestartRequest(request *dap.RestartRequest) { s.sendNotYetImplementedErrorResponse(request.Request) } -// onSetFunctionBreakpointsRequest sends a not-yet-implemented error response. -// Capability 'supportsFunctionBreakpoints' is not set 'initialize' response. +// functionBpPrefix is the prefix of bp.Name for every breakpoint bp set +// in this request. +const functionBpPrefix = "functionBreakpoint" + func (s *Server) onSetFunctionBreakpointsRequest(request *dap.SetFunctionBreakpointsRequest) { - s.sendNotYetImplementedErrorResponse(request.Request) + if s.noDebugProcess != nil { + s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", "running in noDebug mode") + return + } + // TODO(polina): handle this while running by halting first. + + // According to the spec, setFunctionBreakpoints "replaces all existing function + // breakpoints with new function breakpoints." The simplest way is + // to clear all and then set all. + + // Clear all existing function breakpoints in the file. + // Function breakpoints are set with the Name field set to be the + // function name. We cannot use FunctionName to determine this, because + // the debugger sets the function name for all breakpoints. + if err := s.clearMatchingBreakpoints(func(bp *api.Breakpoint) bool { return strings.HasPrefix(bp.Name, functionBpPrefix) }); err != nil { + s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", err.Error()) + return + } + + // Set all requested breakpoints. + response := &dap.SetFunctionBreakpointsResponse{Response: *newResponse(request.Request)} + response.Body.Breakpoints = make([]dap.Breakpoint, len(request.Arguments.Breakpoints)) + for i, want := range request.Arguments.Breakpoints { + spec, err := locspec.Parse(want.Name) + if err != nil { + response.Body.Breakpoints[i].Message = err.Error() + continue + } + if loc, ok := spec.(*locspec.NormalLocationSpec); !ok || loc.FuncBase == nil { + // Other locations do not make sense in the context of function breakpoints. + // Regex locations are likely to resolve to multiple places and offset locations + // are only meaningful at the time the breakpoint was created. + response.Body.Breakpoints[i].Message = fmt.Sprintf("breakpoint name %q could not be parsed as a function. name must be in the format 'funcName', 'funcName:line' or 'fileName:line'.", want.Name) + continue + } + + if want.Name[0] == '.' { + response.Body.Breakpoints[i].Message = "breakpoint names that are relative paths are not supported." + continue + } + // Find the location of the function name. CreateBreakpoint requires the name to include the base + // (e.g. main.functionName is supported but not functionName). + // We first find the location of the function, and then set breakpoints for that location. + locs, err := s.debugger.FindLocationSpec(-1, 0, 0, want.Name, spec, true, s.args.substitutePathClientToServer) + if err != nil { + response.Body.Breakpoints[i].Message = err.Error() + continue + } + if len(locs) == 0 { + response.Body.Breakpoints[i].Message = fmt.Sprintf("no location found for %q", want.Name) + continue + } + if len(locs) > 0 { + s.log.Debugf("multiple locations found for %s", want.Name) + response.Body.Breakpoints[i].Message = fmt.Sprintf("multiple locations found for %s, function breakpoint is only set for the first location", want.Name) + } + + // Set breakpoint using the PCs that were found. + loc := locs[0] + got, err := s.debugger.CreateBreakpoint( + &api.Breakpoint{Name: fmt.Sprintf("%s%d", functionBpPrefix, i), Addr: loc.PC, Addrs: loc.PCs, Cond: want.Condition}) + response.Body.Breakpoints[i].Verified = (err == nil) + if err != nil { + response.Body.Breakpoints[i].Message = err.Error() + } else { + response.Body.Breakpoints[i].Id = got.ID + response.Body.Breakpoints[i].Line = got.Line + response.Body.Breakpoints[i].Source = dap.Source{Name: filepath.Base(got.File), Path: got.File} + } + } + s.send(response) +} + +func (s *Server) clearMatchingBreakpoints(matchCondition func(*api.Breakpoint) bool) error { + existing := s.debugger.Breakpoints() + for _, bp := range existing { + // Skip special breakpoints such as for panic. + if bp.ID < 0 { + continue + } + // Skip breakpoints that do not meet the condition. + if !matchCondition(bp) { + continue + } + _, err := s.debugger.ClearBreakpoint(bp) + if err != nil { + return err + } + } + return nil } // onStepBackRequest sends a not-yet-implemented error response. @@ -2144,6 +2222,9 @@ func (s *Server) doRunCommand(command string, asyncSetupDone chan struct{}) { stopped.Body.Reason = "exception" stopped.Body.Description = "Paused on panic" } + if strings.HasPrefix(state.CurrentThread.Breakpoint.Name, functionBpPrefix) { + stopped.Body.Reason = "function breakpoint" + } } } else { s.exceptionErr = err diff --git a/service/dap/server_test.go b/service/dap/server_test.go index 6175a0af..dc66c2c8 100644 --- a/service/dap/server_test.go +++ b/service/dap/server_test.go @@ -1949,6 +1949,259 @@ func TestSetBreakpoint(t *testing.T) { }) } +// TestSetFunctionBreakpoints is inspired by service/test.TestClientServer_FindLocations. +func TestSetFunctionBreakpoints(t *testing.T) { + runTest(t, "locationsprog", func(client *daptest.Client, fixture protest.Fixture) { + runDebugSessionWithBPs(t, client, "launch", + // Launch + func() { + client.LaunchRequest("exec", fixture.Path, !stopOnEntry) + }, + // Set breakpoints + fixture.Source, []int{30}, // b main.main + []onBreakpoint{{ + execute: func() { + handleStop(t, client, 1, "main.main", 30) + + type Breakpoint struct { + line int + sourceName string + verified bool + errMsg string + } + expectSetFunctionBreakpointsResponse := func(bps []Breakpoint) { + t.Helper() + got := client.ExpectSetFunctionBreakpointsResponse(t) + if len(got.Body.Breakpoints) != len(bps) { + t.Errorf("got %#v,\nwant len(Breakpoints)=%d", got, len(bps)) + return + } + for i, bp := range got.Body.Breakpoints { + if bps[i].line < 0 { + if bp.Verified != bps[i].verified || !strings.Contains(bp.Message, bps[i].errMsg) { + t.Errorf("got breakpoints[%d] = %#v, \nwant %#v", i, bp, bps[i]) + } + continue + } + // Some function breakpoints may be in packages that have been imported and we do not control, so + // we do not always want to check breakpoint lines. + if (bps[i].line >= 0 && bp.Line != bps[i].line) || bp.Verified != bps[i].verified || bp.Source.Name != bps[i].sourceName { + t.Errorf("got breakpoints[%d] = %#v, \nwant %#v", i, bp, bps[i]) + } + } + } + + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "anotherFunction"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{26, filepath.Base(fixture.Source), true, ""}}) + + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "main.anotherFunction"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{26, filepath.Base(fixture.Source), true, ""}}) + + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "SomeType.String"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{14, filepath.Base(fixture.Source), true, ""}}) + + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "(*SomeType).String"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{14, filepath.Base(fixture.Source), true, ""}}) + + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "main.SomeType.String"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{14, filepath.Base(fixture.Source), true, ""}}) + + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "main.(*SomeType).String"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{14, filepath.Base(fixture.Source), true, ""}}) + + // Test line offsets + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "main.anotherFunction:1"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{27, filepath.Base(fixture.Source), true, ""}}) + + // Test function names in imported package. + // Issue #275 + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "io/ioutil.ReadFile"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, "ioutil.go", true, ""}}) + + // Issue #296 + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "/io/ioutil.ReadFile"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, "ioutil.go", true, ""}}) + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "ioutil.ReadFile"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, "ioutil.go", true, ""}}) + + // Function Breakpoint name also accepts breakpoints that are specified as file:line. + // TODO(suzmue): We could return an error, but it probably is not necessary since breakpoints, + // and function breakpoints come in with different requests. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: fmt.Sprintf("%s:14", fixture.Source)}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{14, filepath.Base(fixture.Source), true, ""}}) + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: fmt.Sprintf("%s:14", filepath.Base(fixture.Source))}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{14, filepath.Base(fixture.Source), true, ""}}) + + // Expect error for ambiguous function name. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "String"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, "", false, "Location \"String\" ambiguous"}}) + + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "main.String"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, "", false, "Location \"main.String\" ambiguous"}}) + + // Expect error for function that does not exist. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "fakeFunction"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, "", false, "location \"fakeFunction\" not found"}}) + + // Expect error for negative line number. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "main.anotherFunction:-1"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, "", false, "line offset negative or not a number"}}) + + // Expect error when function name is regex. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: `/^.*String.*$/`}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, "", false, "breakpoint name \"/^.*String.*$/\" could not be parsed as a function"}}) + + // Expect error when function name is an offset. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "+1"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, filepath.Base(fixture.Source), false, "breakpoint name \"+1\" could not be parsed as a function"}}) + + // Expect error when function name is a line number. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "14"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, filepath.Base(fixture.Source), false, "breakpoint name \"14\" could not be parsed as a function"}}) + + // Expect error when function name is an address. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "*b"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, filepath.Base(fixture.Source), false, "breakpoint name \"*b\" could not be parsed as a function"}}) + + // Expect error when function name is a relative path. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: fmt.Sprintf(".%s%s:14", string(filepath.Separator), filepath.Base(fixture.Source))}, + }) + // This relative path could also be caught by the parser, so we should not match the error message. + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, filepath.Base(fixture.Source), false, ""}}) + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: fmt.Sprintf("..%s%s:14", string(filepath.Separator), filepath.Base(fixture.Source))}, + }) + // This relative path could also be caught by the parser, so we should not match the error message. + expectSetFunctionBreakpointsResponse([]Breakpoint{{-1, filepath.Base(fixture.Source), false, ""}}) + + // Test multiple function breakpoints. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "SomeType.String"}, {Name: "anotherFunction"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{14, filepath.Base(fixture.Source), true, ""}, {26, filepath.Base(fixture.Source), true, ""}}) + + // Test multiple breakpoints to the same location. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "SomeType.String"}, {Name: "(*SomeType).String"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{14, filepath.Base(fixture.Source), true, ""}, {-1, "", false, "Breakpoint exists"}}) + + // Set two breakpoints at SomeType.String and SomeType.SomeFunction. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "SomeType.String"}, {Name: "SomeType.SomeFunction"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{14, filepath.Base(fixture.Source), true, ""}, {22, filepath.Base(fixture.Source), true, ""}}) + + // Clear SomeType.String, reset SomeType.SomeFunction (SomeType.String is called before SomeType.SomeFunction). + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "SomeType.SomeFunction"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{22, filepath.Base(fixture.Source), true, ""}}) + + // Expect the next breakpoint to be at SomeType.SomeFunction. + client.ContinueRequest(1) + client.ExpectContinueResponse(t) + + if se := client.ExpectStoppedEvent(t); se.Body.Reason != "function breakpoint" || se.Body.ThreadId != 1 { + t.Errorf("got %#v, want Reason=\"function breakpoint\", ThreadId=1", se) + } + handleStop(t, client, 1, "main.(*SomeType).SomeFunction", 22) + + // Set a breakpoint at the next line in the program. + client.SetBreakpointsRequest(fixture.Source, []int{23}) + got := client.ExpectSetBreakpointsResponse(t) + if len(got.Body.Breakpoints) != 1 { + t.Errorf("got %#v,\nwant len(Breakpoints)=%d", got, 1) + return + } + bp := got.Body.Breakpoints[0] + if bp.Line != 23 || bp.Verified != true || bp.Source.Path != fixture.Source { + t.Errorf("got breakpoints[0] = %#v, \nwant Line=23 Verified=true Source.Path=%q", bp, fixture.Source) + } + + // Set a function breakpoint, this should not clear the breakpoint that was set in the previous setBreakpoints request. + client.SetFunctionBreakpointsRequest([]dap.FunctionBreakpoint{ + {Name: "anotherFunction"}, + }) + expectSetFunctionBreakpointsResponse([]Breakpoint{{26, filepath.Base(fixture.Source), true, ""}}) + + // Expect the next breakpoint to be at line 23. + client.ContinueRequest(1) + client.ExpectContinueResponse(t) + + if se := client.ExpectStoppedEvent(t); se.Body.Reason != "breakpoint" || se.Body.ThreadId != 1 { + t.Errorf("got %#v, want Reason=\"breakpoint\", ThreadId=1", se) + } + handleStop(t, client, 1, "main.(*SomeType).SomeFunction", 23) + + // Set a breakpoint, this should not clear the breakpoint that was set in the previous setFunctionBreakpoints request. + client.SetBreakpointsRequest(fixture.Source, []int{37}) + got = client.ExpectSetBreakpointsResponse(t) + if len(got.Body.Breakpoints) != 1 { + t.Errorf("got %#v,\nwant len(Breakpoints)=%d", got, 1) + return + } + bp = got.Body.Breakpoints[0] + if bp.Line != 37 || bp.Verified != true || bp.Source.Path != fixture.Source { + t.Errorf("got breakpoints[0] = %#v, \nwant Line=23 Verified=true Source.Path=%q", bp, fixture.Source) + } + + // Expect the next breakpoint to be at line anotherFunction. + client.ContinueRequest(1) + client.ExpectContinueResponse(t) + + if se := client.ExpectStoppedEvent(t); se.Body.Reason != "function breakpoint" || se.Body.ThreadId != 1 { + t.Errorf("got %#v, want Reason=\"function breakpoint\", ThreadId=1", se) + } + handleStop(t, client, 1, "main.anotherFunction", 26) + + }, + disconnect: true, + }}) + }) +} + func expectSetBreakpointsResponseAndStoppedEvent(t *testing.T, client *daptest.Client) (se *dap.StoppedEvent, br *dap.SetBreakpointsResponse) { for i := 0; i < 2; i++ { switch m := client.ExpectMessage(t).(type) { @@ -2499,7 +2752,7 @@ func TestNextAndStep(t *testing.T) { client.ExpectNextResponse(t) client.ExpectContinuedEvent(t) if se := client.ExpectStoppedEvent(t); se.Body.Reason != "error" || se.Body.Text != "unknown goroutine -1000" { - t.Errorf("got %#v, want Reaspon=\"error\", Text=\"unknown goroutine -1000\"", se) + t.Errorf("got %#v, want Reason=\"error\", Text=\"unknown goroutine -1000\"", se) } handleStop(t, client, 1, "main.inlineThis", 5) }, @@ -3287,9 +3540,6 @@ func TestOptionalNotYetImplementedResponses(t *testing.T) { client.RestartRequest() expectNotYetImplemented("restart") - client.SetFunctionBreakpointsRequest() - expectNotYetImplemented("setFunctionBreakpoints") - client.StepBackRequest() expectNotYetImplemented("stepBack") diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go index f58dc337..0aa4de22 100644 --- a/service/debugger/debugger.go +++ b/service/debugger/debugger.go @@ -1585,9 +1585,28 @@ func (d *Debugger) FindLocation(goid, frame, deferredCall int, locStr string, in return nil, err } + return d.findLocation(goid, frame, deferredCall, locStr, loc, includeNonExecutableLines, substitutePathRules) +} + +// FindLocationSpec will find the location specified by 'locStr' and 'locSpec'. +// 'locSpec' should be the result of calling 'locspec.Parse(locStr)'. 'locStr' +// is also passed, because it made be used to broaden the search criteria, if +// the parsed result did not find anything. +func (d *Debugger) FindLocationSpec(goid, frame, deferredCall int, locStr string, locSpec locspec.LocationSpec, includeNonExecutableLines bool, substitutePathRules [][2]string) ([]api.Location, error) { + d.targetMutex.Lock() + defer d.targetMutex.Unlock() + + if _, err := d.target.Valid(); err != nil { + return nil, err + } + + return d.findLocation(goid, frame, deferredCall, locStr, locSpec, includeNonExecutableLines, substitutePathRules) +} + +func (d *Debugger) findLocation(goid, frame, deferredCall int, locStr string, locSpec locspec.LocationSpec, includeNonExecutableLines bool, substitutePathRules [][2]string) ([]api.Location, error) { s, _ := proc.ConvertEvalScope(d.target, goid, frame, deferredCall) - locs, err := loc.Find(d.target, d.processArgs, s, locStr, includeNonExecutableLines, substitutePathRules) + locs, err := locSpec.Find(d.target, d.processArgs, s, locStr, includeNonExecutableLines, substitutePathRules) for i := range locs { if locs[i].PC == 0 { continue