Skip to content

Commit

Permalink
threads: log every wait and notify
Browse files Browse the repository at this point in the history
When troubleshooting deadlocks in WebAssembly modules, it is important
to understand which `wait` instructions are still pending a `notify`.
It would be nice to have some kind of `--warn-deadlock-after=1s` flag
available that would poll the parking lot for `wait`s hanging past the
time limit, but I realized the real value would be to tie the `wait`
instruction (through CLIF) to the original source code, if debug
information were available. This did not seem to be entirely feasible,
since CLIF loses the original Wasm source context (is this true?) and I
was not confident that we would be able to use `addr2line` to map from
Wasm instructions to source (e.g., see @cfallin's
[issue](gimli-rs/addr2line#265)).

Instead, this change simply logs each valid `wait` and `notify`
execution, leaving it to the user to figure out which one is hanging
(should not be too difficult) and how to map this back to their source
code (more difficult).
  • Loading branch information
abrown committed Oct 11, 2023
1 parent 8101bf9 commit ff8ff0f
Showing 1 changed file with 12 additions and 2 deletions.
14 changes: 12 additions & 2 deletions crates/runtime/src/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,7 @@ impl SharedMemory {
/// Implementation of `memory.atomic.notify` for this shared memory.
pub fn atomic_notify(&self, addr_index: u64, count: u32) -> Result<u32, Trap> {
validate_atomic_addr(&self.0.def.0, addr_index, 4, 4)?;
log::trace!("memory.atomic.notify(addr={addr_index}, count={count})");
Ok(self.0.spot.unpark(addr_index, count))
}

Expand All @@ -576,13 +577,18 @@ impl SharedMemory {
timeout: Option<Instant>,
) -> Result<WaitResult, Trap> {
let addr = validate_atomic_addr(&self.0.def.0, addr_index, 4, 4)?;
log::trace!(
"memory.atomic.wait32(addr={addr_index}, expected={expected}, timeout={timeout:?})"
);

// SAFETY: `addr_index` was validated by `validate_atomic_addr` above.
assert!(std::mem::size_of::<AtomicU32>() == 4);
assert!(std::mem::align_of::<AtomicU32>() <= 4);
let atomic = unsafe { &*(addr as *const AtomicU32) };

// We want the sequential consistency of `SeqCst` to ensure that the `load` sees the value that the `notify` will/would see.
// All WASM atomic operations are also `SeqCst`.
// We want the sequential consistency of `SeqCst` to ensure that the
// `load` sees the value that the `notify` will/would see. All WASM
// atomic operations are also `SeqCst`.
let validate = || atomic.load(Ordering::SeqCst) == expected;

Ok(self.0.spot.park(addr_index, validate, timeout))
Expand All @@ -596,6 +602,10 @@ impl SharedMemory {
timeout: Option<Instant>,
) -> Result<WaitResult, Trap> {
let addr = validate_atomic_addr(&self.0.def.0, addr_index, 8, 8)?;
log::trace!(
"memory.atomic.wait64(addr={addr_index}, expected={expected}, timeout={timeout:?})"
);

// SAFETY: `addr_index` was validated by `validate_atomic_addr` above.
assert!(std::mem::size_of::<AtomicU64>() == 8);
assert!(std::mem::align_of::<AtomicU64>() <= 8);
Expand Down

0 comments on commit ff8ff0f

Please sign in to comment.