Repository: go-delve/delve
Pull request #2490 (2021-05-17 16:38:18)

pkg/proc: add support for hit count condition breakpoints
Similar to conditional breakpoints, users may want to set a breakpoint based on how many times a breakpoint has been hit. This change adds support for this feature to the proc layer by checking the hit condition after updating the hit count. The hit condition is implemented as a binary expression where the expr.X is replaced with the TotalHitCount for the breakpoint.

Updates #2481 

suzmue: @aarzilli I propagated this through to `service/dap` but it required some changes to how we set breakpoints to do this correctly (using amend instead of clear/create). I can split this back out into a couple of PRs if that will make it easier to review.
derekparker: CI failure:

```
make test
# github.com/go-delve/delve/service/dap
vet: service/dap/server_test.go:2387:6: undeclared name: handleStop
*** Error code 2
```
aarzilli: I think this needs a rebase.
derekparker: Re-running CI.

Patch:
diff --git a/pkg/proc/breakpoints.go b/pkg/proc/breakpoints.go
index 0b5dafea..3c264c0c 100644
--- a/pkg/proc/breakpoints.go
+++ b/pkg/proc/breakpoints.go
@@ -6,6 +6,7 @@ import (
 	"go/ast"
 	"go/constant"
 	"go/parser"
+	"go/token"
 	"reflect"
 )
 
