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: Value live at entry [1.19 backport] #54917

Closed
gopherbot opened this issue Sep 7, 2022 · 6 comments
Closed

cmd/compile: Value live at entry [1.19 backport] #54917

gopherbot opened this issue Sep 7, 2022 · 6 comments
Assignees
@mdempsky
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Milestone
Go1.19.2

Comments

@gopherbot
Copy link
Contributor

@cuonglm requested issue #54911 to be considered for backport to the next 1.19 minor release.

@gopherbot please backport this to go1.19
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Sep 7, 2022
@gopherbot gopherbot mentioned this issue Sep 7, 2022
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 7, 2022
@gopherbot gopherbot added this to the Go1.19.2 milestone Sep 7, 2022
@mknyszek
Copy link
Contributor

mknyszek commented Sep 7, 2022

Deferring approval until we can decide whether this should also be applied to 1.18.

@mdempsky said in the non-backport issue:

Yeah, I think CL 422196 is safe to backport if it fixes a regression in 1.19.

Does this apply to 1.18 as well? How does this interact with the no-backports-of-generic-fixes policy to 1.18?

Thanks!

@cuonglm
Copy link
Member

cuonglm commented Sep 7, 2022

Deferring approval until we can decide whether this should also be applied to 1.18.

@mdempsky said in the non-backport issue:

Yeah, I think CL 422196 is safe to backport if it fixes a regression in 1.19.

Does this apply to 1.18 as well? How does this interact with the no-backports-of-generic-fixes policy to 1.18?

Thanks!

Hmm, to be clear, the original issue happens since go1.18, so this is not a regression at all.

@mdempsky
Copy link
Contributor

mdempsky commented Sep 7, 2022

Deferring approval until we can decide whether this should also be applied to 1.18.

I've written my thoughts on backporting this fix at #54911 (comment).

@mknyszek
Copy link
Contributor

mknyszek commented Sep 7, 2022

@mdempsky Thanks! Since the fix is safe and the workaround is onerous (and applies to both proposed backports), I'm inclined to approve.

@mknyszek mknyszek added the CherryPickApproved Used during the release process for point releases label Sep 7, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Sep 7, 2022
@mknyszek mknyszek moved this to In Progress in Go Compiler / Runtime Sep 7, 2022
@julieqiu julieqiu removed this from Go Security Sep 8, 2022
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/429896 mentions this issue: [release-branch.go1.19] cmd/compile/internal/inline: fix latent CalleeEffects issue

@gopherbot
Copy link
Contributor Author

Closed by merging 225bcec to release-branch.go1.19.

Repository owner moved this from In Progress to Done in Go Compiler / Runtime Sep 19, 2022
gopherbot pushed a commit that referenced this issue Sep 19, 2022
@mdempsky @cherrymui
[release-branch.go1.19] cmd/compile/internal/inline: fix latent Calle…
225bcec 
…eEffects issue

ir.ClosureExpr implements ir.InitNode, so ir.InitExpr can prepend init
statements to it. However, CalleeEffects wasn't aware of this and
could cause the init statements to get dropped when inlining a call to
a closure.

This isn't an issue today, because we don't create closures with init
statements. But I ran into this within unified IR.

Easy and robust solution: just take advantage that ir.TakeInit can
handle any node.

Fixes #54917.

Change-Id: Ica05fbf6a8c5be4b11927daf84491a1140da5431
Reviewed-on: https://go-review.googlesource.com/c/go/+/422196
Reviewed-by: Than McIntosh <[email protected]>
Run-TryBot: Matthew Dempsky <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Cuong Manh Le <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/429896
Reviewed-by: Michael Knyszek <[email protected]>
bradfitz pushed a commit to tailscale/go that referenced this issue Oct 5, 2022
@mdempsky @bradfitz
[release-branch.go1.19] cmd/compile/internal/inline: fix latent Calle…
e51935e 
…eEffects issue

ir.ClosureExpr implements ir.InitNode, so ir.InitExpr can prepend init
statements to it. However, CalleeEffects wasn't aware of this and
could cause the init statements to get dropped when inlining a call to
a closure.

This isn't an issue today, because we don't create closures with init
statements. But I ran into this within unified IR.

Easy and robust solution: just take advantage that ir.TakeInit can
handle any node.

Fixes golang#54917.

Change-Id: Ica05fbf6a8c5be4b11927daf84491a1140da5431
Reviewed-on: https://go-review.googlesource.com/c/go/+/422196
Reviewed-by: Than McIntosh <[email protected]>
Run-TryBot: Matthew Dempsky <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Cuong Manh Le <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/429896
Reviewed-by: Michael Knyszek <[email protected]>
bradfitz pushed a commit to tailscale/go that referenced this issue Oct 5, 2022
@mdempsky @bradfitz
[release-branch.go1.19] cmd/compile/internal/inline: fix latent Calle…
f6f2b8e 
…eEffects issue

ir.ClosureExpr implements ir.InitNode, so ir.InitExpr can prepend init
statements to it. However, CalleeEffects wasn't aware of this and
could cause the init statements to get dropped when inlining a call to
a closure.

This isn't an issue today, because we don't create closures with init
statements. But I ran into this within unified IR.

Easy and robust solution: just take advantage that ir.TakeInit can
handle any node.

Fixes golang#54917.

Change-Id: Ica05fbf6a8c5be4b11927daf84491a1140da5431
Reviewed-on: https://go-review.googlesource.com/c/go/+/422196
Reviewed-by: Than McIntosh <[email protected]>
Run-TryBot: Matthew Dempsky <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Cuong Manh Le <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/429896
Reviewed-by: Michael Knyszek <[email protected]>
bradfitz pushed a commit to tailscale/go that referenced this issue Oct 5, 2022
@mdempsky @bradfitz
[release-branch.go1.19] cmd/compile/internal/inline: fix latent Calle…
f6f8a7e 
…eEffects issue

ir.ClosureExpr implements ir.InitNode, so ir.InitExpr can prepend init
statements to it. However, CalleeEffects wasn't aware of this and
could cause the init statements to get dropped when inlining a call to
a closure.

This isn't an issue today, because we don't create closures with init
statements. But I ran into this within unified IR.

Easy and robust solution: just take advantage that ir.TakeInit can
handle any node.

Fixes golang#54917.

Change-Id: Ica05fbf6a8c5be4b11927daf84491a1140da5431
Reviewed-on: https://go-review.googlesource.com/c/go/+/422196
Reviewed-by: Than McIntosh <[email protected]>
Run-TryBot: Matthew Dempsky <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Cuong Manh Le <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/429896
Reviewed-by: Michael Knyszek <[email protected]>
@golang golang locked and limited conversation to collaborators Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mdempsky mdempsky

Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Projects
None yet
Milestone
Go1.19.2
Development

No branches or pull requests
4 participants
@mdempsky @mknyszek @cuonglm @gopherbot