Support parsing Go 1.17 stack traces

[nvanbenschoten]

Fixes #61.

In Go 1.17, the format of function arguments printed in stack traces was improved. This is mentioned in the release notes: https://tip.golang.org/doc/go1.17#compiler

The format of stack traces from the runtime (printed when an uncaught
panic occurs, or when runtime.Stack is called) is improved. Previously,
the function arguments were printed as hexadecimal words based on the
memory layout. Now each argument in the source code is printed
separately, separated by commas. Aggregate-typed (struct, array, string,
slice, interface, and complex) arguments are delimited by curly braces.
A caveat is that the value of an argument that only lives in a register
and is not stored to memory may be inaccurate. Function return values
(which were usually inaccurate) are no longer printed.

This runtime change was primarily performed in https://go-review.googlesource.com/c/go/+/304470. The description on that CL presents the change in greater detail.

This commit adds support to panicparse for parsing these new aggregate-typed arguments. It does so while retaining the type structure from the stack traces, which allows for the structure to be faithfully recreated and for aggregate-type information to be accurately augmented, when available. The aggregate-type structure is stored in a tree of mutually recursive Args and Arg structs.

The commit also adds support for handling the new "offset too large" operator that was added in the same runtime CL. This operator comes in the form of an "_" scalar argument in cases where the argument's frame offset was too large (>= 0xf0), preventing the argument from being printed in the stack trace.

This change raises questions about backwards compatability in a few different areas.

• this change does not break backwards compatability with pre-1.17 stack traces. However, due to the nature of how most of the unit tests are set up (i.e. shelling out to local go binary), we are losing some test coverage of pre-1.17 stack trace parsing. Is that ok?
• similarly, this change breaks the library's tests for pre-1.17 installations of Go. As a result, it bumps the Go version run in CI. Is that ok?
• this change will not cause users of the github.com/maruel/panicparse/v2/stack library to fail to compile. However, due to the additions to the Arg struct, users of the library will fail to observe all arguments when parsing post-1.17 stack traces (but not pre-1.17 stack traces) unless they are aware of the new IsAggregate field. Is that ok?

[codecov]

Codecov Report

Merging #62 (21cb5f0) into main (98fd2ed) will decrease coverage by 0.1%.
The diff coverage is 88.0%.

@@           Coverage Diff           @@
##            main     #62     +/-   ##
=======================================
- Coverage   90.0%   89.9%   -0.1%     
=======================================
  Files         12      12             
  Lines       3471    3587    +116     
=======================================
+ Hits        3124    3226    +102     
- Misses       282     291      +9     
- Partials      65      70      +5     

Impacted Files	Coverage Δ
stack/stack.go	83.9% <78.9%> (-0.5%)	⬇️
stack/source.go	85.2% <84.6%> (-2.3%)	⬇️
stack/context.go	86.2% <100.0%> (+1.1%)	⬆️
stack/webstack/webstack.go	96.2% <0.0%> (+5.7%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98fd2ed...21cb5f0. Read the comment docs.

[maruel]

Wow thanks so much for working on this.

Since it's a somewhat tricky change, would you mind creating one PR with the //go:build commit to reduce the number of files touched by this PR? Thanks!

Answering your questions:

• For now pre-1.17 coverage is important, it's fine to skip tests based on compiler version.
• The CI both test current and previous version, please keep them working. They indeed broke.
• For your latest question, I'll get back to you once I took a deeper look.

[nvanbenschoten]

Thanks @maruel, I'm glad this is going in the right direction and that you're open to an external contribution. Also, thank you for this library - we're big fans over at CockroachDB!

Since it's a somewhat tricky change, would you mind creating one PR with the //go:build commit to reduce the number of files touched by this PR?

Done in #63 and #64.

For now pre-1.17 coverage is important, it's fine to skip tests based on compiler version.

I updated the commit to expand the version-dependent testing to support pre and post-1.17 stack traces.

The CI both test current and previous version, please keep them working. They indeed broke.

CI should be green now across the board.

For your latest question, I'll get back to you once I took a deeper look.

Sounds good to me. I'm happy to answer questions if I can be of any help.

[maruel]

Update: I fixed issues that a debian maintainer discovered so you'll want to rebase.

I'd like to have the unsafeString() change to be in a separate PR. It's unrelated to the fix.

Is the handling about ast.BasicLit and ast.Ellipsis in source.go also unrelated?

[derekperkins]

Excited for this to land!

[maruel]

Sorry for the late response. This PR is quite hard to review because it changes multiple things at once. It would really help if "1 feature change = 1 PR".

Here I can see that you further disable inlining, improve source parsing and improve go1.17 processing support. These should be separate.

[maruel]

Thanks for the change!

I cherry-picked the two commits with attribution but minor change as d7adfee and 5825e8f. Tests now pass everywhere.

[nvanbenschoten]

Thanks @maruel for the help with this change, and thanks again for the fantastic library!

[derekperkins]

Thanks to both of you! When should we expect a release with this included?

[nvanbenschoten]

@derekperkins I think @maruel already has you covered with v2.2.0: https://github.com/maruel/panicparse/releases/tag/v2.2.0.

[derekperkins]

Thanks. I saw that and since it only referenced GitHub Actions, I thought it was a maintenance release and didn't dig deeper.

[maruel]

Oh thanks for the note, converted to a real release. https://github.com/maruel/panicparse/releases/tag/v2.2.0 now talks about go1.17.

[derekperkins]

Thanks!