Use CallersFrames to build StackFrames (required for go 1.12)

[mkobetic]

Goal

Go 1.12 uses more aggressive function call inlining which requires employing runtime.CallersFrames() to rebuild the function call stack correctly. (see https://golang.org/doc/go1.12#compiler). Included test case demonstrates the problem when trying to reconstruct the stack captured by the github.com/pkg/errors package.

If you run the test with the debug prints uncommented (and without the fix), you'll see that the top frame in the original stack and the top frame in the stack built by StackFrames() don't match. This is because the errors.New() call is (likely) inlined and therefore this part of https://golang.org/pkg/runtime/#FuncForPC applies

If pc represents multiple functions because of inlining, it returns the a *Func describing the innermost function, but with an entry of the outermost function.

$ go test -run StackFrames

/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:31
	github.com/mkobetic/bugsnag-go/errors.boom
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:29
	github.com/mkobetic/bugsnag-go/errors.boom
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:29
	github.com/mkobetic/bugsnag-go/errors.boom
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:29
	github.com/mkobetic/bugsnag-go/errors.boom
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:29
	github.com/mkobetic/bugsnag-go/errors.boom
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:29
	github.com/mkobetic/bugsnag-go/errors.boom
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:12
	github.com/mkobetic/bugsnag-go/errors.Test_StackFrames
/usr/local/go/src/testing/testing.go:865
	testing.tRunner
/usr/local/go/src/runtime/asm_amd64.s:1337
	runtime.goexit

/Users/martin/shopify/go/src/github.com/pkg/errors/errors.go:105 (0x10f9ea0)
	New: stack: callers(),
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:29 (0x10f9f36)
	boom: return boom(depth - 1)
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:29 (0x10f9f36)
	boom: return boom(depth - 1)
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:29 (0x10f9f36)
	boom: return boom(depth - 1)
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:29 (0x10f9f36)
	boom: return boom(depth - 1)
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:29 (0x10f9f36)
	boom: return boom(depth - 1)
/Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:12 (0x10f994f)
	Test_StackFrames: err := boom(5)
/usr/local/go/src/testing/testing.go:865 (0x10b97f0)
	tRunner: fn(t)
/usr/local/go/src/runtime/asm_amd64.s:1337 (0x1057761)
	goexit: BYTE	$0x90	// NOP

--- FAIL: Test_StackFrames (0.00s)
    /Users/martin/go/src/github.com/mkobetic/bugsnag-go/errors/stackframe_test.go:22: top frames don't match
        github.com/mkobetic/bugsnag-go/errors.boom
        github.com/pkg/errors.New
FAIL
FAIL	github.com/mkobetic/bugsnag-go/errors	0.007s
Error: Tests failed.

Design

Use the runtime.CallersFrames() function to reconstruct the stack as documented.

Changeset

Since the use of FuncForPC should be avoided we need to create StackFrames from runtime.Frames instead of PCs. The change modifies StackFrames() and NewStackFrame() accordingly.

Tests

Added Test_StackFrames to demonstrate the problem. I tried to reproduce it without using the github.com/pkg/errors at first but was struggling to get the compiler to inline. So I reverted back to using the errors package where I first observed the problem. We can try harder to reproduce without the package or otherwise include it in the dependencies (My test runs pulled the package from my GOPATH).

Discussion

I made this PR largely to document the problem and propose the solution. Feel free to reject it for a different approach.

Alternative Approaches

This is the documented way to reconstruct the call stack, it's probably not worth trying to come up with alternative solutions.

Outstanding Questions

1. What do we want to do with github.com/pkg/errors in the test case. Can it be added as a dependency or should we try harder to reproduce inlining without it?

Review

For the submitter, initial self-review:

• Commented on code changes inline explain the reasoning behind the approach
• Reviewed the test cases added for completeness and possible points for discussion
• A changelog entry was added for the goal of this pull request
• Check the scope of the changeset - is everything in the diff required for the pull request?
• This pull request is ready for:
  • Initial review of the intended approach, not yet feature complete
  • Structural review of the classes, functions, and properties modified
  • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

• Consistency across platforms for structures or concepts added or modified
• Consistency between the changeset and the goal stated above
• Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
• Usage friction - is the proposed change in usage cumbersome or complicated?
• Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
• Concurrency concerns - if components are accessed asynchronously, what issues will arise
• Thoroughness of added tests and any missing edge cases
• Idiomatic use of the language

[mkobetic]

Looks like this is largely a duplicate of #113. Not sure if there are any bits here that are still useful, feel free to reject in favor of the other PR.

[mathewbyrne]

I just commented on #113, but this change actually looks better to me.

We're run into an issue currently where the function returned from FuncForPC is returning an incorrect function. The inlining changes sound like they could be the culprit, but switching to CallersFrames appears to correct the issue.

This looks like it should be a no-brainer to merge. Could someone from the bugsnag team take a look at this change?

Edit: also happy to help get this PR into shape too if it helps.

[xljones]

Hey @mathewbyrne, we'll take a look into this as priorities allow. However, I suspect this won't be actioned in the immediate future. If this is something you want to take further on a fork of the repo, please do so, and we can take a look at any of these changes, and see if we can merge them into the master and sort a new release to capture the changes.

[johnkiely1]

This has been released in v1.5.4