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: inlining function that references function literals generates bad code [1.19 backport] #59158

Closed
gopherbot opened this issue Mar 20, 2023 · 5 comments
Closed

cmd/compile: inlining function that references function literals generates bad code [1.19 backport] #59158

gopherbot opened this issue Mar 20, 2023 · 5 comments
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.9

Comments

@gopherbot
Copy link

gopherbot commented Mar 20, 2023 •
edited by dmitshur

@ianlancetaylor requested issue #54632 to be considered for backport to the next 1.19 minor release.

Please open backport issue for 1.19

Reportedly this issue occurs in 1.19 as well.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Mar 20, 2023
@gopherbot gopherbot mentioned this issue Mar 20, 2023
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 20, 2023
@gopherbot gopherbot added this to the Go1.19.8 milestone Mar 20, 2023
@heschi
Copy link
Contributor

heschi commented Mar 22, 2023

Since this is a serious bug, and the fix already shipped in 1.20 (http://go.dev/cl/425257), a backport to 1.19 seems appropriate and safe.

@heschi heschi added the CherryPickApproved Used during the release process for point releases label Mar 22, 2023
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Mar 22, 2023
@gopherbot
Copy link
Author

Change https://go.dev/cl/479629 mentions this issue: [release-branch.go1.19] cmd/compile: defer transitive inlining until after AST is edited

@gopherbot
Copy link
Author

Closed by merging 837b131 to release-branch.go1.19.

gopherbot pushed a commit that referenced this issue Apr 3, 2023
@mdempsky @mknyszek
[release-branch.go1.19] cmd/compile: defer transitive inlining until …
837b131 
…after AST is edited

This CL changes the inliner to process transitive inlining iteratively
after the AST has actually been edited, rather than recursively and
immediately. This is important for handling indirect function calls
correctly, because ir.reassigned walks the function body looking for
reassignments; whereas previously the inlined reassignments might not
have been actually added to the AST yet.

Fixes #59158.

Change-Id: I0dd69813c8a70b965174e0072335bc00afedf286
Reviewed-on: https://go-review.googlesource.com/c/go/+/425257
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit f983a93)
Reviewed-on: https://go-review.googlesource.com/c/go/+/479629
Reviewed-by: Heschi Kreinick <heschi@google.com>
@mknyszek mknyszek reopened this Apr 3, 2023
@mknyszek mknyszek reopened this Apr 3, 2023
@mknyszek mknyszek modified the milestones: Go1.19.8, Go1.19.9 Apr 3, 2023
@gopherbot
Copy link
Author

Change https://go.dev/cl/481797 mentions this issue: [release-branch.go1.19] cmd/compile: defer transitive inlining until after AST is edited

gopherbot pushed a commit that referenced this issue Apr 5, 2023
@mknyszek
[release-branch.go1.19] cmd/compile: defer transitive inlining until …
e6130c6 
…after AST is edited

This CL changes the inliner to process transitive inlining iteratively
after the AST has actually been edited, rather than recursively and
immediately. This is important for handling indirect function calls
correctly, because ir.reassigned walks the function body looking for
reassignments; whereas previously the inlined reassignments might not
have been actually added to the AST yet.

Fixes #59158.

This change was previously reverted as CL 481796 because the branch
was frozen for release.

Change-Id: I97fcd32956cc1349d87a92066e8559cb90da73b7
Reviewed-on: https://go-review.googlesource.com/c/go/+/481797
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur dmitshur changed the title cmd/compile: incorrect inlining of function swapping two funcs [1.19 backport] cmd/compile: inlining function that references function literals generates bad code [1.19 backport] Apr 24, 2023
@dmitshur
Copy link
Contributor

Closed by merging CL 481797 (commit e6130c6) into release-branch.go1.19.

@golang golang locked and limited conversation to collaborators Apr 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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.9
Development

No branches or pull requests
4 participants
@mknyszek @dmitshur @gopherbot @heschi