-
Notifications
You must be signed in to change notification settings - Fork 112
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
Alter PAGE_SIZE usage #534
Conversation
#485 I believe this will fix the crashes that @misakikasumi observed when trying to build Windows Huge page support. |
@nwf-msr this primarily affects your PowerPC setup, so if you could make sure I haven't broken anything that would be great. |
This breaks a debug build of
|
Ah, a little prodding and some belated understanding: snmalloc/src/snmalloc/backend_helpers/smallbuddyrange.h Lines 172 to 180 in 1b8aa6b
needs to grab things in multiples of OS_PAGE_SIZE otherwise the mprotect insidesnmalloc/src/snmalloc/pal/pal_posix.h Lines 227 to 242 in 1b8aa6b
will return EINVAL (as it is here; should we SNMALLOC_ASSERT on that returning 0?). The assert isn't saving us because CommitRange passes NoZero :snmalloc/src/snmalloc/backend_helpers/commitrange.h Lines 26 to 32 in 1b8aa6b
Maybe the PAL's assertion should actually be
or maybe just
since |
Moving to draft. To fix this issue is going to conflict with #530. To fix, we need
|
Using PAGE_SIZE as the minimum size of the CHUNK means that if this is configured to 2MiB, then there is a gap between MAX_SMALL_SIZECLASS_SIZE, and MIN_CHUNK_SIZE, and thus we can't represent certain sizes,
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.
LGTM and passes on POWER
Using PAGE_SIZE as the minimum size of the CHUNK means that if this is
configured to 2MiB, then there is a gap between
MAX_SMALL_SIZECLASS_SIZE, and MIN_CHUNK_SIZE, and thus
we can't represent certain sizes.
This removes that connection, and adds a static assert to check for the issue.