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: stop marking arguments live for entire function call #15843

Closed
rsc opened this issue May 26, 2016 · 5 comments
Closed

cmd/compile: stop marking arguments live for entire function call #15843

rsc opened this issue May 26, 2016 · 5 comments
Labels
FrozenDueToAge
Milestone
Go1.8

Comments

@rsc
Copy link
Contributor

rsc commented May 26, 2016

For Go 1.8 we probably need to stop marking args live for the entire function call. It just doesn't fit with the sophisticated analyses being done by the SSA back end. Programmers will have runtime.KeepAlive already in Go 1.7, so people doing Go 1.8 beta testing will be able to update their code in a (Go 1.7-)backwards compatible way. And maybe we can find some way to help people find problems faster. It's going to be quite a band-aid to rip off.

But see my recent comment in #15277 for the pain we're in if we don't do this.

/cc @randall77 @aclements @RLH @dr2chase
@rsc rsc added this to the Go1.8 milestone May 26, 2016
@randall77 randall77 mentioned this issue Aug 13, 2016
Closed
@randall77
Copy link
Contributor

Here's the plan:

  1. Tell people they have to use runtime.KeepAlive
  2. Stop marking arguments live for the entire function call.
  3. Have the SSA backend intrinsify runtime.KeepAlive so it costs little to nothing.

I'm signing myself up to do #2 and #3. I will put #2 behind a flag of some sort and have it default to the new 1.8 behavior. We should probably think more about how to do #1 robustly.

@randall77 randall77 self-assigned this Aug 30, 2016
@bradfitz
Copy link
Contributor

I don't think we need to go out of our way for step 1. It was already in the Go 1.7 release notes, and we'd put it in the Go 1.8 release notes again, probably a bit more prominently.

I think even a flag is pretty generous.

@davecheney
Copy link
Contributor

Same about the flag, it just gives people a way to continue to use subtly
incorrect code for longer than their users expect, but less than they
really want.

On Wed, 31 Aug 2016, 08:33 Brad Fitzpatrick notifications@github.com
wrote:

I don't think we need to go out of our way for step 1. It was already in
the Go 1.7 release notes, and we'd put it in the Go 1.8 release notes
again, probably a bit more prominently.

I think even a flag is pretty generous.

—
You are receiving this because you are subscribed to this thread.

Reply to this email directly, view it on GitHub
#15843 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAcAwpnjm6-f5Fpn46i_RBUBZf56RuHks5qlK_IgaJpZM4InKmX
.

@gopherbot
Copy link

CL https://golang.org/cl/28310 mentions this issue.

gopherbot pushed a commit that referenced this issue Sep 19, 2016
@randall77
cmd/compile: args no longer live until end-of-function
ca4089a 
We're dropping this behavior in favor of runtime.KeepAlive.
Implement runtime.KeepAlive as an intrinsic.

Update #15843

Change-Id: Ib60225bd30d6770ece1c3c7d1339a06aa25b1cbc
Reviewed-on: https://go-review.googlesource.com/28310
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@randall77
Copy link
Contributor

Ok, #2 and #3 are in, and there is no flag.

@dsnet dsnet mentioned this issue Oct 12, 2016
Closed
@dsnet dsnet mentioned this issue Oct 31, 2016
Closed
@golang golang locked and limited conversation to collaborators Sep 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
5 participants
@bradfitz @davecheney @rsc @randall77 @gopherbot