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

runtime: doAllThreadsSyscall has an unaligned atomic load on 32-bit architectures #51776

Closed
mknyszek opened this issue Mar 17, 2022 · 15 comments
Closed

runtime: doAllThreadsSyscall has an unaligned atomic load on 32-bit architectures #51776

mknyszek opened this issue Mar 17, 2022 · 15 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.19

Comments

@mknyszek
Copy link
Contributor

The 64-bit loads here and here are unaligned on 32-bit architectures (see #599).

I would just fix this, but it's unclear to me what the right fix is. This is right at the top of the m struct, so adding padding is tricky because there could be other 64-bit loads below in the struct that will become unaligned. (I think maybe fastrand? I didn't check, maybe there isn't any.) Simultaneously, making these loads not atomic seems slightly dangerous on weak memory architectures.

This might need a backport to Go 1.18? I think these lines are new in Go 1.18.

CC @prattmic @aclements
@mknyszek mknyszek added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 17, 2022
@mknyszek mknyszek added this to the Go1.19 milestone Mar 17, 2022
@mknyszek
Copy link
Contributor Author

Oh, I should also mention, these loads are the only atomic accesses of this field.

@prattmic
Copy link
Member

procid is actually only 32-bits on Linux (gettid returns uint32), so as a workaround we could cast to *uint32 and use a 32-bit atomic load.

@mknyszek
Copy link
Contributor Author

We also suspect this isn't actually failing on the builders because cgo needs to be disabled for the test that checks this codepath, and the only nocgo builder for Linux is linux/amd64 (nothing 32-bit).

@prattmic
Copy link
Member

cc @golang/release re @mknyszek's note that we have no nocgo coverage outside of amd64.

@dmitshur
Copy link
Contributor

dmitshur commented Mar 18, 2022 •
edited

Ack. If it's deemed helpful, adding a 32-bit nocgo builder using any of our virtualized VM-based host types, e.g., linux-386-nocgo, sounds reasonable. I didn't find an existing issue for it (other than a related #22705), so please feel free to file a new one and send a CL for it. I'll prioritize the permissions work to so that you'd also be able to deploy it after the CL lands. Thanks!

@aclements
Copy link
Member

Potentially relevant is #50860, since those atomic types would let us fix these alignment issues.

This might need a backport to Go 1.18? I think these lines are new in Go 1.18.

I believe you are right that this is new in 1.18.

@prattmic
Copy link
Member

Fixed in https://go.dev/cl/399754.

@gopherbot Please backport to 1.18. This is a regression with no workaround.

@gopherbot gopherbot mentioned this issue Apr 12, 2022
Closed
@gopherbot
Copy link

Backport issue(s) opened: #52305 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link

Change https://go.dev/cl/399954 mentions this issue: runtime: align m.procid to 8 bytes on 32-bit systems

@gopherbot
Copy link

Change https://go.dev/cl/399955 mentions this issue: [release-branch.go1.18] runtime: align m.procid to 8 bytes on 32-bit systems

gopherbot pushed a commit that referenced this issue May 9, 2022
@prattmic @heschi
[release-branch.go1.18] runtime: align m.procid to 8 bytes on 32-bit …
4f45424 
…systems

https://go-review.googlesource.com/c/go/+/383434 started using
atomic Load64 on this field, which breaks 32 bit platforms which
require 64-bit alignment of uint64s that are passed to atomic operations.

Not sure why this doesn't break everywhere, but I saw it break on
my laptop during all.bash.

For #51776.
Fixes #52305.

Change-Id: Ida27b23068b3cc7208fce3c97b69a464ccf68209
Reviewed-on: https://go-review.googlesource.com/c/go/+/399754
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
(cherry-picked from commit ea7e3e3)
Reviewed-on: https://go-review.googlesource.com/c/go/+/399954
Run-TryBot: Michael Pratt <mpratt@google.com>
@bradfitz bradfitz mentioned this issue May 10, 2022
Closed
@bradfitz
Copy link
Contributor

Even without builder coverage, we could at least have some coverage that all fields in the runtime that are accessed as 64-bit atomics all have correct offsets for a handful of varied build contexts?

bradfitz added a commit to tailscale/tailscale that referenced this issue May 10, 2022
@bradfitz
go.toolchain.rev: bump Tailscale Go toolchain
fbbd4f7 
For tailscale/go@bb6009e

(The cherry-picked fix for golang/go#51776)

Change-Id: Ib4a67c0f82256c62989fbba2cc9c15b728313ebd
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@bradfitz bradfitz mentioned this issue May 10, 2022
Merged
@fauust fauust mentioned this issue May 10, 2022
Merged
bradfitz added a commit to tailscale/tailscale that referenced this issue May 10, 2022
@bradfitz
go.toolchain.rev: bump Tailscale Go toolchain
4c75605 
For tailscale/go@bb6009e

(The cherry-picked fix for golang/go#51776)

Change-Id: Ib4a67c0f82256c62989fbba2cc9c15b728313ebd
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
@cherrymui
Copy link
Member

Does procid really need to be 64-bit? I think it is the tid (thread ID), which is 32-bit on many platforms. Where do we really need 64-bit procid?

@mknyszek
Copy link
Contributor Author

I asked @prattmic the same thing: it's because on some OSes (any OS that uses pthread_self) represent the thread ID as a uintptr, which in the worst case is 64 bits wide.

Though, we could maybe just make it a uintptr?

@ianlancetaylor
Copy link
Contributor

@mknyszek Is there anything left to do here? This issue is currently in the 1.19 milestone. Thanks.

@mknyszek
Copy link
Contributor Author

I believe this is now fixed.

@golang golang locked and limited conversation to collaborators Jun 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Release Status & Release Blockers
Status: Done
Milestone
Go1.19
Development

No branches or pull requests
8 participants
@bradfitz @mknyszek @prattmic @dmitshur @aclements @ianlancetaylor @gopherbot @cherrymui