PenaDevops/delve
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
* 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 #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
This commit is contained in:
Derek Parker 2021-07-27 23:58:02 -07:00 committed by GitHub
parent 26e7f67cc4
commit f6681c6090
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 10 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
pkg/proc/amd64_arch.go
View File

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

2
pkg/proc/arm64_arch.go
View File

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

4
pkg/proc/i386_arch.go
View File

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

9
pkg/proc/proc_test.go
View File

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

2
pkg/proc/target.go
View File

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

2
service/dap/server.go
View File

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

8
service/dap/server_test.go
View File

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