@@ -71,6 +72,12 @@ type Breakpoint struct {
 	Cond ast.Expr
 	// internalCond is the same as Cond but used for the condition of internal breakpoints
 	internalCond ast.Expr
+	// HitCond: if not nil the breakpoint will be triggered only if the evaluated HitCond returns
+	// true with the TotalHitCount.
+	HitCond *struct {
+		Op  token.Token
+		Val int
+	}
 
 	// ReturnInfo describes how to collect return variables when this
 	// breakpoint is hit as a return breakpoint.
@@ -164,36 +171,72 @@ type returnBreakpointInfo struct {
 // CheckCondition evaluates bp's condition on thread.
 func (bp *Breakpoint) CheckCondition(thread Thread) BreakpointState {
 	bpstate := BreakpointState{Breakpoint: bp, Active: false, Internal: false, CondError: nil}
-	if bp.Cond == nil && bp.internalCond == nil {
+	bpstate.checkCond(thread)
+	// Update the breakpoint hit counts.
+	if bpstate.Breakpoint != nil && bpstate.Active {
+		if g, err := GetG(thread); err == nil {
+			bpstate.HitCount[g.ID]++
+		}
+		bpstate.TotalHitCount++
+	}
+	bpstate.checkHitCond(thread)
+	return bpstate
+}
+
+func (bpstate *BreakpointState) checkCond(thread Thread) {
+	if bpstate.Cond == nil && bpstate.internalCond == nil {
 		bpstate.Active = true
-		bpstate.Internal = bp.IsInternal()
-		return bpstate
+		bpstate.Internal = bpstate.IsInternal()
+		return
 	}
 	nextDeferOk := true
-	if bp.Kind&NextDeferBreakpoint != 0 {
+	if bpstate.Kind&NextDeferBreakpoint != 0 {
 		var err error
 		frames, err := ThreadStacktrace(thread, 2)
 		if err == nil {
 			nextDeferOk = isPanicCall(frames)
 			if !nextDeferOk {
-				nextDeferOk, _ = isDeferReturnCall(frames, bp.DeferReturns)
+				nextDeferOk, _ = isDeferReturnCall(frames, bpstate.DeferReturns)
 			}
 		}
 	}
-	if bp.IsInternal() {
+	if bpstate.IsInternal() {
 		// Check internalCondition if this is also an internal breakpoint
-		bpstate.Active, bpstate.CondError = evalBreakpointCondition(thread, bp.internalCond)
+		bpstate.Active, bpstate.CondError = evalBreakpointCondition(thread, bpstate.internalCond)
 		bpstate.Active = bpstate.Active && nextDeferOk
 		if bpstate.Active || bpstate.CondError != nil {
 			bpstate.Internal = true
-			return bpstate
+			return
 		}
 	}
-	if bp.IsUser() {
+	if bpstate.IsUser() {
 		// Check normal condition if this is also a user breakpoint
-		bpstate.Active, bpstate.CondError = evalBreakpointCondition(thread, bp.Cond)
+		bpstate.Active, bpstate.CondError = evalBreakpointCondition(thread, bpstate.Cond)
+	}
+}
+
+// checkHitCond evaluates bp's hit condition on thread.
+func (bpstate *BreakpointState) checkHitCond(thread Thread) {
+	if bpstate.HitCond == nil || !bpstate.Active || bpstate.Internal {
+		return
+	}
+	// Evaluate the breakpoint condition.
+	switch bpstate.HitCond.Op {
+	case token.EQL:
+		bpstate.Active = int(bpstate.TotalHitCount) == bpstate.HitCond.Val
+	case token.NEQ:
+		bpstate.Active = int(bpstate.TotalHitCount) != bpstate.HitCond.Val
+	case token.GTR:
+		bpstate.Active = int(bpstate.TotalHitCount) > bpstate.HitCond.Val
+	case token.LSS:
+		bpstate.Active = int(bpstate.TotalHitCount) < bpstate.HitCond.Val
+	case token.GEQ:
+		bpstate.Active = int(bpstate.TotalHitCount) >= bpstate.HitCond.Val
+	case token.LEQ:
+		bpstate.Active = int(bpstate.TotalHitCount) <= bpstate.HitCond.Val
+	case token.REM:
+		bpstate.Active = int(bpstate.TotalHitCount)%bpstate.HitCond.Val == 0
 	}
-	return bpstate
 }
 
 func isPanicCall(frames []Stackframe) bool {
diff --git a/pkg/proc/gdbserial/gdbserver.go b/pkg/proc/gdbserial/gdbserver.go
index e324b034..12123a63 100644
--- a/pkg/proc/gdbserial/gdbserver.go
+++ b/pkg/proc/gdbserial/gdbserver.go
@@ -1787,12 +1787,6 @@ func (t *gdbThread) SetCurrentBreakpoint(adjustPC bool) error {
 			}
 		}
 		t.CurrentBreakpoint = bp.CheckCondition(t)
-		if t.CurrentBreakpoint.Breakpoint != nil && t.CurrentBreakpoint.Active {
-			if g, err := proc.GetG(t); err == nil {
-				t.CurrentBreakpoint.HitCount[g.ID]++
-			}
-			t.CurrentBreakpoint.TotalHitCount++
-		}
 	}
 	return nil
 }
diff --git a/pkg/proc/native/threads.go b/pkg/proc/native/threads.go
index 62aa55ba..5ae8abc0 100644
--- a/pkg/proc/native/threads.go
+++ b/pkg/proc/native/threads.go
@@ -155,12 +155,6 @@ func (t *nativeThread) SetCurrentBreakpoint(adjustPC bool) error {
 
 	if bp != nil {
 		t.CurrentBreakpoint = bp.CheckCondition(t)
-		if t.CurrentBreakpoint.Breakpoint != nil && t.CurrentBreakpoint.Active {
-			if g, err := proc.GetG(t); err == nil {
-				t.CurrentBreakpoint.HitCount[g.ID]++
-			}
-			t.CurrentBreakpoint.TotalHitCount++
-		}
 	}
 	return nil
 }
diff --git a/service/api/conversions.go b/service/api/conversions.go
index 08514b4b..7a2d7179 100644
--- a/service/api/conversions.go
+++ b/service/api/conversions.go
@@ -47,6 +47,9 @@ func ConvertBreakpoint(bp *proc.Breakpoint) *Breakpoint {
 	var buf bytes.Buffer
 	printer.Fprint(&buf, token.NewFileSet(), bp.Cond)
 	b.Cond = buf.String()
+	if bp.HitCond != nil {
+		b.HitCond = fmt.Sprintf("%s %d", bp.HitCond.Op.String(), bp.HitCond.Val)
+	}
 
 	return b
 }
diff --git a/service/api/types.go b/service/api/types.go
index 26b165d5..f9163237 100644
--- a/service/api/types.go
+++ b/service/api/types.go
@@ -67,6 +67,9 @@ type Breakpoint struct {
 
 	// Breakpoint condition
 	Cond string
+	// Breakpoint hit count condition.
+	// Supported hit count conditions are "NUMBER" and "OP NUMBER".
+	HitCond string
 
 	// Tracepoint flag, signifying this is a tracepoint.
 	Tracepoint bool `json:"continue"`
diff --git a/service/dap/server.go b/service/dap/server.go
index 92de211f..6dcbd4e8 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -548,7 +548,6 @@ func (s *Server) handleRequest(request dap.Message) {
 		s.onSetBreakpointsRequest(request)
 	case *dap.SetFunctionBreakpointsRequest:
 		// Optional (capability ‘supportsFunctionBreakpoints’)
-		// TODO: implement this request in V1
 		s.onSetFunctionBreakpointsRequest(request)
 	case *dap.SetExceptionBreakpointsRequest:
 		// Optional (capability ‘exceptionBreakpointFilters’)
@@ -1030,40 +1029,241 @@ func (s *Server) onSetBreakpointsRequest(request *dap.SetBreakpointsRequest) {
 
 	// According to the spec we should "set multiple breakpoints for a single source
 	// and clear all previous breakpoints in that source." The simplest way is
-	// to clear all and then set all.
+	// to clear all and then set all. To maintain state (for hit count conditions)
+	// we want to amend existing breakpoints.
 	//
-	// TODO(polina): should we optimize this as follows?
 	// See https://github.com/golang/vscode-go/issues/163 for details.
 	// If a breakpoint:
 	// -- exists and not in request => ClearBreakpoint
 	// -- exists and in request => AmendBreakpoint
 	// -- doesn't exist and in request => SetBreakpoint
 
-	// 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 {
+	// Get all existing breakpoints that match for this source.
+	sourceRequestPrefix := fmt.Sprintf("sourceBp Path=%q ", request.Arguments.Source.Path)
+	existingBps := s.getMatchingBreakpoints(sourceRequestPrefix)
+	bpAdded := make(map[string]struct{}, len(existingBps))
+
+	// Amend existing breakpoints.
+	breakpoints := make([]dap.Breakpoint, len(request.Arguments.Breakpoints))
+	for i, want := range request.Arguments.Breakpoints {
+		reqString := fmt.Sprintf("%s Line=%d Column=%d", sourceRequestPrefix, want.Line, want.Column)
+		var err error
+		got, ok := existingBps[reqString]
+		if !ok {
+			// Skip if the breakpoint does not already exist.
+			// These will be created after deleting existing
+			// breakpoints to avoid conflicts.
+			continue
+		}
+		if _, ok := bpAdded[reqString]; ok {
+			err = fmt.Errorf("Breakpoint exists at %q, line: %d, column: %d", request.Arguments.Source.Path, want.Line, want.Column)
+		} else {
+			got.Cond = want.Condition
+			got.HitCond = want.HitCondition
+			err = s.debugger.AmendBreakpoint(got)
+			bpAdded[reqString] = struct{}{}
+		}
+
+		updateBreakpointsResponse(breakpoints, i, err, got, clientPath)
+	}
+
+	// Clear existing breakpoints that were not added.
+	err := s.clearBreakpoints(existingBps, bpAdded)
+	if err != nil {
 		s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", err.Error())
 		return
 	}
 
-	// Set all requested breakpoints.
+	for i, want := range request.Arguments.Breakpoints {
+		reqString := fmt.Sprintf("%s Line=%d Column=%d", sourceRequestPrefix, want.Line, want.Column)
+		if _, ok := existingBps[reqString]; ok {
+			continue
+		}
+
+		var got *api.Breakpoint
+		var err error
+		if _, ok := bpAdded[reqString]; ok {
+			err = fmt.Errorf("Breakpoint exists at %q, line: %d, column: %d", request.Arguments.Source.Path, want.Line, want.Column)
+		} else {
+			// Create new breakpoints.
+			got, err = s.debugger.CreateBreakpoint(
+				&api.Breakpoint{File: serverPath, Line: want.Line, Cond: want.Condition, HitCond: want.HitCondition, Name: reqString})
+			bpAdded[reqString] = struct{}{}
+		}
+
+		updateBreakpointsResponse(breakpoints, i, err, got, clientPath)
+	}
 	response := &dap.SetBreakpointsResponse{Response: *newResponse(request.Request)}
-	response.Body.Breakpoints = make([]dap.Breakpoint, len(request.Arguments.Breakpoints))
+	response.Body.Breakpoints = breakpoints
+
+	s.send(response)
+}
+
+func updateBreakpointsResponse(breakpoints []dap.Breakpoint, i int, err error, got *api.Breakpoint, path string) {
+	breakpoints[i].Verified = (err == nil)
+	if err != nil {
+		breakpoints[i].Message = err.Error()
+	} else {
+		breakpoints[i].Id = got.ID
+		breakpoints[i].Line = got.Line
+		breakpoints[i].Source = dap.Source{Name: filepath.Base(path), Path: path}
+	}
+}
+
+// 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) {
+	if s.noDebugProcess != nil {
+		s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", "running in noDebug mode")
+		return
+	}
+
+	// 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. To maintain state (for hit count conditions)
+	// we want to amend existing breakpoints.
+	//
+	// See https://github.com/golang/vscode-go/issues/163 for details.
+	// If a breakpoint:
+	// -- exists and not in request => ClearBreakpoint
+	// -- exists and in request => AmendBreakpoint
+	// -- doesn't exist and in request => SetBreakpoint
+
+	// Get all existing function breakpoints.
+	existingBps := s.getMatchingBreakpoints(functionBpPrefix)
+	bpAdded := make(map[string]struct{}, len(existingBps))
+	for _, bp := range existingBps {
+		existingBps[bp.Name] = bp
+	}
+
+	// Amend any existing breakpoints.
+	breakpoints := make([]dap.Breakpoint, len(request.Arguments.Breakpoints))
 	for i, want := range request.Arguments.Breakpoints {
-		got, err := s.debugger.CreateBreakpoint(
-			&api.Breakpoint{File: serverPath, Line: want.Line, Cond: want.Condition})
-		response.Body.Breakpoints[i].Verified = (err == nil)
-		if err != nil {
-			response.Body.Breakpoints[i].Line = want.Line
-			response.Body.Breakpoints[i].Message = err.Error()
+		reqString := fmt.Sprintf("%s Name=%s", functionBpPrefix, want.Name)
+		var err error
+		got, ok := existingBps[reqString]
+		if !ok {
+			// Skip if the breakpoint does not already exist.
+			// These will be created after deleting existing
+			// breakpoints to avoid conflicts.
+			continue
+		}
+		if _, ok := bpAdded[reqString]; ok {
+			err = fmt.Errorf("Breakpoint exists at function %q", want.Name)
 		} else {
-			response.Body.Breakpoints[i].Id = got.ID
-			response.Body.Breakpoints[i].Line = got.Line
-			response.Body.Breakpoints[i].Source = dap.Source{Name: request.Arguments.Source.Name, Path: clientPath}
+			got.Cond = want.Condition
+			got.HitCond = want.HitCondition
+			err = s.debugger.AmendBreakpoint(got)
+			bpAdded[reqString] = struct{}{}
+		}
+
+		var clientPath string
+		if got != nil {
+			clientPath = s.toClientPath(got.File)
+		}
+		updateBreakpointsResponse(breakpoints, i, err, got, clientPath)
+	}
+
+	// Clear existing breakpoints that were not added.
+	err := s.clearBreakpoints(existingBps, bpAdded)
+	if err != nil {
+		s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", err.Error())
+		return
+	}
+
+	// Create new breakpoints.
+	for i, want := range request.Arguments.Breakpoints {
+		reqString := fmt.Sprintf("%s Name=%s", functionBpPrefix, want.Name)
+		if _, ok := existingBps[reqString]; ok {
+			// Amend existing breakpoints.
+			continue
+		}
+
+		// Set the function breakpoint breakpoint
+		spec, err := locspec.Parse(want.Name)
+		if err != nil {
+			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.
+			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] == '.' {
+			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.
+		var locs []api.Location
+		locs, err = s.debugger.FindLocationSpec(-1, 0, 0, want.Name, spec, true, s.args.substitutePathClientToServer)
+		if err != nil {
+			breakpoints[i].Message = err.Error()
+			continue
+		}
+		if len(locs) == 0 {
+			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)
+			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{Addr: loc.PC, Addrs: loc.PCs, Cond: want.Condition, Name: reqString})
+
+		var clientPath string
+		if got != nil {
+			clientPath = s.toClientPath(got.File)
 		}
+		updateBreakpointsResponse(breakpoints, i, err, got, clientPath)
 	}
+
+	response := &dap.SetFunctionBreakpointsResponse{Response: *newResponse(request.Request)}
+	response.Body.Breakpoints = breakpoints
+
 	s.send(response)
 }
 
+func (s *Server) clearBreakpoints(existingBps map[string]*api.Breakpoint, bpAdded map[string]struct{}) error {
+	for req, bp := range existingBps {
+		if _, ok := bpAdded[req]; ok {
+			continue
+		}
+		_, err := s.debugger.ClearBreakpoint(bp)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (s *Server) getMatchingBreakpoints(prefix string) map[string]*api.Breakpoint {
+	existing := s.debugger.Breakpoints()
+	matchingBps := make(map[string]*api.Breakpoint, len(existing))
+	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 !strings.HasPrefix(bp.Name, prefix) {
+			continue
+		}
+		matchingBps[bp.Name] = bp
+	}
+	return matchingBps
+}
+
 func (s *Server) onSetExceptionBreakpointsRequest(request *dap.SetExceptionBreakpointsRequest) {
 	// Unlike what DAP documentation claims, this request is always sent
 	// even though we specified no filters at initialization. Handle as no-op.
@@ -1957,102 +2157,6 @@ func (s *Server) onRestartRequest(request *dap.RestartRequest) {
 	s.sendNotYetImplementedErrorResponse(request.Request)
 }
 
-// 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) {
-	if s.noDebugProcess != nil {
-		s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", "running in noDebug mode")
-		return
-	}
-
-	// 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.
 // Capability 'supportsStepBack' is not set 'initialize' response.
 func (s *Server) onStepBackRequest(request *dap.StepBackRequest) {
diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go
index 00ff9333..e4505b31 100644
--- a/service/debugger/debugger.go
+++ b/service/debugger/debugger.go
@@ -6,11 +6,13 @@ import (
 	"errors"
 	"fmt"
 	"go/parser"
+	"go/token"
 	"os"
 	"path/filepath"
 	"regexp"
 	"runtime"
 	"sort"
+	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -629,9 +631,6 @@ func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoin
 	)
 
 	if requestedBp.Name != "" {
-		if err = api.ValidBreakpointName(requestedBp.Name); err != nil {
-			return nil, err
-		}
 		if (d.findBreakpointByName(requestedBp.Name) != nil) || (d.findDisabledBreakpointByName(requestedBp.Name) != nil) {
 			return nil, errors.New("breakpoint name already exists")
 		}
@@ -742,9 +741,6 @@ func (d *Debugger) AmendBreakpoint(amend *api.Breakpoint) error {
 	if originals == nil && !disabled {
 		return fmt.Errorf("no breakpoint with ID %d", amend.ID)
 	}
-	if err := api.ValidBreakpointName(amend.Name); err != nil {
-		return err
-	}
 	if !amend.Disabled && disabled { // enable the breakpoint
 		bp, err := d.target.SetBreakpointWithID(amend.ID, amend.Addr)
 		if err != nil {
@@ -789,9 +785,63 @@ func copyBreakpointInfo(bp *proc.Breakpoint, requested *api.Breakpoint) (err err
 	if requested.Cond != "" {
 		bp.Cond, err = parser.ParseExpr(requested.Cond)
 	}
+	bp.HitCond = nil
+	if requested.HitCond != "" {
+		opTok, val, parseErr := parseHitCondition(requested.HitCond)
+		if err == nil {
+			err = parseErr
+		}
+		if parseErr == nil {
+			bp.HitCond = &struct {
+				Op  token.Token
+				Val int
+			}{opTok, val}
+		}
+	}
 	return err
 }
 
+func parseHitCondition(hitCond string) (token.Token, int, error) {
+	// A hit condition can be in the following formats:
+	// - "number"
+	// - "OP number"
+	hitConditionRegex := regexp.MustCompile(`((=|>|<|%|!)+|)( |)((\d|_)+)`)
+
+	match := hitConditionRegex.FindStringSubmatch(strings.TrimSpace(hitCond))
+	if match == nil || len(match) != 6 {
+		return 0, 0, fmt.Errorf("unable to parse breakpoint hit condition: %q\nhit conditions should be of the form \"number\" or \"OP number\"", hitCond)
+	}
+
+	opStr := match[1]
+	var opTok token.Token
+	switch opStr {
+	case "==", "":
+		opTok = token.EQL
+	case ">=":
+		opTok = token.GEQ
+	case "<=":
+		opTok = token.LEQ
+	case ">":
+		opTok = token.GTR
+	case "<":
+		opTok = token.LSS
+	case "%":
+		opTok = token.REM
+	case "!=":
+		opTok = token.NEQ
+	default:
+		return 0, 0, fmt.Errorf("unable to parse breakpoint hit condition: %q\ninvalid operator: %q", hitCond, opStr)
+	}
+
+	numStr := match[4]
+	val, parseErr := strconv.Atoi(numStr)
+	if parseErr != nil {
+		return 0, 0, fmt.Errorf("unable to parse breakpoint hit condition: %q\ninvalid number: %q", hitCond, numStr)
+	}
+
+	return opTok, val, nil
+}
+
 // ClearBreakpoint clears a breakpoint.
 func (d *Debugger) ClearBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoint, error) {
 	d.targetMutex.Lock()
@@ -956,7 +1006,7 @@ func (d *Debugger) CreateWatchpoint(goid, frame, deferredCall int, expr string,
 	if err != nil {
 		return nil, err
 	}
-	if api.ValidBreakpointName(expr) == nil && d.findBreakpointByName(expr) == nil {
+	if d.findBreakpointByName(expr) == nil {
 		bp.Name = expr
 	}
 	return api.ConvertBreakpoint(bp), nil
diff --git a/service/rpc1/server.go b/service/rpc1/server.go
index 558594dd..2844dc82 100644
--- a/service/rpc1/server.go
+++ b/service/rpc1/server.go
@@ -103,6 +103,9 @@ func (s *RPCServer) ListBreakpoints(arg interface{}, breakpoints *[]*api.Breakpo
 }
 
 func (s *RPCServer) CreateBreakpoint(bp, newBreakpoint *api.Breakpoint) error {
+	if err := api.ValidBreakpointName(bp.Name); err != nil {
+		return err
+	}
 	createdbp, err := s.debugger.CreateBreakpoint(bp)
 	if err != nil {
 		return err
@@ -139,6 +142,9 @@ func (s *RPCServer) ClearBreakpointByName(name string, breakpoint *api.Breakpoin
 
 func (s *RPCServer) AmendBreakpoint(amend *api.Breakpoint, unused *int) error {
 	*unused = 0
+	if err := api.ValidBreakpointName(amend.Name); err != nil {
+		return err
+	}
 	return s.debugger.AmendBreakpoint(amend)
 }
 
diff --git a/service/rpc2/server.go b/service/rpc2/server.go
index 3d5edb1c..05dbfbda 100644
--- a/service/rpc2/server.go
+++ b/service/rpc2/server.go
@@ -249,6 +249,9 @@ type CreateBreakpointOut struct {
 //
 // - Otherwise the value specified by arg.Breakpoint.Addr will be used.
 func (s *RPCServer) CreateBreakpoint(arg CreateBreakpointIn, out *CreateBreakpointOut) error {
+	if err := api.ValidBreakpointName(arg.Breakpoint.Name); err != nil {
+		return err
+	}
 	createdbp, err := s.debugger.CreateBreakpoint(&arg.Breakpoint)
 	if err != nil {
 		return err
@@ -314,6 +317,9 @@ func (s *RPCServer) ToggleBreakpoint(arg ToggleBreakpointIn, out *ToggleBreakpoi
 		}
 	}
 	bp.Disabled = !bp.Disabled
+	if err := api.ValidBreakpointName(bp.Name); err != nil {
+		return err
+	}
 	if err := s.debugger.AmendBreakpoint(bp); err != nil {
 		return err
 	}
@@ -334,6 +340,9 @@ type AmendBreakpointOut struct {
 //
 // arg.Breakpoint.ID must be a valid breakpoint ID
 func (s *RPCServer) AmendBreakpoint(arg AmendBreakpointIn, out *AmendBreakpointOut) error {
+	if err := api.ValidBreakpointName(arg.Breakpoint.Name); err != nil {
+		return err
+	}
 	return s.debugger.AmendBreakpoint(&arg.Breakpoint)
 }
 


Test patch:
diff --git a/pkg/proc/proc_test.go b/pkg/proc/proc_test.go
index ed9aa16c..d2cafaf1 100644
--- a/pkg/proc/proc_test.go
+++ b/pkg/proc/proc_test.go
@@ -1685,6 +1685,78 @@ func TestCondBreakpointError(t *testing.T) {
 	})
 }
 
+func TestHitCondBreakpointEQ(t *testing.T) {
+	withTestProcess("break", t, func(p *proc.Target, fixture protest.Fixture) {
+		bp := setFileBreakpoint(p, t, fixture.Source, 7)
+		bp.HitCond = &struct {
+			Op  token.Token
+			Val int
+		}{token.EQL, 3}
+
+		assertNoError(p.Continue(), t, "Continue()")
+		ivar := evalVariable(p, t, "i")
+
+		i, _ := constant.Int64Val(ivar.Value)
+		if i != 3 {
+			t.Fatalf("Stoppend on wrong hitcount %d\n", i)
+		}
+
+		err := p.Continue()
+		if _, exited := err.(proc.ErrProcessExited); !exited {
+			t.Fatalf("Unexpected error on Continue(): %v", err)
+		}
+	})
+}
+
+func TestHitCondBreakpointGEQ(t *testing.T) {
+	protest.AllowRecording(t)
+	withTestProcess("break", t, func(p *proc.Target, fixture protest.Fixture) {
+		bp := setFileBreakpoint(p, t, fixture.Source, 7)
+		bp.HitCond = &struct {
+			Op  token.Token
+			Val int
+		}{token.GEQ, 3}
+
+		for it := 3; it <= 10; it++ {
+			assertNoError(p.Continue(), t, "Continue()")
+			ivar := evalVariable(p, t, "i")
+
+			i, _ := constant.Int64Val(ivar.Value)
+			if int(i) != it {
+				t.Fatalf("Stoppend on wrong hitcount %d\n", i)
+			}
+		}
+
+		assertNoError(p.Continue(), t, "Continue()")
+	})
+}
+
+func TestHitCondBreakpointREM(t *testing.T) {
+	protest.AllowRecording(t)
+	withTestProcess("break", t, func(p *proc.Target, fixture protest.Fixture) {
+		bp := setFileBreakpoint(p, t, fixture.Source, 7)
+		bp.HitCond = &struct {
+			Op  token.Token
+			Val int
+		}{token.REM, 2}
+
+		for it := 2; it <= 10; it += 2 {
+			assertNoError(p.Continue(), t, "Continue()")
+			ivar := evalVariable(p, t, "i")
+
+			i, _ := constant.Int64Val(ivar.Value)
+			if int(i) != it {
+				t.Fatalf("Stoppend on wrong hitcount %d\n", i)
+			}
+		}
+
+		err := p.Continue()
+		if _, exited := err.(proc.ErrProcessExited); !exited {
+			t.Fatalf("Unexpected error on Continue(): %v", err)
+		}
+	})
+}
+
 func TestIssue356(t *testing.T) {
 	// slice with a typedef does not get printed correctly
 	protest.AllowRecording(t)
diff --git a/service/dap/daptest/client.go b/service/dap/daptest/client.go
index cb2036b7..2b897241 100644
--- a/service/dap/daptest/client.go
+++ b/service/dap/daptest/client.go
@@ -235,7 +235,6 @@ func (c *Client) SetConditionalBreakpointsRequest(file string, lines []int, cond
 			Path: file,
 		},
 		Breakpoints: make([]dap.SourceBreakpoint, len(lines)),
-		//sourceModified: false,
 	}
 	for i, l := range lines {
 		request.Arguments.Breakpoints[i].Line = l
@@ -247,6 +246,26 @@ func (c *Client) SetConditionalBreakpointsRequest(file string, lines []int, cond
 	c.send(request)
 }
 
+// SetBreakpointsRequest sends a 'setBreakpoints' request with conditions.
+func (c *Client) SetHitConditionalBreakpointsRequest(file string, lines []int, conditions map[int]string) {
+	request := &dap.SetBreakpointsRequest{Request: *c.newRequest("setBreakpoints")}
+	request.Arguments = dap.SetBreakpointsArguments{
+		Source: dap.Source{
+			Name: filepath.Base(file),
+			Path: file,
+		},
+		Breakpoints: make([]dap.SourceBreakpoint, len(lines)),
+	}
+	for i, l := range lines {
+		request.Arguments.Breakpoints[i].Line = l
+		cond, ok := conditions[l]
+		if ok {
+			request.Arguments.Breakpoints[i].HitCondition = cond
+		}
+	}
+	c.send(request)
+}
+
 // SetExceptionBreakpointsRequest sends a 'setExceptionBreakpoints' request.
 func (c *Client) SetExceptionBreakpointsRequest() {
 	request := &dap.SetBreakpointsRequest{Request: *c.newRequest("setExceptionBreakpoints")}
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index 21fac004..75f5c6dd 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -1865,6 +1865,12 @@ func checkBreakpoints(t *testing.T, client *daptest.Client, bps []Breakpoint, br
 		return
 	}
 	for i, bp := range breakpoints {
+		if bps[i].line < 0 && !bps[i].verified {
+			if bp.Verified != bps[i].verified || !strings.Contains(bp.Message, bps[i].msgPrefix) {
+				t.Errorf("got breakpoints[%d] = %#v, \nwant %#v", i, bp, bps[i])
+			}
+			continue
+		}
 		if bp.Line != bps[i].line || bp.Verified != bps[i].verified || bp.Source.Path != bps[i].path ||
 			!strings.HasPrefix(bp.Message, bps[i].msgPrefix) {
 			t.Errorf("got breakpoints[%d] = %#v, \nwant %#v", i, bp, bps[i])
@@ -1903,7 +1909,7 @@ func TestSetBreakpoint(t *testing.T) {
 
 					// Set another breakpoint inside the loop in loop(), twice to trigger error
 					client.SetBreakpointsRequest(fixture.Source, []int{8, 8})
-					expectSetBreakpointsResponse(t, client, []Breakpoint{{8, fixture.Source, true, ""}, {8, "", false, "Breakpoint exists"}})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{8, fixture.Source, true, ""}, {-1, "", false, "Breakpoint exists"}})
 
 					// Continue into the loop
 					client.ContinueRequest(1)
@@ -1942,7 +1948,7 @@ func TestSetBreakpoint(t *testing.T) {
 
 					// Set at a line without a statement
 					client.SetBreakpointsRequest(fixture.Source, []int{1000})
-					expectSetBreakpointsResponse(t, client, []Breakpoint{{1000, "", false, "could not find statement"}}) // all cleared, none set
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{-1, "", false, "could not find statement"}}) // all cleared, none set
 				},
 				// The program has an infinite loop, so we must kill it by disconnecting.
 				disconnect: true,
@@ -1978,7 +1984,7 @@ func TestSetFunctionBreakpoints(t *testing.T) {
 							return
 						}
 						for i, bp := range got.Body.Breakpoints {
-							if bps[i].line < 0 {
+							if bps[i].line < 0 && !bps[i].verified {
 								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])
 								}
@@ -2361,6 +2367,71 @@ func TestSetFunctionBreakpointWhileRunning(t *testing.T) {
 	})
 }
 
+func TestHitConditionBreakpoints(t *testing.T) {
+	runTest(t, "break", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{4},
+			[]onBreakpoint{{
+				execute: func() {
+					client.SetHitConditionalBreakpointsRequest(fixture.Source, []int{7}, map[int]string{7: "4"})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{7, fixture.Source, true, ""}})
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					checkStop(t, client, 1, "main.main", 7)
+
+					// Check that we are stopped at the correct value of i.
+					client.VariablesRequest(1001)
+					locals := client.ExpectVariablesResponse(t)
+					checkVarExact(t, locals, 0, "i", "i", "4", "int", noChildren)
+
+					// Change the hit condition.
+					client.SetHitConditionalBreakpointsRequest(fixture.Source, []int{7}, map[int]string{7: "% 2"})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{7, fixture.Source, true, ""}})
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					checkStop(t, client, 1, "main.main", 7)
+
+					// Check that we are stopped at the correct value of i.
+					client.VariablesRequest(1001)
+					locals = client.ExpectVariablesResponse(t)
+					checkVarExact(t, locals, 0, "i", "i", "6", "int", noChildren)
+
+					// Expect an error if an assignment is passed.
+					client.SetHitConditionalBreakpointsRequest(fixture.Source, []int{7}, map[int]string{7: "= 2"})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{-1, "", false, ""}})
+
+					// Change the hit condition.
+					client.SetHitConditionalBreakpointsRequest(fixture.Source, []int{7}, map[int]string{7: "< 8"})
+					expectSetBreakpointsResponse(t, client, []Breakpoint{{7, fixture.Source, true, ""}})
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					checkStop(t, client, 1, "main.main", 7)
+
+					// Check that we are stopped at the correct value of i.
+					client.VariablesRequest(1001)
+					locals = client.ExpectVariablesResponse(t)
+					checkVarExact(t, locals, 0, "i", "i", "7", "int", noChildren)
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+
+					client.ExpectTerminatedEvent(t)
+				},
+				disconnect: false,
+			}})
+	})
+}
+
 // TestLaunchSubstitutePath sets a breakpoint using a path
 // that does not exist and expects the substitutePath attribute
 // in the launch configuration to take care of the mapping.