Skip to content

fix: 32bit-ARMv7 crashes on android #2654

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

Merged

Conversation

NoelStephensUnity
Copy link
Collaborator

This PR resolves the issue with ARMv7 builds crashing on the Android.

fix: #2646

Changelog

  • Fixed: Issue where ARMv7 Android builds would crash when trying to validate the batch header.

Testing and Documentation

  • Includes integration tests (?)
  • No documentation changes or additions were necessary.

Temporarily commenting out the batch header hash check to have a user validate that this fixes the issue with Android Armv7 builds.
This resolves the issue with XXHash causing ARMv7 builds to crash when validating the batched message header and payload.
Keep messages sent word aligned.
Use the 32bit version of XXHash.
Prevent padding on the NetworkBatchHeader
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review August 7, 2023 21:16
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner August 7, 2023 21:16
NoelStephensUnity and others added 3 commits August 7, 2023 16:40
fixing whitespace issue with the other test changes.
This resolves the tools bytes measured tests.
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner August 8, 2023 16:16
NoelStephensUnity and others added 5 commits August 8, 2023 11:41
This resolves the tools bytes measured tests.
switching back to 64 bit hash values for batch header validation.
reverting the switch to 32 bit hash values.
reverting the conditional registration of either 32bit or 64 bit hash values in either table to just register in both tables.
 reverting the additional CR/LF
reverting the code to be exactly as it was (no need to make organizational changes)
removing the alignment calculation from the observed byte count.
Removing additional CR/LFs.
removing extra LF/CRs.
@simon-lemay-unity
Copy link
Contributor

Will let you decide if you think it's worth the hit to readability, but you can calculate aligned lengths this way:

alignedLength = (length + 7) & ~7;

It avoids dealing with floating points and only uses fast integer instructions.

@ShadauxCat
Copy link
Collaborator

Will let you decide if you think it's worth the hit to readability, but you can calculate aligned lengths this way:

alignedLength = (length + 7) & ~7;

It avoids dealing with floating points and only uses fast integer instructions.

Oh good point. +1 to this @NoelStephensUnity

Applying Simon's suggestion.
This update assures we won't go above the MTU size due to word alignment.
removing VS 2022 auto-assigned namespace...<grr>
@NoelStephensUnity NoelStephensUnity merged commit cbc2d72 into develop Aug 8, 2023
@NoelStephensUnity NoelStephensUnity deleted the fix/32bit-custom-named-message-crash-android branch August 8, 2023 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Game crashes when joining on Android 11?
5 participants