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

Jump to bottom

go/reflect: Incorrect behavior on arm64 when using MakeFunc / Call [1.18 backport] #53397

Closed
gopherbot opened this issue Jun 15, 2022 · 12 comments
Closed

go/reflect: Incorrect behavior on arm64 when using MakeFunc / Call [1.18 backport] #53397

gopherbot opened this issue Jun 15, 2022 · 12 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.18.6

Comments

@gopherbot
Copy link

@cherrymui requested issue #52788 to be considered for backport to the next 1.18 minor release.

@gopherbot please open backport issues for previous releases. This is a miscompilation that could cause valid programs to behave incorrectly. Thanks.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Jun 15, 2022
@gopherbot gopherbot mentioned this issue Jun 15, 2022
Closed
@gopherbot gopherbot added this to the Go1.18.4 milestone Jun 15, 2022
@joedian joedian added the CherryPickApproved Used during the release process for point releases label Jun 22, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Jun 22, 2022
@gopherbot gopherbot modified the milestones: Go1.18.4, Go1.18.5 Jul 12, 2022
@gopherbot gopherbot modified the milestones: Go1.18.5, Go1.18.6 Aug 1, 2022
@dmitshur
Copy link
Contributor

dmitshur commented Aug 3, 2022

@cherrymui This request is approved, so we'll need to prepare the backport CLs.

Should we backport CL 405114 for the fix, and also the 3 followup CLs 405115, 405116, and 405553? If so, I can prepare the stack and add you as reviewer (or you can mail the CLs and make me reviewer, if that's easier).

@gopherbot
Copy link
Author

Change https://go.dev/cl/421457 mentions this issue: [release-branch.go1.18] cmd/compile: fix If lowering on ARM64

@gopherbot
Copy link
Author

Change https://go.dev/cl/421459 mentions this issue: [release-branch.go1.18] cmd/compile: fix boolean comparison on PPC64

@gopherbot
Copy link
Author

Change https://go.dev/cl/421460 mentions this issue: [release-branch.go1.18] cmd/compile: fix boolean comparison on RISCV64

@gopherbot
Copy link
Author

Change https://go.dev/cl/421458 mentions this issue: [release-branch.go1.18] cmd/compile: more fix on boolean ops on ARM64

@cherrymui
Copy link
Member

@dmitshur yes, thanks! FYI, if these CLs are submitted in order, there will be temporary failures on PPC64 and RISCV builders, and the later CLs on the stack fix them. Thanks.

@thanm
Copy link
Contributor

thanm commented Aug 8, 2022

Hi @cherrymui , I'm taking over from @dmitshur on release rotation this week. OK for me to submit these CLs this morning?

@cherrymui
Copy link
Member

@thanm yes, okay to submit them when you're ready. No rush. Thanks.

@gopherbot
Copy link
Author

Closed by merging e05bd75 to release-branch.go1.18.

@gopherbot
Copy link
Author

Closed by merging 276a7bf to release-branch.go1.18.

@gopherbot
Copy link
Author

Closed by merging 21befdc to release-branch.go1.18.

@gopherbot
Copy link
Author

Closed by merging e1099eb to release-branch.go1.18.

gopherbot pushed a commit that referenced this issue Aug 8, 2022
@cherrymui @thanm
[release-branch.go1.18] cmd/compile: fix If lowering on ARM64
e05bd75 
On ARM64, an If block is lowered to (NZ cond yes no). This is
incorrect because cond is a boolean value and therefore only the
last byte is meaningful (same as AMD64, see ARM64Ops.go). But here
we are comparing a full register width with 0. Correct it by
comparing only the last bit.

For #52788.
Fixes #53397.

Change-Id: I2cacf9f3d2f45e149c361a290f511b2d4ed845c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/405114
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/421457
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Aug 8, 2022
@cherrymui @thanm
[release-branch.go1.18] cmd/compile: more fix on boolean ops on ARM64
e1099eb 
Following CL 421457, the extension rule is also wrong. It is safe
to drop the extension if the value is from a boolean-generating
instruction, but not a boolean-typed Value in general (e.g. a Phi
or a in-register parameter). Fix it.

Updates #52788.
Updates #53397.

Change-Id: Icf3028fe8e90806f9f57fbe2b38d47da27a97e2a
Reviewed-on: https://go-review.googlesource.com/c/go/+/405115
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/421458
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue Aug 8, 2022
@cherrymui @thanm
[release-branch.go1.18] cmd/compile: fix boolean comparison on PPC64
276a7bf 
Following CL 421457, for PPC64.

Should fix PPC64 builds.

Updates #52788.
Updates #53397.

Change-Id: I193ac31cfba18b4f907dd2523b51368251fd6fad
Reviewed-on: https://go-review.googlesource.com/c/go/+/405116
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/421459
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue Aug 8, 2022
@cherrymui @thanm
[release-branch.go1.18] cmd/compile: fix boolean comparison on RISCV64
21befdc 
Following CL 421457, for RISCV64.

May fix RISCV64 builds.

Updates #52788.
Updates #53397.

Change-Id: Ifc34658703d1e8b97665e7b862060152e3005d71
Reviewed-on: https://go-review.googlesource.com/c/go/+/405553
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/421460
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@heschi heschi mentioned this issue Aug 19, 2022
Closed
@golang golang locked and limited conversation to collaborators Aug 8, 2023
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 FrozenDueToAge
Projects
None yet
Milestone
Go1.18.6
Development

No branches or pull requests
5 participants
@dmitshur @gopherbot @thanm @cherrymui @joedian