allocator_api tests failing

[overlookmotel]

The CI failure discovered in #229 is also failing on main (reproduced locally on M1 Macbook Pro).

cargo test --features allocator_api on nightly is sufficient to trigger it.

The failing test is:

bumpalo/tests/all/allocator_api.rs

Lines 163 to 211 in f8278d6

	fn allocator_shrink_align_change(layouts: Vec<(usize, usize)>) -> bool {
	let mut layouts: Vec<_> = layouts.into_iter().map(|(size, align)| {
	const MIN_SIZE: usize = 1;
	const MAX_SIZE: usize = 1024;
	const MIN_ALIGN: usize = 1;
	const MAX_ALIGN: usize = 64;
	let align = usize::min(usize::max(align, MIN_ALIGN), MAX_ALIGN);
	let align = usize::next_power_of_two(align);
	let size = usize::min(usize::max(size, MIN_SIZE), MAX_SIZE);
	let size = usize::max(size, align);
	Layout::from_size_align(size, align).unwrap()
	}).collect();
	layouts.sort_unstable_by_key(|l| l.size());
	layouts.reverse();
	let b = AllocatorDebug::new(Bump::new());
	let mut layout_iter = layouts.into_iter();
	if let Some(initial_layout) = layout_iter.next() {
	let mut pointer = b.allocate(initial_layout).unwrap();
	if !is_pointer_aligned_to(pointer, initial_layout.align()) {
	return false;
	}
	let mut old_layout = initial_layout;
	for new_layout in layout_iter {
	let res = unsafe { b.shrink(pointer.cast(), old_layout, new_layout) };
	if old_layout.align() < new_layout.align() {
	if res.is_ok() {
	return false;
	}
	} else {
	pointer = res.unwrap();
	if !is_pointer_aligned_to(pointer, new_layout.align()) {
	return false;
	}
	old_layout = new_layout;
	}
	}
	}
	true
	}

Have traced it to:

bumpalo/src/lib.rs

Lines 1705 to 1745 in f8278d6

	unsafe fn shrink(
	&self,
	ptr: NonNull<u8>,
	old_layout: Layout,
	new_layout: Layout,
	) -> Result<NonNull<u8>, AllocErr> {
	let old_size = old_layout.size();
	let new_size = new_layout.size();
	let align_is_compatible = old_layout.align() >= new_layout.align();
	if !align_is_compatible {
	return Err(AllocErr);
	}
	// This is how much space we would *actually* reclaim while satisfying
	// the requested alignment.
	let delta = round_down_to(old_size - new_size, new_layout.align());
	if self.is_last_allocation(ptr)
	// Only reclaim the excess space (which requires a copy) if it
	// is worth it: we are actually going to recover "enough" space
	// and we can do a non-overlapping copy.
	&& delta >= old_size / 2
	{
	let footer = self.current_chunk_footer.get();
	let footer = footer.as_ref();
	// NB: new_ptr is aligned, because ptr *has to* be aligned, and we
	// made sure delta is aligned.
	let new_ptr = NonNull::new_unchecked(footer.ptr.get().as_ptr().add(delta));
	footer.ptr.set(new_ptr);
	// NB: we know it is non-overlapping because of the size check
	// in the `if` condition.
	ptr::copy_nonoverlapping(ptr.as_ptr(), new_ptr.as_ptr(), new_size);
	Ok(new_ptr)
	} else {
	Ok(ptr)
	}
	}

That test seems to produce a lot of calls to Bump::shrink where old_size and new_size are both 1, and ptr and new_ptr are equal (so the copy is indeed overlapping).

In these cases, delta == 0 and old_size / 2 == 0, so (delta >= old_size / 2) == true.

Changing that test to delta > 0 && delta >= old_size / 2 solves this specific problem (cargo test --all-features passes), but I'm not sure if it still would allow other illegal cases through.

I'm afraid I don't understand the logic well enough to make a PR, but I hope this may be helpful in finding a fix.

[overlookmotel]

Maybe delta >= (old_size + 1) / 2 so the division rounds up instead of down?

[overlookmotel]

Yes, I think delta >= (old_size + 1) / 2 is right. Have made a PR #232.