Add shared linear memory as prerequisite for wasip1-threads#359
Add shared linear memory as prerequisite for wasip1-threads#359MaxDesiatov wants to merge 11 commits into
wasip1-threads#359Conversation
A `shared` memory is backed by `SharedMemoryStorage`, which reserves its full address range up front so the base pointer stays stable, then commits pages on `memory.grow` under a spinlock that the SIGSEGV/SIGBUS handler also takes, so a concurrent grow cannot race an in-flight access; `memory.atomic.wait32/64` and `memory.atomic.notify` block and wake threads through `AtomicParkingLot`. Adds `SharedMemoryStorageTests` and `AtomicParkingLotTests`.
kateinoigakukun
left a comment
kateinoigakukun
left a comment
There was a problem hiding this comment.
Let's split out AtomicParkingLot's pthread_cond_t change to make review easier 🙇
| // Shared memory always uses software bounds checking. A guard-page fault | ||
| // recovered by siglongjmp would unwind the interpreter past a lock held by a | ||
| // concurrent grow or atomic operation, so the multi-threaded path must stay | ||
| // free of non-local jumps. mprotect guards are reserved for non-shared memory. | ||
| let isNonSharedMemory = sp.currentInstance?.memories.first?.withValue { $0.sharedStorage == nil } ?? true | ||
| let shouldUseMprotectTrapGuards = | ||
| store.value.engine.configuration.memoryBoundsChecking == .mprotect && isNonSharedMemory |
There was a problem hiding this comment.
Hmm, I actually didn't get this. Is it even theoretically possible that "thread X causes Wasm OOB access while owning memory.grow or atomic.wait lock? Given that siglongjmp is a thread local stack unwinding, I guess it won't be possible to happen?
| /// A type that can be interned into a unique identifier. | ||
| /// Used for efficient equality comparison. | ||
| protocol Internable { | ||
| package protocol Internable { |
There was a problem hiding this comment.
Q: Why do we need to expose some structures in this file to package level?
|
|
||
| /// If this memory is shared, the mmap-backed storage is held here. | ||
| /// Multiple `MemoryEntity` instances (across threads) may reference the same `SharedMemoryStorage`. | ||
| let sharedStorage: SharedMemoryEntity? |
There was a problem hiding this comment.
I think the memory storage should be consistently represented by let storage member to avoid having many if let sharedStorage below and we can remove shared-variant of memory instructions. Also even further, I'm wondering if we really need a new SharedMemoryEntity type here, can't we add shared mode to MprotectLinearMemory?
2 participants
Added corresponding shared memory load/instructions that call
reloadSharedMemorySize, updated atomic instructions to do the same.