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

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

Open
mknyszek opened this issue Mar 17, 2022 · 13 comments
Open
Labels
NeedsDecision
Milestone

Comments

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Mar 17, 2022

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 label Mar 17, 2022
@mknyszek mknyszek added this to the Go1.19 milestone Mar 17, 2022
@mknyszek
Copy link
Contributor Author

@mknyszek mknyszek commented Mar 18, 2022

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

@prattmic
Copy link
Member

@prattmic prattmic commented Mar 18, 2022

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

@mknyszek mknyszek commented Mar 18, 2022

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

@prattmic prattmic commented Mar 18, 2022

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

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Mar 18, 2022

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

@aclements aclements commented Mar 21, 2022

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

@prattmic prattmic commented Apr 12, 2022

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

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

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 12, 2022

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

@gopherbot gopherbot commented Apr 12, 2022

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

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 12, 2022

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
…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
Copy link
Contributor

@bradfitz bradfitz commented May 10, 2022

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
For tailscale/go@bb6009e

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

Change-Id: Ib4a67c0f82256c62989fbba2cc9c15b728313ebd
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit to tailscale/tailscale that referenced this issue May 10, 2022
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
Contributor

@cherrymui cherrymui commented May 10, 2022

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

@mknyszek mknyszek commented May 10, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision
Projects
None yet
Development

No branches or pull requests

7 participants