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

x/tools/go/ssa: defer into stack frame #66601

Closed
timothy-king opened this issue Mar 29, 2024 · 16 comments
Closed

x/tools/go/ssa: defer into stack frame #66601

timothy-king opened this issue Mar 29, 2024 · 16 comments
Labels
Proposal Proposal-Accepted Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@timothy-king
Copy link
Contributor

timothy-king commented Mar 29, 2024

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 to Defer, a new ssa builtin function ssa:defers(), and new opaque type deferStack.

ssa:defers() returns a reference to the current function's defer stack when are executed by RunDefers. ssa:defers is a Builtin and the return type is a pointer to a deferStack. When Defer.Into != nil, it is a pointer to a deferStack 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:

var #defers = runtime.deferrangefunc()
f(func() {
	runtime.deferprocat(func() { print("A") }, #defers)
})

ssa:defers() is analogous to runtime.deferrangefunc() and Defer.Into != nil is equivalent to runtime.deferprocat.

Better suggestions for names are welcomed.

A simplification we may also wish to do is to require Defer.Into to not be equal to nil (instead of optionally nil). This might not be backwards compatible with existing ssa users on existing code. They would see ssa:defers() calls and the new *deferStack type. The proposed implementation currently only emits a value for Into for range-over-func loops.

@timothy-king timothy-king added this to the Go1.23 milestone Mar 29, 2024
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Mar 29, 2024
@timothy-king
Copy link
Contributor Author

https://go.dev/cl/555075 proposed implementation of GOEXPERIMENT=rangefunc for x/tools/go/ssa.

@adonovan
Copy link
Member

adonovan commented Mar 31, 2024

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.)

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Apr 3, 2024
@timothy-king
Copy link
Contributor Author

Another option is for ssa:deferstack() to be a new ssa Instruction, Stack. The new Instruction would take no arguments and produce a value referring to the stack frame of the current function. The type of the value would be a pointer to an opaque stack type. A new Instruction type would not be backwards compatible with type switches that attempt to be complete over Instructions.

If we start to require the Stack field in Defer and RunDefers, my preference would be to have Stack over ssa:deferstack(). It is marginally easier to deal with Instructions than calls to Builtins and we will expect to have a lot more of these in preexisting code. If we leave the Stack field optional, I do not have a strong preference between a new Builtin and a new Instruction.

@timothy-king
Copy link
Contributor Author

(cc @dominikh)

@adonovan
Copy link
Member

adonovan commented Apr 3, 2024

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).

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/555075 mentions this issue: go/ssa: support range-over-func

@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

The proposal is to add

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
    ...
}

@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

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.

@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Apr 10, 2024
@adonovan
Copy link
Member

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.

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.)

@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

None of these seem like arguments against 'DeferStack' instead of 'Stack'.

@adonovan
Copy link
Member

adonovan commented May 9, 2024

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 
    ...
}

@timothy-king
Copy link
Contributor Author

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).

gopherbot pushed a commit to golang/tools that referenced this issue May 14, 2024
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>
@rsc
Copy link
Contributor

rsc commented May 15, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add

type Defer struct {
+   DeferStack Value // stack of deferred functions (from ssa:deferstack() intrinsic) onto which this function is pushed 
    ...
}

@rsc rsc moved this from Active to Likely Accept in Proposals May 15, 2024
@rsc
Copy link
Contributor

rsc commented May 23, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add

type Defer struct {
+   DeferStack Value // stack of deferred functions (from ssa:deferstack() intrinsic) onto which this function is pushed 
    ...
}

@rsc rsc moved this from Likely Accept to Accepted in Proposals May 23, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/590975 mentions this issue: go/ssa: export Defer.DeferStack field

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Accepted Tools This label describes issues relating to any tools in the x/tools repository.
Projects
Status: Accepted
Development

No branches or pull requests

4 participants