test: fix test for OOM instead of overflow#320
Open
overlookmotel wants to merge 1 commit intofitzgen:mainfrom
Open
test: fix test for OOM instead of overflow#320overlookmotel wants to merge 1 commit intofitzgen:mainfrom
overlookmotel wants to merge 1 commit intofitzgen:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates an outdated OOM regression test to reflect bumpalo’s downward-bumping behavior and avoid platform-dependent failures.
Changes:
- Prevents the test from allocating a new backing chunk by setting an allocation limit to the currently allocated bytes.
- Changes the “overflow” trigger size computation to
p + 1(with added rationale about address-space assumptions) to reliably cause wrap-around behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or 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
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.
This test appears to be outdated.
It was originally introduced in e53e771, and then modified in a221762.
The test was introduced (by the looks of it) before Bumpalo switched to bumping downwards. When bumping upwards,
size = usize::MAX - p + 1made sense as a value which would be guaranteed overflow the bump pointer.a221762 fixed failures on nightly, but did not alter the basic logic. It changed
size = (isize::MAX as usize) - p + 1.On 64-bit platforms, this did still produce
sizevalues which would underflow inchunk_cursor_ptr.wrapping_sub(size), but more by accident than design - only because pointers don't use full 64-bit address space sop < (1 << 48). On 32-bit platforms where it's possiblepcould be e.g.0x3FFF_F000,sizecould be small, it might not underflow, and the test could fail becausealloc_layoutsucceeds.Fix this test by:
size = p + 1.bump_cursor.wrapping_sub(size)is guaranteed to wrap around with this value ofsize.bumpcannot allocate a new chunk to service the allocation request (possibly it could do successfully ifpis small).The test does assume that allocations are always made in bottom half of address space. If they're not,
sizewill be> isize::MAXandLayout::from_size_alignwill returnErr, so the test will fail.That assumption was already implicit in the test - previously if
p > isize::MAX, then calculatingsizewould involve arithmetic overflow, which would panic.This assumption is (to my knowledge) fairly safe on 64-bit platforms, where top half of address space is typically reserved for the kernel. But I'm not sure if it is safe on 32-bit platforms e.g. WASM - perhaps they may use the full 32-bit address space for heap.