Skip to content

Conversation

@sklose
Copy link
Owner

@sklose sklose commented Dec 5, 2024

to address issues #6

@sklose sklose force-pushed the bug/reproduce-issue-6 branch 5 times, most recently from dc76d82 to 7c838e3 Compare December 7, 2024 02:47
@edef1c
Copy link
Contributor

edef1c commented Dec 12, 2024

Note that just asm!("/* {ptr} */", ptr = in(reg) = buf.ptr, options(nostack, preserves_flags)) as a barrier is enough: we don't actually need to do the pointer computation in assembly. Per the asm! spec, rustc isn't allowed to inspect the assembly and rely on those instructions being the ones actually executed, so we can use it to represent the MMU's implicit memcpy. This avoids needing architecture-specific assembly.

@sklose sklose force-pushed the bug/reproduce-issue-6 branch 3 times, most recently from a62c483 to 70e58b0 Compare December 14, 2024 20:47
@sklose sklose force-pushed the bug/reproduce-issue-6 branch from 70e58b0 to 7b69a17 Compare December 14, 2024 20:55
@sklose sklose changed the title reproduce undef behavior in unit test fix undef behavior caused by memory aliasing Dec 14, 2024
@sklose sklose marked this pull request as ready for review December 14, 2024 20:58
@sklose sklose added the bug Something isn't working label Dec 14, 2024
@sklose
Copy link
Owner Author

sklose commented Dec 14, 2024

Thanks, @edef1c. I appreciate your help with this. It looks like the test is passing now. Could you give it a look over before I merge it and publish a new version?

@edef1c
Copy link
Contributor

edef1c commented Dec 18, 2024

The Index/IndexMut code looks good. But I'm not sure we can hide need for fencing from the public API for as_ptr / as_mut_ptr, since users of those methods are likely to do things that require fencing themselves. The best approach I've found is explicitly exposing a pub fn fence(&self) { … } and clearly documenting the need, but we can't really stop people from shooting themselves in the foot.

The API I've adopted for the unsafe raw buffer library in my own codebases looks like this:

	/// Make the buffer contents consistent by (logically) copying memory.
	pub fn fence(&self) {
		unsafe {
			// the MMU does the actual memcpy, so this requires no CPU instructions
			asm!("/* {ptr} */", ptr = in(reg) self.ptr.as_ptr(), options(nostack, preserves_flags));
		}
	}

	/// SAFETY: The buffer segment needs to be in a consistent state, and `len` needs to be in bounds.
	pub unsafe fn get(&self, offset: usize, len: usize) -> NonNull<[u8]> {
		debug_assert!(len <= self.capacity());
		NonNull::slice_from_raw_parts(
			NonNull::new_unchecked(self.ptr.as_ptr().add(offset & self.mask.get())),
			len,
		)
	}

(nb: the SAFETY comment isn't quite right yet, since it's really about dereferenceability rather than about when get is invoked, and in principle merely producing the pointer isn't unsafe in its own right)

The discipline the calling code adopts is to invoke the fence method before making an &mut [u8] available and when committing written data to the buffer. That doesn't really gel with the Index / IndexMut API of magic-buffer, where we don't have a good place to hook the "change of control" of buffer segments, but we can get away with hiding that by fencing on both mutable and immutable indexing, as this PR does; it just doesn't carry over cleanly to the raw API.

I think we're a little stuck here, and the most pragmatic option might be to deprecate the current raw pointer APIs, and introduce new ones with clearer safety properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants