-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
x/tools/go/ssa: defer into stack frame #66601
Comments
https://go.dev/cl/555075 proposed implementation of |
I agree that this is needed, and I agree it is cleaner to require the Defer{Into} field in all cases rather than make it implicitly "the current defer stack" when omitted. Let's use the same name for both fields: package ssa
type Defer struct {
+ Stack Value // stack (from ssa:deferstack() intrinsic) onto which this function is pushed
...
}
type RunDefers struct {
+ Stack Value // stack (from ssa:deferstack() intrinsic) of functions to call in pop order
...
} (Aside: it would be nice if we could avoid the double indirection of vars plumbed through closures when they are never assigned from within the closure; in the case of deferstacks we know a priori that this is the case.) |
Another option is for If we start to require the |
(cc @dominikh) |
There's no need for a new instruction, so let's not add one. As we've seen from the types.Alias work, breaking existing switches is a very expensive business. Adding new fields (even required ones) to existing instructions is easy, as there is only one algorithm that creates instructions (the builder). |
Change https://go.dev/cl/555075 mentions this issue: |
The proposal is to add
|
There is significant opportunity for confusion between the call stack and the defer stack. I think the name 'Stack' is not specific enough. It should be 'Defers' or 'DeferStack' or 'DeferList' or something like that. |
This proposal has been added to the active column of the proposals project |
Users never populate this field, they only read it, and the doc comments will make clear that it refers to the logical (opaque) stack of deferred functions. (I can't see what stack-of-calls it could be confused with.) |
None of these seem like arguments against 'DeferStack' instead of 'Stack'. |
Ok, deferstack it is. I don't see any real need for the RunDefers.DeferStack field since its value must always be the same as would be obtained by a call to ssa:deferstack(), so let's avoid doubling the size of that instruction. @timothy-king @rsc Are we all happy with this:? package ssa
type Defer struct {
+ DeferStack Value // stack of deferred functions (from ssa:deferstack() intrinsic) onto which this function is pushed
...
} |
I'm happy. There is nothing we do not need. And what is there is straightforward to extend later if we change our minds (like a Stack instruction). |
Adds support for range over function types. The basic idea is to rewrite for x := range f { ... } into yield := func(x T) bool { ... } f(yield) This adds a new type of synthetic functions to represent the yield function of a range-over-func statement. The syntax for such functions is an *ast.RangeStmt. Yields are considered anonymous functions in the source function. More extensive details can be found in the comments to builder.rangeFunc. Yield functions can be exited by break, continue, break, goto, and return statements as well as labelled variants of these statements. Each Function tracks the unresolved exits from its body. After the call f(yield), the generated code checks which statement exited the loop and handles the exit to resume execution. Defer has a new field _Stack. If not nil, Into is value of opaque type *deferStack. *deferStack is a representation of the stack of defers of a function's stack frame. A *deferStack Value can be gotten by calling a new builtin function `ssa:deferstack()`. Updates golang/go#66601 Change-Id: I563cdde839f2fce3e29c766aa9d192715dcb319b Reviewed-on: https://go-review.googlesource.com/c/tools/+/555075 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alan Donovan <adonovan@google.com>
Based on the discussion above, this proposal seems like a likely accept. The proposal is to add
|
No change in consensus, so accepted. 🎉 The proposal is to add
|
Change https://go.dev/cl/590975 mentions this issue: |
Proposal Details
This proposal is to extend x/tools/go/ssa to support for defers when running with
GOEXPERIMENT=rangefunc
.This adds a new
Into
field toDefer
, a new ssa builtin functionssa:defers()
, and new opaque typedeferStack
.ssa:defers()
returns a reference to the current function's defer stack when are executed byRunDefers
.ssa:defers
is aBuiltin
and the return type is a pointer to adeferStack
. WhenDefer.Into != nil
, it is a pointer to adeferStack
and is pushed onto that defer stack.It is worth comparing these to the new runtime intrinsics to implement defer with range-over-func yields:
ssa:defers()
is analogous toruntime.deferrangefunc()
andDefer.Into != nil
is equivalent toruntime.deferprocat
.Better suggestions for names are welcomed.
A simplification we may also wish to do is to require
Defer.Into
to not be equal tonil
(instead of optionally nil). This might not be backwards compatible with existing ssa users on existing code. They would seessa:defers()
calls and the new*deferStack
type. The proposed implementation currently only emits a value forInto
for range-over-func loops.The text was updated successfully, but these errors were encountered: