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: encoding/binary.PutUint16 sometimes doesn't write [1.20 backport] #59374

Closed
gopherbot opened this issue Apr 1, 2023 · 2 comments
Closed

cmd/compile: encoding/binary.PutUint16 sometimes doesn't write [1.20 backport] #59374

gopherbot opened this issue Apr 1, 2023 · 2 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.20.4

Comments

@gopherbot
Copy link
Contributor

@randall77 requested issue #59367 to be considered for backport to the next 1.20 minor release.

@gopherbot please open backport issues for 1.20 and 1.19.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Apr 1, 2023
@gopherbot gopherbot mentioned this issue Apr 1, 2023
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 1, 2023
@gopherbot gopherbot added this to the Go1.20.3 milestone Apr 1, 2023
@mknyszek mknyszek modified the milestones: Go1.20.3, Go1.20.4 Apr 4, 2023
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/483176 mentions this issue: [release-branch.go1.20] cmd/compile: use correct type for byteswaps on multi-byte stores

@dr2chase dr2chase added the CherryPickApproved Used during the release process for point releases label Apr 12, 2023
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Apr 12, 2023
@gopherbot
Copy link
Contributor Author

Closed by merging 0684cec to release-branch.go1.20.

gopherbot pushed a commit that referenced this issue Apr 24, 2023
@randall77 @gopherbot
[release-branch.go1.20] cmd/compile: use correct type for byteswaps o…
0684cec 
…n multi-byte stores

Use the type of the store for the byteswap, not the type of the
store's value argument.

Normally when we're storing a 16-bit value, the value being stored is
also typed as 16 bits. But sometimes it is typed as something smaller,
usually because it is the result of an upcast from a smaller value,
and that upcast needs no instructions.

If the type of the store's arg is thinner than the type being stored,
and the byteswap'd value uses that thinner type, and the byteswap'd
value needs to be spilled & restored, that spill/restore happens using
the thinner type, which causes us to lose some of the top bits of the
value.

Fixes #59374

Change-Id: If6ce1e8a76f18bf8e9d79871b6caa438bc3cce4d
Reviewed-on: https://go-review.googlesource.com/c/go/+/481395
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
Run-TryBot: Keith Randall <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
(cherry picked from commit b3bc862)
Reviewed-on: https://go-review.googlesource.com/c/go/+/483176
Auto-Submit: Dmitri Shuralyov <[email protected]>
bradfitz pushed a commit to tailscale/go that referenced this issue May 25, 2023
@randall77 @bradfitz
[release-branch.go1.20] cmd/compile: use correct type for byteswaps o…
46c0b8f 
…n multi-byte stores

Use the type of the store for the byteswap, not the type of the
store's value argument.

Normally when we're storing a 16-bit value, the value being stored is
also typed as 16 bits. But sometimes it is typed as something smaller,
usually because it is the result of an upcast from a smaller value,
and that upcast needs no instructions.

If the type of the store's arg is thinner than the type being stored,
and the byteswap'd value uses that thinner type, and the byteswap'd
value needs to be spilled & restored, that spill/restore happens using
the thinner type, which causes us to lose some of the top bits of the
value.

Fixes golang#59374

Change-Id: If6ce1e8a76f18bf8e9d79871b6caa438bc3cce4d
Reviewed-on: https://go-review.googlesource.com/c/go/+/481395
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
Run-TryBot: Keith Randall <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
(cherry picked from commit b3bc862)
Reviewed-on: https://go-review.googlesource.com/c/go/+/483176
Auto-Submit: Dmitri Shuralyov <[email protected]>
bradfitz pushed a commit to tailscale/go that referenced this issue May 25, 2023
@randall77 @bradfitz
[release-branch.go1.20] cmd/compile: use correct type for byteswaps o…
def4448 
…n multi-byte stores

Use the type of the store for the byteswap, not the type of the
store's value argument.

Normally when we're storing a 16-bit value, the value being stored is
also typed as 16 bits. But sometimes it is typed as something smaller,
usually because it is the result of an upcast from a smaller value,
and that upcast needs no instructions.

If the type of the store's arg is thinner than the type being stored,
and the byteswap'd value uses that thinner type, and the byteswap'd
value needs to be spilled & restored, that spill/restore happens using
the thinner type, which causes us to lose some of the top bits of the
value.

Fixes golang#59374

Change-Id: If6ce1e8a76f18bf8e9d79871b6caa438bc3cce4d
Reviewed-on: https://go-review.googlesource.com/c/go/+/481395
Reviewed-by: David Chase <[email protected]>
Reviewed-by: Cherry Mui <[email protected]>
Run-TryBot: Keith Randall <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
(cherry picked from commit b3bc862)
Reviewed-on: https://go-review.googlesource.com/c/go/+/483176
Auto-Submit: Dmitri Shuralyov <[email protected]>
@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.20.4
Development

No branches or pull requests
3 participants
@mknyszek @dr2chase @gopherbot