[Merged by Bors] - Ensure MessageServer.ReqID is 8 byte aligned #2346
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Atomic operations such as atomic.AddUint64 must operate on 64-bit aligned memory when running on 32-bit systems, otherwise a runtime will panic occur. This is a known issue in go and might addressed in a future release, see golang/go#36606.
noamnelke
approved these changes
Mar 25, 2021
There was a problem hiding this comment.
This looks good and we should merge it, but I worry that this type of issue will recur in the future if we don't have a way to automatically test it. Is there any way you know to test for this issue on a 64-bit system (ideal, since we'd be able to test it on our development machines)? Otherwise I can try to find out if Github actions provides 32-bit systems to test on.
|
I'm not sure how we can create a test that will break on a 64 bit system, the next best thing could be to run a static linter over the whole code base. For example the original issue was found using go staticheck in vscode, see: https://staticcheck.io/docs/checks#SA1027. Perhaps running a linter could be added as a test to the CI system? |
|
Yes, the |
antonlerner
approved these changes
Jul 13, 2021
|
bors merge |
bors bot
pushed a commit
that referenced
this pull request
Jul 13, 2021
Atomic operations such as atomic.AddUint64 must operate on 64-bit aligned memory when running on 32-bit systems, otherwise a runtime will panic occur. This is a known issue in go and might be addressed in a future release, see golang/go#36606. ## Motivation Using atomic.AddUint64 on non 64 bit aligned address on 32 bit systems results in a runtime panic. The solution is to guarantee the data it operates on is 64 bit aligned. This can be done by defining such fields as the first item in a struct. See #2336. Closes #2336 ## Changes Moving ReqID to the top of MessageServer guarantees 8 byte alignment. See https://go101.org/article/memory-layout.html section _The Alignment Requirement for 64-bit Word Atomic Operations_ for details on why this works. ## Test Plan This was tested on a Raspberry Pi 4B (armv7l) and on linux x86_64 resulting in no breaking changes. ## TODO - [ ] Test changes ## DevOps Notes - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on) Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>
|
Build failed: |
|
bors merge |
bors bot
pushed a commit
that referenced
this pull request
Jul 13, 2021
Atomic operations such as atomic.AddUint64 must operate on 64-bit aligned memory when running on 32-bit systems, otherwise a runtime will panic occur. This is a known issue in go and might be addressed in a future release, see golang/go#36606. ## Motivation Using atomic.AddUint64 on non 64 bit aligned address on 32 bit systems results in a runtime panic. The solution is to guarantee the data it operates on is 64 bit aligned. This can be done by defining such fields as the first item in a struct. See #2336. Closes #2336 ## Changes Moving ReqID to the top of MessageServer guarantees 8 byte alignment. See https://go101.org/article/memory-layout.html section _The Alignment Requirement for 64-bit Word Atomic Operations_ for details on why this works. ## Test Plan This was tested on a Raspberry Pi 4B (armv7l) and on linux x86_64 resulting in no breaking changes. ## TODO - [ ] Test changes ## DevOps Notes - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on) Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>
|
Build failed: |
|
bors merge |
bors bot
pushed a commit
that referenced
this pull request
Jul 13, 2021
Atomic operations such as atomic.AddUint64 must operate on 64-bit aligned memory when running on 32-bit systems, otherwise a runtime will panic occur. This is a known issue in go and might be addressed in a future release, see golang/go#36606. ## Motivation Using atomic.AddUint64 on non 64 bit aligned address on 32 bit systems results in a runtime panic. The solution is to guarantee the data it operates on is 64 bit aligned. This can be done by defining such fields as the first item in a struct. See #2336. Closes #2336 ## Changes Moving ReqID to the top of MessageServer guarantees 8 byte alignment. See https://go101.org/article/memory-layout.html section _The Alignment Requirement for 64-bit Word Atomic Operations_ for details on why this works. ## Test Plan This was tested on a Raspberry Pi 4B (armv7l) and on linux x86_64 resulting in no breaking changes. ## TODO - [ ] Test changes ## DevOps Notes - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on) Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>
|
bors merge- |
|
Canceled. |
|
bors merge |
bors bot
pushed a commit
that referenced
this pull request
Jul 13, 2021
Atomic operations such as atomic.AddUint64 must operate on 64-bit aligned memory when running on 32-bit systems, otherwise a runtime will panic occur. This is a known issue in go and might be addressed in a future release, see golang/go#36606. ## Motivation Using atomic.AddUint64 on non 64 bit aligned address on 32 bit systems results in a runtime panic. The solution is to guarantee the data it operates on is 64 bit aligned. This can be done by defining such fields as the first item in a struct. See #2336. Closes #2336 ## Changes Moving ReqID to the top of MessageServer guarantees 8 byte alignment. See https://go101.org/article/memory-layout.html section _The Alignment Requirement for 64-bit Word Atomic Operations_ for details on why this works. ## Test Plan This was tested on a Raspberry Pi 4B (armv7l) and on linux x86_64 resulting in no breaking changes. ## TODO - [ ] Test changes ## DevOps Notes - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on) Co-authored-by: Nikita Kryuchkov <nkryuchkov10@gmail.com>
|
Pull request successfully merged into develop. Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Atomic operations such as atomic.AddUint64 must operate on 64-bit aligned memory when running on 32-bit systems, otherwise a runtime will panic occur. This is a known issue in go and might be addressed in a future release, see golang/go#36606.
Motivation
Using atomic.AddUint64 on non 64 bit aligned address on 32 bit systems results in a runtime panic. The solution is to guarantee the data it operates on is 64 bit aligned. This can be done by defining such fields as the first item in a struct. See #2336.
Closes #2336
Changes
Moving ReqID to the top of MessageServer guarantees 8 byte alignment. See https://go101.org/article/memory-layout.html section The Alignment Requirement for 64-bit Word Atomic Operations for details on why this works.
Test Plan
This was tested on a Raspberry Pi 4B (armv7l) and on linux x86_64 resulting in no breaking changes.
TODO
DevOps Notes