pkg/proc: Prefer throw instead of fatalthrow (go-delve#2616)

* pkg/proc: Prefer throw instead of fatalthrow

Currently there is a breakpoint set at runtime.fatalthrow to catch any
situation where the runtime crashes (e.g. deadlock).
When we do this, we go up a frame in order to parse the crash reason.
The problem is that there is no guarentee the "s" variable we attempt to
parse will still be considered "live".
Since runtime.fatalthrow is never called directly, set a breakpoint on
runtime.throw instead and prevent having
to search up a stack frame in order to
get the throw reason.

Fixes go-delve#2602

* service/dap: Fix TestFatalThrowBreakpoint

* Reenable TestFatalThrow DAP test

* service/dap: Don't skip test on < 1.17

* service/dap: Update test constraint for 1.16

* pkg/proc: Reinstate runtime.fatalthrow as switchstack exception

pkg/proc/amd64_arch.go

@@ -219,7 +219,7 @@ func amd64SwitchStack(it *stackIterator, _ *op.DwarfRegisters) bool {
 		return true
 
 	default:
-		if it.systemstack && it.top && it.g != nil && strings.HasPrefix(it.frame.Current.Fn.Name, "runtime.") && it.frame.Current.Fn.Name != "runtime.fatalthrow" {
+		if it.systemstack && it.top && it.g != nil && strings.HasPrefix(it.frame.Current.Fn.Name, "runtime.") && it.frame.Current.Fn.Name != "runtime.throw" && it.frame.Current.Fn.Name != "runtime.fatalthrow" {
 			// The runtime switches to the system stack in multiple places.
 			// This usually happens through a call to runtime.systemstack but there
 			// are functions that switch to the system stack manually (for example
@@ -228,7 +228,7 @@ func amd64SwitchStack(it *stackIterator, _ *op.DwarfRegisters) bool {
 			// calls we switch directly to the goroutine stack if we detect that the
 			// function at the top of the stack is a runtime function.
 			//
-			// The function "runtime.fatalthrow" is deliberately excluded from this
+			// The function "runtime.throw" is deliberately excluded from this
 			// because it can end up in the stack during a cgo call and switching to
 			// the goroutine stack will exclude all the C functions from the stack
 			// trace.

pkg/proc/arm64_arch.go

@@ -156,7 +156,7 @@ func arm64SwitchStack(it *stackIterator, callFrameRegs *op.DwarfRegisters) bool
 			it.pc = newlr
 			return true
 		default:
-			if it.systemstack && it.top && it.g != nil && strings.HasPrefix(it.frame.Current.Fn.Name, "runtime.") && it.frame.Current.Fn.Name != "runtime.fatalthrow" {
+			if it.systemstack && it.top && it.g != nil && strings.HasPrefix(it.frame.Current.Fn.Name, "runtime.") && it.frame.Current.Fn.Name != "runtime.throw" && it.frame.Current.Fn.Name != "runtime.fatalthrow" {
 				// The runtime switches to the system stack in multiple places.
 				// This usually happens through a call to runtime.systemstack but there
 				// are functions that switch to the system stack manually (for example

pkg/proc/i386_arch.go

@@ -148,7 +148,7 @@ func i386SwitchStack(it *stackIterator, _ *op.DwarfRegisters) bool {
 		return true
 
 	default:
-		if it.systemstack && it.top && it.g != nil && strings.HasPrefix(it.frame.Current.Fn.Name, "runtime.") && it.frame.Current.Fn.Name != "runtime.fatalthrow" {
+		if it.systemstack && it.top && it.g != nil && strings.HasPrefix(it.frame.Current.Fn.Name, "runtime.") && it.frame.Current.Fn.Name != "runtime.throw" && it.frame.Current.Fn.Name != "runtime.fatalthrow" {
 			// The runtime switches to the system stack in multiple places.
 			// This usually happens through a call to runtime.systemstack but there
 			// are functions that switch to the system stack manually (for example
@@ -157,7 +157,7 @@ func i386SwitchStack(it *stackIterator, _ *op.DwarfRegisters) bool {
 			// calls we switch directly to the goroutine stack if we detect that the
 			// function at the top of the stack is a runtime function.
 			//
-			// The function "runtime.fatalthrow" is deliberately excluded from this
+			// The function "runtime.throw" is deliberately excluded from this
 			// because it can end up in the stack during a cgo call and switching to
 			// the goroutine stack will exclude all the C functions from the stack
 			// trace.

pkg/proc/proc_test.go

@@ -1354,14 +1354,7 @@ func TestThreadFrameEvaluation(t *testing.T) {
 		scope, err := proc.ConvertEvalScope(p, 0, 0, 0)
 		assertNoError(err, t, "ConvertEvalScope() on frame 0")
 		_, err = scope.EvalVariable("s", normalLoadConfig)
-		if err == nil {
-			t.Errorf("expected error for EvalVariable(\"s\") on frame 0")
-		}
-
-		scope, err = proc.ConvertEvalScope(p, 0, 1, 0)
-		assertNoError(err, t, "ConvertEvalScope() on frame 1")
-		_, err = scope.EvalVariable("s", normalLoadConfig)
-		assertNoError(err, t, "EvalVariable(\"s\") on frame 1")
+		assertNoError(err, t, "EvalVariable(\"s\") on frame 0")
 	})
 }
 

pkg/proc/target.go

@@ -375,7 +375,7 @@ func (t *Target) createUnrecoveredPanicBreakpoint() {
 
 // createFatalThrowBreakpoint creates the a breakpoint as runtime.fatalthrow.
 func (t *Target) createFatalThrowBreakpoint() {
-	fatalpcs, err := FindFunctionLocation(t.Process, "runtime.fatalthrow", 0)
+	fatalpcs, err := FindFunctionLocation(t.Process, "runtime.throw", 0)
 	if err == nil {
 		bp, err := t.SetBreakpointWithID(fatalThrowID, fatalpcs[0])
 		if err == nil {

service/dap/server.go

@@ -2731,7 +2731,7 @@ func (s *Server) onExceptionInfoRequest(request *dap.ExceptionInfoRequest) {
 }
 
 func (s *Server) throwReason(goroutineID int) (string, error) {
-	return s.getExprString("s", goroutineID, 1)
+	return s.getExprString("s", goroutineID, 0)
 }
 
 func (s *Server) panicReason(goroutineID int) (string, error) {

service/dap/server_test.go

@@ -3689,10 +3689,6 @@ func TestPanicBreakpointOnNext(t *testing.T) {
 }
 
 func TestFatalThrowBreakpoint(t *testing.T) {
-	// This is not currently flaky for Go 1.17 see https://github.com/golang/go/issues/46425.
-	if goversion.VersionAfterOrEqual(runtime.Version(), 1, 17) {
-		t.Skip()
-	}
 	runTest(t, "fatalerror", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSessionWithBPs(t, client, "launch",
 			// Launch
@@ -3750,9 +3746,9 @@ func TestFatalThrowBreakpoint(t *testing.T) {
 					client.ContinueRequest(1)
 					client.ExpectContinueResponse(t)
 
-					// TODO(suzmue): Enable this test for 1.17 when https://github.com/golang/go/issues/46425 is fixed.
+					// This does not work for Go 1.16 so skip by detecting versions before or after 1.16.
 					var text string
-					if !goversion.VersionAfterOrEqual(runtime.Version(), 1, 16) {
+					if !goversion.VersionAfterOrEqual(runtime.Version(), 1, 16) || goversion.VersionAfterOrEqual(runtime.Version(), 1, 17) {
 						text = "\"all goroutines are asleep - deadlock!\""
 					}
 					se := client.ExpectStoppedEvent(t)