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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: return statement does not set result parameters before executing deferred function returned by a literal function #32175

Closed
sandan opened this issue May 21, 2019 · 10 comments
Closed

cmd/compile: return statement does not set result parameters before executing deferred function returned by a literal function #32175

sandan opened this issue May 21, 2019 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.13

Comments

@sandan
Copy link

sandan commented May 21, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/mark/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

See the playground example: https://play.golang.org/p/SptuCsFa3uw
I have two defer statements in their own functions. They both call anonymous functions that return a function to execute at the end of the function (when the result variable has been assigned the value in the return statement).

The functions differ in how the result variable is assigned it's value. One does it implicitly by returning the expression. The other does a bare return after modifying the named return variable.

What did you expect to see?

I expected to see the anonymous function (that prints "on exit") print the result variable with a value of 16 in the not_working() function.

What did you see instead?

That anonymous function printed 0 for the result return variable (the initial value of the result name return variable).
@sandan sandan changed the title bug: defer on-entry and on-semantics with anonymous functions defer: on-entry and on-semantics with anonymous functions May 21, 2019
@bcmills
Copy link
Contributor

bcmills commented May 22, 2019

An equivalent but slightly-more-readable version: https://play.golang.org/p/h5m8NezzYro

@bcmills bcmills changed the title defer: on-entry and on-semantics with anonymous functions cmd/compile: return statement does not set result parameters before executing deferred function returned by a literal function May 22, 2019
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 22, 2019
@bcmills bcmills added this to the Go1.13 milestone May 22, 2019
@bcmills
Copy link
Contributor

bcmills commented May 22, 2019

CC @randall77 @griesemer @mdempsky

@ianlancetaylor
Copy link
Member

Works as expected with gccgo.

Works as expected with Go 1.4 and earlier, but not with Go 1.5 and later.

@mdempsky
Copy link
Contributor

I suspect the issue is that return statements don't mark the result parameters as Assigned, so we perform the closure pass-by-value optimization. E.g., adding result = 0 after var f func() = ... in not_working makes it behave correctly.

@mdempsky
Copy link
Contributor

mdempsky commented May 22, 2019 •
edited
Loading

Actually, I think it's even more mundane than that: the first occurrence of outer here should be outermost, like the rest of the line:

go/src/cmd/compile/internal/gc/closure.go

Lines 188 to 194 in 24b4301

// out parameters will be assigned to implicitly upon return.
if outer.Class() != PPARAMOUT && !outermost.Addrtaken() && !outermost.Assigned() && v.Type.Width <= 128 {
v.Name.SetByval(true)
} else {
outermost.SetAddrtaken(true)
outer = nod(OADDR, outer, nil)
}

@StephanVerbeeck
Copy link

StephanVerbeeck commented May 22, 2019 •
edited
Loading

Could be related to the introduction of named output arguments and the duality involved (#31630) ?
There are in fact two paths now to return the values:

  1. direct via the return statement (bypassing the output variables that were declared as function arguments)
  2. assignment to output variables where they are picked up by return statement without arguments.

The timeline (sequence of instructions) might not be correct in combination with one or more levels of defer.

@mdempsky
Copy link
Contributor

@StephanVerbeeck I don't think so. I'm pretty sure it's like I stated in my last comment: outer.Class() != PPARAMOUT should be outermost.Class() != PPARAMOUT, because in multiply-nested function literals when referencing the outermost function's return parameters (like in the issue report), outer.Class() will be PAUTOHEAP instead.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/178541 mentions this issue: cmd/compile: fix capture-by-reference of return parameters

@sandan
Copy link
Author

sandan commented May 28, 2019

Thanks! How do I find out what version of golang will have the fix?

@mdempsky
Copy link
Contributor

@sandan The fix will be included in Go 1.13. Thanks for the report!

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 12, 2019
@golang golang locked and limited conversation to collaborators Jun 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
7 participants
@mdempsky @dmitshur @ianlancetaylor @bcmills @sandan @gopherbot @StephanVerbeeck