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

go/types: incorrect alignment of sync/atomic.Int64 and Uint64 #53884

Closed
aclements opened this issue Jul 14, 2022 · 3 comments
Closed

go/types: incorrect alignment of sync/atomic.Int64 and Uint64 #53884

aclements opened this issue Jul 14, 2022 · 3 comments
Assignees
@aclements
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.19

Comments

@aclements
Copy link
Member

What version of Go are you using (go version)?

$ go version
go version devel go1.19-266c70c263 Thu Jul 14 15:54:36 2022 +0000 linux/amd64

Does this issue reproduce with the latest release?

No.

What did you do?

go/types doesn't understand the special alignment of the sync/atomic.Int64 and sync/atomic.Uint64 types that were added in this development cycle (ffe48e0, #47141).

What did you expect to see?

go/types and the gc compiler should agree on the layout of structs that include fields of these types. These fields should always be 8-byte aligned.

What did you see instead?

On 32-bit platforms, sync/atomic.Int64 and Uint64 fields are 4-byte aligned according to go/types.

I have a CL fixing this that I'll send out shortly.

/cc @rsc @griesemer
@aclements aclements added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 14, 2022
@aclements aclements added this to the Go1.19 milestone Jul 14, 2022
@aclements aclements self-assigned this Jul 14, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/417555 mentions this issue: go/types: correct alignment of atomic.Int64

@aclements
Copy link
Member Author

Tentatively marking this as a release-blocker, since it affects go/types' handling of a new type in 1.19.

@mdempsky
Copy link
Contributor

mdempsky commented Jul 18, 2022 •
edited
Loading

Note that go/types.StdSizes has never perfectly matched cmd/compile's layout. E.g., #12886.

If there's a reason that sync/atomic.Int64 needs special treatment to ensure consistent treatment, then perhaps we should revisit that.

jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@aclements @jproberts
go/types, types2: correct alignment of atomic.Int64
d66eda2 
atomic.Int64 has special logic in the compiler to ensure it's 8-byte
aligned on 32-bit architectures. The equivalent logic is missing in
go/types, which means the compiler and go/types can come to different
conclusions about the layout of types.

Fix this by mirroring the compiler's logic into go/types.

Fixes golang#53884.

Change-Id: I3f58a56babb76634839a161ca174c8f085fe3ba4
Reviewed-on: https://go-review.googlesource.com/c/go/+/417555
Reviewed-by: Robert Findley <[email protected]>
@golang golang locked and limited conversation to collaborators Jul 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@aclements aclements

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
3 participants
@mdempsky @aclements @gopherbot