service/dap: implement function breakpoints (#2450)

* service/dap: implement setFunctionBreakpoints request

* Fix the errors that would not allow func set

* use find locations instead of FindFunctionLocation

* add function breakpoint tests

* return after sending error response

* revert changes to debugger

* exclude regexp function names

* remove switch statement with one case

* remove ReadFile ambiguous test

* Remove TODO for multiple locs

* remove unnecessary setting of bp.Verified on error

* tighten condition for breakpoint name to match function breakpoint

* add tests for different loc types, add FindLocationSpec

* add test using base name of file

* make functionBreakpoint name a constant

* update stop reason to function breakpoint

* remove comment about optimizing onSetFunctionBreakpoints

* respond to review

* add comments to test

* change functionBpPrefix to const

* handle relative paths

* fix capabilites check

* update function breakpoint tests to check for failure

* use negative line number to determine which are errors
This commit is contained in:
Suzy Mueller 2021-05-18 13:25:16 -04:00 committed by GitHub
parent 4e582fa553
commit 745a513179
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 384 additions and 28 deletions

@ -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.

@ -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

@ -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")

@ -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