oonitemplates: remove code causing crash on x86/arm64 #360
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
The matter is better explained in the newly added code comment. I feel okay removing the code, because that was a okay-lets-test-it piece of code that was only useful for running such test. A key takeaway from the investigation of this issue is that we really want to simplify and streamline the way in which netx interacts with the rest of the world, to avoid using channels where we can more easily have (1) code that directly logs, if logging is the intent, and (2) code that directly saves data, if that is the intent. I will open an issue to document these changes more in depth. Here as a general reflection, I'd just like to reiterate that if we were using less complex/general channel based patterns, we would be reducing the odds that we see some unexpected behaviour like in this case. Another aspect is that we should have QA running everytime we do generate a new Android build - see #358.
bassosimone
added a commit
that referenced
this pull request
Mar 18, 2020
* Revert "oonitemplates: remove code causing crash on x86/arm64 (#360)" This reverts commit 4e3bde3 because it makes sense to check whether this crash was actually caused by the alignment instead #399. Conflicts: internal/oonitemplates/oonitemplates.go internal/oonitemplates/oonitemplates_test.go * Replace the atomic package with atomicx The atomicx package is less efficient however it completely avoids using atomic semantics on int64, which may lead to crash if the variable we are accessing is not properly aligned. See https://golang.org/pkg/sync/atomic/#pkg-note-BUG, which reads: > On x86-32, the 64-bit functions use instructions unavailable > before the Pentium MMX. > > On non-Linux ARM, the 64-bit functions use instructions > unavailable before the ARMv6k core. > > On ARM, x86-32, and 32-bit MIPS, it is the caller's responsibility > to arrange for 64-bit alignment of 64-bit words accessed atomically. The > first word in a variable or in an allocated struct, array, or slice can > be relied upon to be 64-bit aligned. By using a separate package we are able to possibly even change the way in which we implement the code, and surely we are able to shield ourselves from this pitfalls by using a mutex. While in general it may not be okay to always use a mutex, we mostly use atomic semantics for counting. That is, we are not using it in performance critical parts of the tree, hence this fix seems fine to me. Related issue #399.
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.
The matter is better explained in the newly added code comment. I feel
okay removing the code, because that was a okay-lets-test-it piece of
code that was only useful for running such test.
A key takeaway from the investigation of this issue is that we really
want to simplify and streamline the way in which netx interacts with
the rest of the world, to avoid using channels where we can more easily
have (1) code that directly logs, if logging is the intent, and (2)
code that directly saves data, if that is the intent.
I will open an issue to document these changes more in depth. Here as
a general reflection, I'd just like to reiterate that if we were using
less complex/general channel based patterns, we would be reducing the
odds that we see some unexpected behaviour like in this case.
Another aspect is that we should have QA running everytime we do
generate a new Android build - see #358.
Closes #355