-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Start showing inlined functions in stack trace #208
Changes from all commits
1111313
36e411d
e004cea
a41982d
f5fead2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,10 @@ | ||
github.com/bitly/go-simplejson v0.5.0 h1:6IH+V8/tVMab511d5bn4M7EwGXZf9Hj6i2xSwkNEM+Y= | ||
github.com/bitly/go-simplejson v0.5.0/go.mod h1:cXHtHw4XUPsvGaxgjIAn8PhEWG9NfngEKAMDJEczWVA= | ||
github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869 h1:DDGfHa7BWjL4YnC6+E63dPcxHo2sUxDIu8g3QgEJdRY= | ||
github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869/go.mod h1:Ekp36dRnpXw/yCqJaO+ZrUyxD+3VXMFFr56k5XYrpB4= | ||
github.com/bitly/go-simplejson v0.5.1 h1:xgwPbetQScXt1gh9BmoJ6j9JMr3TElvuIyjR8pgdoow= | ||
github.com/bitly/go-simplejson v0.5.1/go.mod h1:YOPVLzCfwK14b4Sff3oP1AmGhI9T9Vsg84etUnlyp+Q= | ||
github.com/bugsnag/panicwrap v1.3.4 h1:A6sXFtDGsgU/4BLf5JT0o5uYg3EeKgGx3Sfs+/uk3pU= | ||
github.com/bugsnag/panicwrap v1.3.4/go.mod h1:D/8v3kj0zr8ZAKg1AQ6crr+5VwKN5eIywRkfhyM/+dE= | ||
github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= | ||
github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= | ||
github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= | ||
github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= | ||
github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 h1:iQTw/8FWTuc7uiaSepXwyf3o52HaUYcV+Tu66S3F5GA= | ||
github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0/go.mod h1:1NbS8ALrpOvjt0rHPNLyCIeMtbizbir8U//inJ+zuB8= | ||
github.com/kr/pretty v0.2.1 h1:Fmg33tUaq4/8ym9TJN1x7sLJnHVwhP33CNkpYV/7rwI= | ||
github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= | ||
github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= | ||
github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= | ||
github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= | ||
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= | ||
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package bugsnag_test | |
|
||
import ( | ||
"fmt" | ||
"runtime" | ||
"strings" | ||
"testing" | ||
|
||
|
@@ -61,21 +62,52 @@ func TestStackframesAreSkippedCorrectly(t *testing.T) { | |
defer notifier.AutoNotify() | ||
crash("NaN") | ||
}() | ||
assertStackframesMatch(t, []errors.StackFrame{ | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func4.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func4", File: "notifier_test.go"}, | ||
}) | ||
|
||
var expected []errors.StackFrame | ||
golangVersion := runtime.Version() | ||
// TODO remove this after dropping support for Golang 1.11 | ||
// Golang version < 1.12 cannot unwrap inlined functions correctly. | ||
if strings.HasPrefix(golangVersion, "go1.11") { | ||
expected = append(expected, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func4.1", File: "notifier_test.go"}, //inlined crash func | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func4.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func4", File: "notifier_test.go"}, | ||
) | ||
} else { | ||
expected = append(expected, | ||
errors.StackFrame{Name: "crash", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func4.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func4", File: "notifier_test.go"}, | ||
) | ||
} | ||
|
||
assertStackframesMatch(t, expected) | ||
}) | ||
t.Run("bugsnag.AutoNotify", func(st *testing.T) { | ||
func() { | ||
defer func() { recover() }() | ||
defer bugsnag.AutoNotify() | ||
crash("NaN") | ||
}() | ||
assertStackframesMatch(t, []errors.StackFrame{ | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func5.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func5", File: "notifier_test.go"}, | ||
}) | ||
var expected []errors.StackFrame | ||
golangVersion := runtime.Version() | ||
// TODO remove this after dropping support for Golang 1.11 | ||
// Golang version < 1.12 cannot unwrap inlined functions correctly. | ||
if strings.HasPrefix(golangVersion, "go1.11") { | ||
expected = append(expected, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func5.1", File: "notifier_test.go"}, //inlined crash func | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func5.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func5", File: "notifier_test.go"}, | ||
) | ||
} else { | ||
expected = append(expected, | ||
errors.StackFrame{Name: "crash", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func5.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func5", File: "notifier_test.go"}, | ||
) | ||
} | ||
|
||
assertStackframesMatch(t, expected) | ||
}) | ||
|
||
// Expect the following frames to be present for *.Recover | ||
|
@@ -92,20 +124,50 @@ func TestStackframesAreSkippedCorrectly(t *testing.T) { | |
defer notifier.Recover() | ||
crash("NaN") | ||
}() | ||
assertStackframesMatch(t, []errors.StackFrame{ | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func6.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func6", File: "notifier_test.go"}, | ||
}) | ||
var expected []errors.StackFrame | ||
golangVersion := runtime.Version() | ||
// TODO remove this after dropping support for Golang 1.11 | ||
// Golang version < 1.12 cannot unwrap inlined functions correctly. | ||
if strings.HasPrefix(golangVersion, "go1.11") { | ||
expected = append(expected, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func6.1", File: "notifier_test.go"}, //inlined crash func | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func6.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func6", File: "notifier_test.go"}, | ||
) | ||
} else { | ||
expected = append(expected, | ||
errors.StackFrame{Name: "crash", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func6.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func6", File: "notifier_test.go"}, | ||
) | ||
} | ||
|
||
assertStackframesMatch(t, expected) | ||
}) | ||
t.Run("bugsnag.Recover", func(st *testing.T) { | ||
func() { | ||
defer bugsnag.Recover() | ||
crash("NaN") | ||
}() | ||
assertStackframesMatch(t, []errors.StackFrame{ | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func7.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func7", File: "notifier_test.go"}, | ||
}) | ||
var expected []errors.StackFrame | ||
golangVersion := runtime.Version() | ||
// TODO remove this after dropping support for Golang 1.11 | ||
// Golang version < 1.12 cannot unwrap inlined functions correctly. | ||
if strings.HasPrefix(golangVersion, "go1.11") { | ||
expected = append(expected, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func7.1", File: "notifier_test.go"}, //inlined crash func | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func7.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func7", File: "notifier_test.go"}, | ||
) | ||
} else { | ||
expected = append(expected, | ||
errors.StackFrame{Name: "crash", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func7.1", File: "notifier_test.go"}, | ||
errors.StackFrame{Name: "TestStackframesAreSkippedCorrectly.func7", File: "notifier_test.go"}, | ||
) | ||
} | ||
|
||
assertStackframesMatch(t, expected) | ||
}) | ||
} | ||
|
||
|
@@ -187,6 +249,7 @@ func assertStackframesMatch(t *testing.T, expected []errors.StackFrame) { | |
if strings.HasSuffix(file, expectedFrame.File) && expectedFrame.Name == method { | ||
lastmatch = index | ||
matched++ | ||
break | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also provide rationale for this change in the PR description? Why did it pass before and not now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It passed before because the stack was:
When I added expansion of inlines the stack became:
|
||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include some justification on the PR description for this change? It seems fairly fundamental to all our stacktrace reporting, so I'm surprised it's incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually moving the whole stack to ignore the newest item. With the -1 added the stack for example looks like:
PTR: main.A
STACK[0]: main.B
PTR: main.B
STACK[1]: main.C
PTR: main.C
STACK[2]: main.main
PTR: main.main
STACK[3]: main.main
PTR: runtime.main
STACK[4]: runtime.main
PTR: runtime.goexit
STACK[5]: runtime.goexit
after changing to not modify the uintptr address:
PTR: main.A
STACK[0]: main.A
PTR: main.B
STACK[1]: main.B
PTR: main.C
STACK[2]: main.C
PTR: main.main
STACK[3]: main.main
PTR: runtime.main
STACK[4]: runtime.main
PTR: runtime.goexit
STACK[5]: runtime.goexit