Skip to content
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

runtime: rewrite gentraceback as an iterator API #54466

Open
aclements opened this issue Aug 15, 2022 · 10 comments
Open

runtime: rewrite gentraceback as an iterator API #54466

aclements opened this issue Aug 15, 2022 · 10 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest
Milestone

Comments

@aclements
Copy link
Member

aclements commented Aug 15, 2022

Currently, all stack walking logic is in one venerable, large, and very, very complicated function: runtime.gentraceback. This function has three distinct operating modes: printing, populating a PC buffer, or invoking a callback. And it has three different modes of unwinding: physical Go frames, inlined Go frames, and cgo frames. It also has several flags. All of this logic is very interwoven.

I would like to replace all of this with a caller-driven iterator-style interface. This is a tracking issue for that change.

An iterator API will consolidate the logic for unwinding and allow us to lift out printing and pcbuf populating into separate code, while replacing the callback mode with direct use of the new API. It will allow us to better layer the different modes of unwinding by creating separate iterator types for physical, inlined, and cgo frames, while keeping the interface ergonomic. This is also a good opportunity to generally clean up this code.

As a follow-on, I plan to dramatically simplify the defer implementation. Regabi enabled many simplifications to defer and we've implemented many of them already, but there are more aggressive simplifications we haven't tackled yet. Part of this is simplifying open-coded defers, and doing that efficiently requires being able to simultaneously walk the stack frames and the defer stack. An iterator API will make this much easier to do.

An alternative approach would be to use a callback interface rather than an iterator. This would be an improvement over the status quo and also be a simpler change, but I think it has two drawbacks: 1. It makes layering physical/inlined frame unwinding more awkward because you need multiple levels of callbacks. 2. It's poorly suited to parallel iteration like we need for the open-coded defer implementation. Long term, an iterator API probably makes it simpler to scan a goroutine stack while the goroutine is still running (reducing per-goroutine latency for goroutines with large stacks) because we can easily pause and resume unwinding, while a callback API doesn't easily afford this opportunity.

@aclements aclements self-assigned this Aug 15, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 15, 2022
@randall77
Copy link
Contributor

randall77 commented Aug 15, 2022

Keep in mind #7181, where we want to print bottom and top of deep stacks. It probably requires walking the stack twice, or keeping a buffer of 50 frames, or something like that.
I think it would not be a problem with this plan, just FYI.

@gopherbot
Copy link

gopherbot commented Aug 16, 2022

Change https://go.dev/cl/424254 mentions this issue: runtime: drop function context from traceback

@gopherbot
Copy link

gopherbot commented Aug 16, 2022

Change https://go.dev/cl/424255 mentions this issue: runtime: drop redundant argument to getArgInfo

@gopherbot
Copy link

gopherbot commented Aug 16, 2022

Change https://go.dev/cl/424257 mentions this issue: runtime: switch gp when jumping stacks during traceback

@aclements
Copy link
Member Author

aclements commented Aug 16, 2022

Keep in mind #7181, where we want to print bottom and top of deep stacks.

I think this will actually make that dramatically easier by inverting the flow of traceback printing. With this change, the printer will drive the stack walk instead of the other way around, so I think it will be much easier for the printer to keep the buffer it needs, and to do so without adding complexity around the stack walk itself.

@gopherbot
Copy link

gopherbot commented Aug 17, 2022

Change https://go.dev/cl/424514 mentions this issue: runtime: replace stkframe.arglen/argmap with methods

@gopherbot
Copy link

gopherbot commented Aug 17, 2022

Change https://go.dev/cl/424516 mentions this issue: runtime: consolidate stkframe and its methods into stkframe.go

@gopherbot
Copy link

gopherbot commented Aug 17, 2022

Change https://go.dev/cl/424515 mentions this issue: runtime: make getStackMap a method of stkframe

@mknyszek mknyszek added this to the Go1.20 milestone Aug 17, 2022
@gopherbot
Copy link

gopherbot commented Aug 26, 2022

Change https://go.dev/cl/425936 mentions this issue: runtime: simplify stkframe.argMapInternal

gopherbot pushed a commit that referenced this issue Sep 2, 2022
Currently, gentraceback tracks the closure context of the outermost
frame. This used to be important for "unstarted" calls to reflect
function stubs, where "unstarted" calls are either deferred functions
or the entry-point of a goroutine that hasn't run. Because reflect
function stubs have a dynamic argument map, we have to reach into
their closure context to fetch to map, and how to do this differs
depending on whether the function has started. This was discovered in
issue #25897.

However, as part of the register ABI, "go" and "defer" were made much
simpler, and any "go" or "defer" of a function that takes arguments or
returns results gets wrapped in a closure that provides those
arguments (and/or discards the results). Hence, we'll see that closure
instead of a direct call to a reflect stub, and can get its static
argument map without any trouble.

The one case where we may still see an unstarted reflect stub is if
the function takes no arguments and has no results, in which case the
compiler can optimize away the wrapper closure. But in this case we
know the argument map is empty: the compiler can apply this
optimization precisely because the target function has no argument
frame.

As a result, we no longer need to track the closure context during
traceback, so this CL drops all of that mechanism.

