Repository: go-delve/delve
Pull request #2553 (2021-06-25 19:08:52)

service/debugger: improve CreateBreakpoint docs
The existing documentation was a little light on details. This patch
gives more insight into how the argument to this function is used and
interpreted.


Patch:
diff --git a/.gitignore b/.gitignore
index b117424d..f6073ada 100644
--- a/.gitignore
+++ b/.gitignore
@@ -9,3 +9,4 @@ localtests
 .idea
 *.iml
 .teamcity/target
+.vscode
diff --git a/_fixtures/fatalerror.go b/_fixtures/fatalerror.go
new file mode 100644
index 00000000..ad1e9bfa
--- /dev/null
+++ b/_fixtures/fatalerror.go
@@ -0,0 +1,6 @@
+package main
+
+func main() {
+	var f func()
+	go f()
+}
diff --git a/pkg/proc/gdbserial/gdbserver.go b/pkg/proc/gdbserial/gdbserver.go
index 12123a63..d2312c15 100644
--- a/pkg/proc/gdbserial/gdbserver.go
+++ b/pkg/proc/gdbserial/gdbserver.go
@@ -1853,8 +1853,11 @@ func (t *gdbThread) SetReg(regNum uint64, reg *op.DwarfRegister) error {
 	gdbreg, ok := t.regs.regs[regName]
 	if !ok && strings.HasPrefix(regName, "xmm") {
 		// XMMn and YMMn are the same amd64 register (in different sizes), if we
-		// don't find XMMn try YMMn instead.
+		// don't find XMMn try YMMn or ZMMn instead.
 		gdbreg, ok = t.regs.regs["y"+regName[1:]]
+		if !ok {
+			gdbreg, ok = t.regs.regs["z"+regName[1:]]
+		}
 	}
 	if !ok {
 		return fmt.Errorf("could not set register %s: not found", regName)
@@ -1913,8 +1916,16 @@ func (regs *gdbRegisters) Slice(floatingPoint bool) ([]proc.Register, error) {
 
 			value := regs.regs[reginfo.Name].value
 			xmmName := "x" + reginfo.Name[1:]
-			r = proc.AppendBytesRegister(r, strings.ToUpper(xmmName), value[:16])
-			r = proc.AppendBytesRegister(r, strings.ToUpper(reginfo.Name), value[16:])
+			r = proc.AppendBytesRegister(r, strings.ToUpper(xmmName), value)
+
+		case reginfo.Bitsize == 512:
+			if !strings.HasPrefix(strings.ToLower(reginfo.Name), "zmm") || !floatingPoint {
+				continue
+			}
+
+			value := regs.regs[reginfo.Name].value
+			xmmName := "x" + reginfo.Name[1:]
+			r = proc.AppendBytesRegister(r, strings.ToUpper(xmmName), value)
 		}
 	}
 	return r, nil
diff --git a/service/dap/server.go b/service/dap/server.go
index 2514b3aa..cad8a880 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -29,6 +29,7 @@ import (
 	"sync"
 
 	"github.com/go-delve/delve/pkg/gobuild"
+	"github.com/go-delve/delve/pkg/goversion"
 	"github.com/go-delve/delve/pkg/locspec"
 	"github.com/go-delve/delve/pkg/logflags"
 	"github.com/go-delve/delve/pkg/proc"
@@ -2509,14 +2510,36 @@ func (s *Server) onExceptionInfoRequest(request *dap.ExceptionInfoRequest) {
 	if bpState != nil && bpState.Breakpoint != nil && (bpState.Breakpoint.Name == proc.FatalThrow || bpState.Breakpoint.Name == proc.UnrecoveredPanic) {
 		switch bpState.Breakpoint.Name {
 		case proc.FatalThrow:
-			// TODO(suzmue): add the fatal throw reason to body.Description.
 			body.ExceptionId = "fatal error"
+			// Attempt to get the value of the throw reason.
+			// This is not currently working for Go 1.16 or 1.17: https://github.com/golang/go/issues/46425.
+			handleError := func(err error) {
+				if err != nil {
+					body.Description = fmt.Sprintf("Error getting throw reason: %s", err.Error())
+				}
+				if goversion.ProducerAfterOrEqual(s.debugger.TargetGoVersion(), 1, 16) {
+					body.Description = "Throw reason unavailable, see https://github.com/golang/go/issues/46425"
+				}
+			}
+
+			exprVar, err := s.debugger.EvalVariableInScope(goroutineID, 1, 0, "s", DefaultLoadConfig)
+			if err == nil {
+				if exprVar.Value != nil {
+					body.Description = exprVar.Value.String()
+				} else {
+					handleError(exprVar.Unreadable)
+				}
+			} else {
+				handleError(err)
+			}
 		case proc.UnrecoveredPanic:
 			body.ExceptionId = "panic"
 			// Attempt to get the value of the panic message.
 			exprVar, err := s.debugger.EvalVariableInScope(goroutineID, 0, 0, "(*msgs).arg.(data)", DefaultLoadConfig)
 			if err == nil {
 				body.Description = exprVar.Value.String()
+			} else {
+				body.Description = fmt.Sprintf("Error getting panic message: %s", err.Error())
 			}
 		}
 	} else {
@@ -2545,7 +2568,7 @@ func (s *Server) onExceptionInfoRequest(request *dap.ExceptionInfoRequest) {
 	}
 
 	frames, err := s.debugger.Stacktrace(goroutineID, s.args.stackTraceDepth, 0)
-	if err == nil && len(frames) > 0 {
+	if err == nil {
 		apiFrames, err := s.debugger.ConvertStacktrace(frames, nil)
 		if err == nil {
 			var buf bytes.Buffer
@@ -2562,6 +2585,8 @@ func (s *Server) onExceptionInfoRequest(request *dap.ExceptionInfoRequest) {
 			})
 			body.Details.StackTrace = buf.String()
 		}
+	} else {
+		body.Details.StackTrace = fmt.Sprintf("Error getting stack trace: %s", err.Error())
 	}
 	response := &dap.ExceptionInfoResponse{
 		Response: *newResponse(request.Request),
diff --git a/service/debugger/debugger.go b/service/debugger/debugger.go
index 6b3d29c2..ef91db46 100644
--- a/service/debugger/debugger.go
+++ b/service/debugger/debugger.go
@@ -237,6 +237,12 @@ func (d *Debugger) checkGoVersion() error {
 	return goversion.Compatible(producer)
 }
 
+func (d *Debugger) TargetGoVersion() string {
+	d.targetMutex.Lock()
+	defer d.targetMutex.Unlock()
+	return d.target.BinInfo().Producer()
+}
+
 // Launch will start a process with the given args and working directory.
 func (d *Debugger) Launch(processArgs []string, wd string) (*proc.Target, error) {
 	if err := verifyBinaryFormat(processArgs[0]); err != nil {
@@ -620,7 +626,33 @@ func (d *Debugger) state(retLoadCfg *proc.LoadConfig) (*api.DebuggerState, error
 	return state, nil
 }
 
-// CreateBreakpoint creates a breakpoint.
+// CreateBreakpoint creates a breakpoint using information from the provided `requestedBp`.
+// This function accepts several different ways of specifying where and how to create the
+// breakpoint that has been requested. Any error encountered during the attempt to set the
+// breakpoint will be returned to the caller.
+//
+// The ways of specifying a breakpoint are listed below in the order they are considered by
+// this function:
+//
+// - If requestedBp.TraceReturn is true then it is expected that
+// requestedBp.Addrs will contain the list of return addresses
+// supplied by the caller.
+//
+// - If requestedBp.File is not an empty string the breakpoint
+// will be created on the specified file:line location
+//
+// - If requestedBp.FunctionName is not an empty string
+// the breakpoint will be created on the specified function:line
+// location.
+//
+// - If requestedBp.Addrs is filled it will create a logical breakpoint
+// corresponding to all specified addresses.
+//
+// - Otherwise the value specified by arg.Breakpoint.Addr will be used.
+//
+// Note that this method will use the first successful method in order to
+// create a breakpoint, so mixing different fields will not result is multiple
+// breakpoints being set.
 func (d *Debugger) CreateBreakpoint(requestedBp *api.Breakpoint) (*api.Breakpoint, error) {
 	d.targetMutex.Lock()
 	defer d.targetMutex.Unlock()
diff --git a/service/rpc2/client.go b/service/rpc2/client.go
index 314274fd..1f3352e3 100644
--- a/service/rpc2/client.go
+++ b/service/rpc2/client.go
@@ -226,6 +226,10 @@ func (c *RPCClient) GetBreakpointByName(name string) (*api.Breakpoint, error) {
 	return &out.Breakpoint, err
 }
 
+// CreateBreakpoint will send a request to the RPC server to create a breakpoint.
+// Please refer to the documentation for `Debugger.CreateBreakpoint` for a description of how
+// the requested breakpoint parameters are interpreted and used:
+// https://pkg.go.dev/github.com/go-delve/delve/service/debugger#Debugger.CreateBreakpoint
 func (c *RPCClient) CreateBreakpoint(breakPoint *api.Breakpoint) (*api.Breakpoint, error) {
 	var out CreateBreakpointOut
 	err := c.call("CreateBreakpoint", CreateBreakpointIn{*breakPoint}, &out)
diff --git a/service/rpc2/server.go b/service/rpc2/server.go
index 05dbfbda..8e30e468 100644
--- a/service/rpc2/server.go
+++ b/service/rpc2/server.go
@@ -235,19 +235,10 @@ type CreateBreakpointOut struct {
 	Breakpoint api.Breakpoint
 }
 
-// CreateBreakpoint creates a new breakpoint.
-//
-// - If arg.Breakpoint.File is not an empty string the breakpoint
-// will be created on the specified file:line location
-//
-// - If arg.Breakpoint.FunctionName is not an empty string
-// the breakpoint will be created on the specified function:line
-// location.
-//
-// - If arg.Breakpoint.Addrs is filled it will create a logical breakpoint
-// corresponding to all specified addresses.
-//
-// - Otherwise the value specified by arg.Breakpoint.Addr will be used.
+// CreateBreakpoint creates a new breakpoint. The client is expected to populate `CreateBreakpointIn`
+// with an `api.Breakpoint` struct describing where to set the breakpoing. For more information on
+// how to properly request a breakpoint via the `api.Breakpoint` struct see the documentation for
+// `debugger.CreateBreakpoint` here: https://pkg.go.dev/github.com/go-delve/delve/service/debugger#Debugger.CreateBreakpoint.
 func (s *RPCServer) CreateBreakpoint(arg CreateBreakpointIn, out *CreateBreakpointOut) error {
 	if err := api.ValidBreakpointName(arg.Breakpoint.Name); err != nil {
 		return err


Test patch:
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index c615d86f..220685d0 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -3490,7 +3490,6 @@ func TestBadAccess(t *testing.T) {
 						if eInfo.Body.ExceptionId != "runtime error" || !strings.HasPrefix(eInfo.Body.Description, errorPrefix) {
 							t.Errorf("\ngot  %#v\nwant ExceptionId=\"runtime error\" Text=\"%s\"", eInfo, errorPrefix)
 						}
-
 					}
 
 					client.ContinueRequest(1)
@@ -3602,6 +3601,41 @@ func TestPanicBreakpointOnNext(t *testing.T) {
 }
 
 func TestFatalThrowBreakpoint(t *testing.T) {
+	runTest(t, "fatalerror", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client, "launch",
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{3},
+			[]onBreakpoint{{
+				execute: func() {
+					checkStop(t, client, 1, "main.main", 3)
+
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+
+					se := client.ExpectStoppedEvent(t)
+					if se.Body.ThreadId != 1 || se.Body.Reason != "exception" || se.Body.Description != "fatal error" {
+						t.Errorf("\ngot  %#v\nwant ThreadId=1 Reason=\"exception\" Description=\"fatal error\"", se)
+					}
+
+					// TODO(suzmue): Enable this test for 1.17 when https://github.com/golang/go/issues/46425 is fixed.
+					errorPrefix := "\"go of nil func value\""
+					if goversion.VersionAfterOrEqual(runtime.Version(), 1, 16) {
+						errorPrefix = "Throw reason unavailable, see https://github.com/golang/go/issues/46425"
+					}
+					client.ExceptionInfoRequest(1)
+					eInfo := client.ExpectExceptionInfoResponse(t)
+					if eInfo.Body.ExceptionId != "fatal error" || !strings.HasPrefix(eInfo.Body.Description, errorPrefix) {
+						t.Errorf("\ngot  %#v\nwant ExceptionId=\"runtime error\" Text=%s", eInfo, errorPrefix)
+					}
+
+				},
+				disconnect: true,
+			}})
+	})
 	runTest(t, "testdeadlock", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSessionWithBPs(t, client, "launch",
 			// Launch
@@ -3621,6 +3655,10 @@ func TestFatalThrowBreakpoint(t *testing.T) {
 					if se.Body.Reason != "exception" || se.Body.Description != "fatal error" {
 						t.Errorf("\ngot  %#v\nwant Reason=\"exception\" Description=\"fatal error\"", se)
 					}
+
+					// TODO(suzmue): Get the exception info for the thread and check the description
+					// includes "all goroutines are asleep - deadlock!".
+					// Stopped events with no selected goroutines need to be supported to test deadlock.
 				},
 				disconnect: true,
 			}})
diff --git a/service/test/integration2_test.go b/service/test/integration2_test.go
index a59e13df..bfdde103 100644
--- a/service/test/integration2_test.go
+++ b/service/test/integration2_test.go
@@ -1565,7 +1565,7 @@ func TestClientServer_FpRegisters(t *testing.T) {
 			if regtest.name == "XMM11" && !avx2 {
 				continue
 			}
-			if regtest.name == "XMM12" && !avx512 {
+			if regtest.name == "XMM12" && (!avx512 || testBackend == "rr") {
 				continue
 			}
 			found := false