-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Align atomics on ARM32 #11822
Align atomics on ARM32 #11822
Conversation
@xacrimon @espadolini oops, looks like my review wasn't needed after all. Feel free to ignore my approval :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making these pointers is kind of cheating. Why not rearrange the fields to ensure alignment?
https://cs.opensource.google/go/go/+/refs/tags/go1.18:src/sync/waitgroup.go;l=23-30 |
@zmb3 Go's spec nor any official docs guarantee that multiple fields types like that will work as far as I am aware. The only guarantee given is that the first field is 64-bit aligned on ARM32 for an allocated struct or type. This is the de-facto way from what I've seen to accomplish this. Even if this somehow works currently, I don't put a lot of trust into the Go compatability guarantees either and I wouldn't expect that to continue working in the future. |
On second thought, we could probably get away with not aligning the 32 bit counters and hope that this doesn't break. I do know that it may be required in some cases on some chips but this probably isn't something we should support as it's not ARMv7 compliant anyway. |
We do use the allocation pattern in other places. I will post a PR soon since I did just get some ideas for cleaning that up. |
Right, and if you have multiple 64-bit fields, and you put them together, they'll all be aligned. This LGTM as-is now. |
I'm not so sure this is guaranteed (at least I couldn't find it documented in the spec or as a part of the stdlib docs) but it doesn't matter for this PR. |
Co-authored-by: Zac Bergquist <[email protected]>
Fixes #11106 (comment)
Needs backport to v8 and v9