We still have to be careful about the unstarted case because we can't
reach into the function's locals frame to pull out its context
(because it has no locals frame). We double-check that in this case
we're at the function entry.

I would prefer to do this with some in-code PCDATA annotations of
where to find the dynamic argument map, but that's a lot of mechanism
to introduce for just this. It might make sense to consider this along
with #53609.

Finally, we beef up the test for this so it more reliably forces the
runtime down this path. It's fundamentally probabilistic, but this
tweak makes it better. Scheduler testing hooks (#54475) would make it
possible to write a reliable test for this.

For #54466, but it's a nice clean-up all on its own.

Change-Id: I16e4f2364ba2ea4b1fec1e27f971b06756e7b09f
Reviewed-on: https://go-review.googlesource.com/c/go/+/424254
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Sep 2, 2022
The f funcInfo argument is always the same as frame.fn, so we don't
need to pass it. I suspect that was there to make the signatures of
getArgInfoFast and getArgInfo more similar, but it's not necessary.

For #54466.

Change-Id: Idc717f4df09e97cad49d52c5b7edf28090908cba
Reviewed-on: https://go-review.googlesource.com/c/go/+/424255
Run-TryBot: Austin Clements <austin@google.com>
Auto-Submit: Austin Clements <austin@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Sep 2, 2022
Currently, when traceback jumps from the system stack to a user stack
(e.g., during profiling tracebacks), it leaves gp pointing at the g0.
This is currently harmless since it's only used during profiling, so
the code paths in gentraceback that care about gp aren't used, but
it's really confusing and would certainly break if _TraceJumpStack
were ever used in a context other than profiling.

Fix this by updating gp to point to the user g when we switch stacks.

For #54466.

Change-Id: I1541e004667a52e37671803ce45c91d8c5308830
Reviewed-on: https://go-review.googlesource.com/c/go/+/424257
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Austin Clements <austin@google.com>
gopherbot pushed a commit that referenced this issue Sep 2, 2022
Currently, stkframe.arglen and stkframe.argmap are populated by
gentraceback under a particular set of circumstances. But because they
can be constructed from other fields in stkframe, they don't need to
be computed eagerly at all. They're also rather misleading, as they're
only part of computing the actual argument map and most callers should
be using getStackMap, which does the rest of the work.

This CL drops these fields from stkframe. It shifts the functions that
used to compute them, getArgInfoFast and getArgInfo, into
corresponding methods stkframe.argBytes and stkframe.argMapInternal.
argBytes is expected to be used by callers that need to know only the
argument frame size, while argMapInternal is used only by argBytes and
getStackMap.

We also move some of the logic from getStackMap into argMapInternal
because the previous split of responsibilities didn't make much sense.
This lets us return just a bitvector from argMapInternal, rather than
both a bitvector, which carries a size, and an "actually use this
size".

The getArgInfoFast function was inlined before (and inl_test checked
this). We drop that requirement from stkframe.argBytes because the
uses of this have shifted and now it's only called from heap dumping
(which never happens) and conservative stack frame scanning (which
very, very rarely happens).

There will be a few follow-up clean-up CLs.

For #54466. This is a nice clean-up on its own, but it also serves to
remove pointers from the traceback state that would eventually become
troublesome write barriers once we stack-rip gentraceback.

Change-Id: I107f98ed8e7b00185c081de425bbf24af02a4163
Reviewed-on: https://go-review.googlesource.com/c/go/+/424514
Run-TryBot: Austin Clements <austin@google.com>
Auto-Submit: Austin Clements <austin@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Sep 2, 2022
This places getStackMap alongside argBytes and argMapInternal as
another method of stkframe.

For #54466, albeit rather indirectly.

Change-Id: I411dda3605dd7f996983706afcbefddf29a68a85
Reviewed-on: https://go-review.googlesource.com/c/go/+/424515
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Austin Clements <austin@google.com>
Auto-Submit: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Sep 2, 2022
The stkframe struct and its methods are strewn across different source
files. Since they actually have a pretty coherent theme at this point,
migrate it all into a new file, stkframe.go. There are no code changes
in this CL.

For #54466, albeit rather indirectly.

Change-Id: Ibe53fc4b1106d131005e1c9d491be838a8f14211
Reviewed-on: https://go-review.googlesource.com/c/go/+/424516
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Austin Clements <austin@google.com>
Auto-Submit: Austin Clements <austin@google.com>
gopherbot pushed a commit that referenced this issue Sep 2, 2022
Use an early return to reduce indentation and clarify flow.

For #54466.

Change-Id: I12ce810bea0f22b8707a175dc5ba66241c0a9a21
Reviewed-on: https://go-review.googlesource.com/c/go/+/425936
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@aclements
Copy link
Member Author

aclements commented Sep 7, 2022

As a follow-on, I plan to dramatically simplify the defer implementation.

Note to self: simplifying defer using an iterator-style gentraceback would also put us in a much better position to fix panic/recover causing bad stacks and memory leaks in the race detector (#26813, #37233).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest
Projects
Status: In Progress
Development

No branches or pull requests

4 participants