Rain (they/them) (Nov 19 2024 at 02:43): Rain (they/them) (Nov 19 2024 at 02:43):hi! thought I'd create a stream for #9544 as part of making wasmtime fully functional on illumos, and also getting rid of a lot of unsafety.
I started off with #9620 to add a type-level wrapper around usize to ensure that it's always host page aligned
Rain (they/them) (Nov 19 2024 at 03:25):A question I had is that https://github.com/bytecodealliance/wasmtime/blob/main/crates/wasmtime/src/runtime/vm/mmap_vec.rs seems to suggest that there's MmapVec::split_at but that doesn't appear to exist. Is that WIP?
Notification Bot (Nov 19 2024 at 15:37):Alex Crichton has marked this topic as unresolved.
Alex Crichton (Nov 19 2024 at 15:38):Oh that method used to exist but no longer does. Which as I think more about this obviates the need for MmapVec entirely...
Alex Crichton (Nov 19 2024 at 19:03):I needed to re-rationalize the docs in https://github.com/bytecodealliance/wasmtime/pull/9614 anyway so I went ahead and removed the Arc as part of that commit and shifted around some unsafe bits
Rain (they/them) (Nov 19 2024 at 19:34):thanks! sorry about the from_file booboo. I thought those would be page aligned too
Rain (they/them) (Nov 19 2024 at 19:34):(should they be? feels like a good invariant to have)
Rain (they/them) (Nov 19 2024 at 23:00):Ah no that wouldn't work. However it looks like the only way to construct a file-backed mmap is using MmapVec -- which suggests that maybe we should have a type-level distinction between aligned and unaligned mmaps
Alex Crichton (Nov 19 2024 at 23:13):ah yeah so Mmap I think should always be page-aligned but MmapVec is not guaranteed to be page-aligned
Alex Crichton (Nov 19 2024 at 23:13):so that'll be the point where sizes are rounded-up and the length of MmapVec will be a plain old usize
Rain (they/them) (Nov 19 2024 at 23:14):Well -- Mmap::from_file is not page aligned, right?
Rain (they/them) (Nov 19 2024 at 23:14):the start is page aligned but the length might not be
Rain (they/them) (Nov 19 2024 at 23:16):https://github.com/bytecodealliance/wasmtime/blob/main/crates/wasmtime/src/runtime/vm/sys/unix/mmap.rs#L93 -- there's no rounding up of len happening
Alex Crichton (Nov 19 2024 at 23:17):oh right yeah good point
Alex Crichton (Nov 19 2024 at 23:18):I had a rough idea that the constructor there would return (Mmap, usize) where usize was the length of the mapping created byte-wise, but the Mmap would store the rounded-up size as the true length
Alex Crichton (Nov 19 2024 at 23:18):(assuming OSes actually round up and give us ownership of the whole page)
Rain (they/them) (Nov 19 2024 at 23:23):Looks like it's rounded up and zeroed out on Linux: https://man7.org/linux/man-pages/man2/mmap.2.html#NOTES
But POSIX doesn't commit to anything: https://pubs.opengroup.org/onlinepubs/9799919799/functions/mmap.html
Rain (they/them) (Nov 20 2024 at 00:16):Yeah I just had a chat with Bryan Cantrill and he said to not assume anything about the rest of memory
Alex Crichton (Nov 20 2024 at 07:18):This may mean we can't store a page aligned length in mmap in that case? That feels a bit unfortunate though... Could also use a separate field to clamp lengths of read-only/executable memory?
Rain (they/them) (Nov 20 2024 at 08:48):I have a draft working with a type-level assertion of this. will post tomorrow
Rain (they/them) (Nov 20 2024 at 08:49):Basically making two versions of Mmap, one that is aligned and one that isn't. From my read of the code they seem to be two generally different kinds of operations that happen to use similar Unix APIs
Alex Crichton (Nov 20 2024 at 15:43):I think it'd be reasonable to split out yeah, especially if one platform has totally different APIs for managing one vs the other (which I suppose is sort of like how Windows treats it)
Rain (they/them) (Nov 20 2024 at 20:14):put up a PR for what I had in mind: https://github.com/bytecodealliance/wasmtime/pull/9639 -- I've rebased 9620 on top of it and it works well
Alex Crichton (Nov 20 2024 at 20:53):Looks great, thanks!
Rain (they/them) (Nov 20 2024 at 20:54):awesome, thanks @Alex Crichton. slowly inching towards making this code less terrifying haha
Rain (they/them) (Nov 22 2024 at 05:28):What's the reason host_page_size is behind the signals-based-traps feature right now? I need HostAlignedByteCount, and therefore host_page_size, as part of the RuntimeLinearMemory API which isn't behind that feature
Rain (they/them) (Nov 22 2024 at 05:32):(specifically for the byte_size function, which returns a multiple of the host page size)
Last updated: Nov 22 2024 at 17:03 UTC