mem::replace don't actually mutate the destination on Android

[jdm]

I have a branch of Servo which crashes when run on Android because a key use of mem::replace does not actually cause the destination to change values. When I add println!("{:?}", self.pending_line) before and after the mem::replace, on desktop the values are updated as expected, but on Android the original value remains.

When I use either of the following instead:

self.pending_line = Line::new(self.floats.writing_mode, &self.minimum_metrics);

or

let mut new_line = Line::new(self.floats.writing_mode, &self.minimum_metrics);
mem::swap(&mut self.pending_line, &mut new_line);

then self.pending is updated as expected and Servo does not crash on Android.

[jdm]

The following also works:

        mem::replace(&mut self.pending_line,
                     Line::new(self.floats.writing_mode, &self.minimum_metrics));
        mem::replace(&mut self.pending_line,
                     Line::new(self.floats.writing_mode, &self.minimum_metrics));

(yes, performing the same operation twice)

[scottmcm]

Hmm, it looks like that type is pretty large (https://github.com/jdm/servo/blob/layoutstuff/components/layout/inline.rs#L221-L245) so will be hitting the simd path in swap (https://github.com/rust-lang/rust/blob/master/src/libcore/ptr.rs#L194-L227) that's currently disabled on some other platforms, so I wonder if the bug's in there somewhere...

[jonas-schievink]

Triage: Not sure if this is still an issue.

Updated libcore SIMD code:

rust/src/libcore/ptr/mod.rs

Lines 418 to 465 in 8ce3f84

	#[inline]
	unsafe fn swap_nonoverlapping_bytes(x: *mut u8, y: *mut u8, len: usize) {
	// The approach here is to utilize simd to swap x & y efficiently. Testing reveals
	// that swapping either 32 bytes or 64 bytes at a time is most efficient for Intel
	// Haswell E processors. LLVM is more able to optimize if we give a struct a
	// #[repr(simd)], even if we don't actually use this struct directly.
	//
	// FIXME repr(simd) broken on emscripten and redox
	#[cfg_attr(not(any(target_os = "emscripten", target_os = "redox")), repr(simd))]
	struct Block(u64, u64, u64, u64);
	struct UnalignedBlock(u64, u64, u64, u64);
	let block_size = mem::size_of::<Block>();
	// Loop through x & y, copying them `Block` at a time
	// The optimizer should unroll the loop fully for most types
	// N.B. We can't use a for loop as the `range` impl calls `mem::swap` recursively
	let mut i = 0;
	while i + block_size <= len {
	// Create some uninitialized memory as scratch space
	// Declaring `t` here avoids aligning the stack when this loop is unused
	let mut t = mem::MaybeUninit::<Block>::uninit();
	let t = t.as_mut_ptr() as *mut u8;
	let x = x.add(i);
	let y = y.add(i);
	// Swap a block of bytes of x & y, using t as a temporary buffer
	// This should be optimized into efficient SIMD operations where available
	copy_nonoverlapping(x, t, block_size);
	copy_nonoverlapping(y, x, block_size);
	copy_nonoverlapping(t, y, block_size);
	i += block_size;
	}
	if i < len {
	// Swap any remaining bytes
	let mut t = mem::MaybeUninit::<UnalignedBlock>::uninit();
	let rem = len - i;
	let t = t.as_mut_ptr() as *mut u8;
	let x = x.add(i);
	let y = y.add(i);
	copy_nonoverlapping(x, t, rem);
	copy_nonoverlapping(y, x, rem);
	copy_nonoverlapping(t, y, rem);
	}
	}

[jdm]

Realistically I don't think there's any point to leaving this open. We never tried to make a minimal reproduction, and nobody else has stumbled across this issue in the intervening five years.

[scottmcm]

Agreed; the implementation of replace is also very different now after #83022

rust/library/core/src/mem/mod.rs

Lines 908 to 912 in 70adb4e

	unsafe {
	let result = ptr::read(dest);
	ptr::write(dest, src);
	result
	}