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

reflect: sort.SliceStable sorts incorrectly on arm64 with less function created with reflect.MakeFunc and slice of sufficient length [1.18 backport] #57211

Closed
gopherbot opened this issue Dec 9, 2022 · 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

Comments

@gopherbot
Copy link
Contributor

@randall77 requested issue #57184 to be considered for backport to the next 1.18 minor release.

Yes, this bug was introduced in 1.18.

@gopherbot please open backport issues.

This bug causes miscompilation of conditional move instructions. In rare cases (involving reflection, but maybe others) the boolean expression triggering the conditional move is wrong.

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Dec 9, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 9, 2022
@gopherbot gopherbot added this to the Go1.18.10 milestone Dec 9, 2022
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/456558 mentions this issue: [release-branch.go1.18] cmd/compile: fix conditional select rule

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/456735 mentions this issue: [release-branch.go1.18] cmd/compile: fix conditional move rule on PPC64

@randall77
Copy link
Contributor

There are 2 CLs for this issue - one for arm64 and one for ppc64. (The test in the first revealed the bug fixed in the second.)

@thanm thanm added the CherryPickApproved Used during the release process for point releases label Dec 14, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Dec 14, 2022
@gopherbot
Copy link
Contributor Author

Closed by merging 337138c to release-branch.go1.18.

@gopherbot
Copy link
Contributor Author

Closed by merging 6aa1e6d to release-branch.go1.18.

gopherbot pushed a commit that referenced this issue Dec 19, 2022
ARM64 maintains booleans in the low byte of registers. Upper parts
of that register are junk.
This rule is using all 32 bits of a boolean-containing register, which
is wrong. Change the rule to only look at the low bit.

Fixes #57211

Change-Id: Ibbef86b2be859df3d06d993db00e1231c481c428
Reviewed-on: https://go-review.googlesource.com/c/go/+/456556
Auto-Submit: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/456558
Reviewed-by: Than McIntosh <thanm@google.com>
gopherbot pushed a commit that referenced this issue Dec 19, 2022
Similar to CL 456556 but for ppc64 instead of arm64.

Change docs about how booleans are stored in registers for ppc64.
We now don't promise to keep the upper bits zeroed; they might be junk.

To test, I changed the boolean generation instructions (MOVBZload* and ISEL*
with boolean type) to OR in 0x100 to the result. all.bash still passed,
so I think nothing else is depending on the upper bits of booleans.

Update #57211

Change-Id: Ie66f8934a0dafa34d0a8c2a37324868d959a852c
Reviewed-on: https://go-review.googlesource.com/c/go/+/456437
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: KAMPANAT THUMWONG (KONG PC) <1992kongpc.kth@gmail.com>
Run-TryBot: Archana Ravindar <aravind5@in.ibm.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/456735
Reviewed-by: Than McIntosh <thanm@google.com>
@golang golang locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

No branches or pull requests

3 participants