Configures Miri to use tree borrows, fixes remaining errors #821
Conversation
zslayton
force-pushed
the
miri-bugfixes
branch
from
8430037
to
52569f3
Compare
| on: [push, pull_request] | ||
| on: [ push, pull_request ] |
There was a problem hiding this comment.
🗺️ Some autoformatting happened in this file.
| if let Some(ptr) = encoding_buffer_ptr { | ||
| let encoding_buffer = unsafe { ptr_to_ref::<'_, BumpVec<'_, u8>>(*ptr).as_slice() }; | ||
| // Write our top level encoding buffer's contents to the output sink. | ||
| output.write_all(encoding_buffer)?; | ||
| // Flush the output sink, which may have its own buffers. | ||
| output.flush()?; | ||
| } |
There was a problem hiding this comment.
🗺️ I ported this more succinct version from the 1.1 writer; notice that it more narrowly scopes the encoding_buffer reference.
| /// Helper function that turns a raw pointer into an immutable reference of the specified type. | ||
| /// | ||
| /// The caller is responsible for confirming that `ptr` is a valid reference to some value | ||
| /// of type `T`. | ||
| pub(crate) unsafe fn ptr_to_ref<'a, T>(ptr: *mut ()) -> &'a T { | ||
| let typed_ptr: *const T = ptr.cast(); | ||
| &*typed_ptr | ||
| } |
There was a problem hiding this comment.
🗺️ Previously we only had a ptr_to_mut_ref which meant we were getting a mutable reference even where a read-only reference would suffice.
| Some(ptr) => unsafe { ptr_to_mut_ref::<'_, BumpVec<'_, u8>>(*ptr).as_slice() }, | ||
| Some(ptr) => unsafe { ptr_to_ref::<'_, BumpVec<'_, u8>>(*ptr).as_slice() }, |
There was a problem hiding this comment.
🗺️ In flush() we only need to read from the encoding buffer, so we don't need a mut ref.
| let symbol_table: &mut SymbolTable = | ||
| &mut unsafe { &mut *self.encoding_context.get() }.symbol_table; | ||
| let macro_table: &mut MacroTable = | ||
| &mut unsafe { &mut *self.encoding_context.get() }.macro_table; | ||
| let encoding_context_ref = unsafe { &mut *self.encoding_context.get() }; | ||
| let EncodingContext { | ||
| macro_table, | ||
| symbol_table, | ||
| .. | ||
| } = encoding_context_ref; |
There was a problem hiding this comment.
🗺️ Here we get a single mutable reference and then use safe code to destructure it into two mutable references.
| let allocator: &BumpAllocator = self.context().allocator(); | ||
| let context_ref = EncodingContextRef::new(allocator.alloc_with(|| self.context())); | ||
| let context_ref = self.context(); | ||
|
|
There was a problem hiding this comment.
🗺️ This was copying the EncodingContextRef to the bump allocator so it would survive the lifetime 'top...but it was already going to survive that because the EncodingContext lives in the ExpandingReader.
Related to #794.
Miri's tree borrows aliasing model is more permissive than stacked borrows (the default), which is known to reject some programs that are actually safe.
This PR configures our CI
miricheck to use tree borrows and addresses several of Miri's complaints. This is enough to satisfy the tests currently in CI (the tests for the experimentalserdefeature).There are still some issues to investigate. When running Miri on the entire test suite, it complains about at least one more problem, though it's unclear whether it's actually a potential problem or another limitation in its aliasing model. This patch is an improvement, however, and I'd rather not block these changes as we look into the others.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.