test: fix test for OOM instead of overflow#320
Merged
fitzgen merged 1 commit intofitzgen:mainfrom Apr 20, 2026
Merged
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.
graphite-app Bot
pushed a commit
to oxc-project/oxc
that referenced
this pull request
Apr 24, 2026
Fix a test for `Bump` which was failing on 32-bit in CI. #21291 temporarily disabled the test. Turn out, there's no easy way to make this test work on 32-bit platforms, and #21510 added ample unit tests which effectively test the same thing. So keep it disabled on 32-bit, but fix the test to make it work properly on 64-bit. Rationale for the fix is explained in fitzgen/bumpalo#320.
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.