[flash/mem] Significant changes to how Manticore does memory management #48
Conversation
|
@jrvanwhy Would you mind looking over this? Please let me know if something I've done seems odd. |
| /// Calling `alloc(0)` must never fail. | ||
| /// Calling `alloc(0, 1)` must never fail. Note that there is no | ||
| /// requirement that calling `alloc(0, n)` will not return a subslice | ||
| /// of previously returned memory. |
There was a problem hiding this comment.
Doesn't calling alloc_aligned(0, 1) fail for OutOfMem? Should OutOfMem be willing to serve up 0-sized allocations?
There was a problem hiding this comment.
I fixed it so that it can actually serve really aligned allocations of zero-size.
| Ok(lv.into_mut()) | ||
| } | ||
|
|
||
| fn alloc_slice<T: AsBytes + FromBytes + Copy>( |
There was a problem hiding this comment.
It's unclear to me whether ArenaExt::alloc_slice will panic, as is required by the trait method documentation (line 154).
There was a problem hiding this comment.
It can panic due to calls of alloc_aligned.
There was a problem hiding this comment.
Sorry, I worded my comment poorly. What is meant was whether it panics must be documented.
src/mem/mod.rs
Outdated
|
|
||
| /// Computes the stride of `T`, that is, its size rounded up to its alignment. | ||
| #[inline] | ||
| pub(in crate) fn stride_of<T>() -> usize { |
There was a problem hiding this comment.
core::mem::size_of::<T>() is guaranteed to be a multiple of core::mem::align_of::<T>() already. See the second sentence of the size_of docs:
More specifically, this is the offset in bytes between successive elements in an array with that item type including alignment padding. Thus, for any type
Tand lengthn,[T; n]has a size ofn * size_of::<T>().
Rust's size_of refers to the same concept as Swift calls "stride". If Rust ever has a distinction akin to Swift's "size"/"stride" distinction, then Rust's equivalent to Swift's "size" will need to be called something else. Here's some more background on the topic: rust-lang/rust#33266
There was a problem hiding this comment.
Wait really? I swear that Rust did not promise this. TIL!
| let mask = align.saturating_sub(1); | ||
| let (addr, overflow) = addr.overflowing_add(mask); | ||
| if overflow { | ||
| return usize::MAX; |
There was a problem hiding this comment.
You may want to mention that in the case of overflow, this will return an unaligned value. I think returning usize::MAX is a bit of a footgun, although my only suggestion is "document it better".
|
Jon: I replied to your suggestions. Let me know if you notice any other oddities. |
Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>
b6affa3 to
abd5356
Compare
Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>
It turns out that FlashZero was too restrictive, since it coupled arena semantics too closely with a flash object. The new API is similar in spirit, but allows for different arenas to be used with different allocations, while also allowing for Flash implementations to optimize whether they need to touch the arena at all. Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>
Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>
Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>
Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>
Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>
Signed-off-by: Miguel Young de la Sota <mcyoung@google.com>
This PR is the result of a lot of pain and suffering on my part trying to make the existing Arena and Flash APIs do the right thing for parsing the PFM. I discovered a few things:
&'flash Flash.The result is this PR, which contains a number of interrelated changes to how Arenas and Flashes interact. The most important once is the elimination of FlashZero, which is replaced with
Flash::direct_read. This function allows the user to request a zero-copy read, providing their own arena for the allocation if the request can't be fulfilled.