Atomics: Handle timeouts in waits in the (single-threaded) interpreter #6408
Conversation
Unless the timeout is negative, in which case the actual correct behavior is to hang indefinitely. Do we want to model that somehow? |
|
Hmm, good question. Maybe we should error on that atm. I pushed that now. |
|
Hmm, a downside to erroring like that is that we'd need to ignore all testcases with such inputs in the fuzzer, or else we error there. It might be simpler for now to just always return 2? |
src/wasm-interpreter.h
Outdated
| // TODO: Add threads support. | ||
| // For now, as there are no other threads, if there is no timeout then | ||
| // error (this would hang forever), and if there is a timeout then | ||
| // just report that we timed out. | ||
| if (timeout.getSingleValue().getInteger() < 0) { | ||
| return Flow(NONCONSTANT_FLOW); | ||
| } |
There was a problem hiding this comment.
This should go after the not equal check if we decide to keep it.
Given that there's no way usefully check the behavior of waiting without actual threads, this seems fine to me as long as we have a comment about it. |
|
I added a comment and switched this to emitting a "host limit" trap. That is necessary for the fuzzer to know to ignore such testcases. I also made us emit that limit for any timeout that is nonzero, as not only an infinite timeout can hang but even a small finite one, inside a loop. |
kripken
changed the title
|
Aren't infinite loops already handled by separate machinery, though? |
|
These wouldn't be infinite, but just very slow. Imagine even a 1ms timeout executed a million times. |
|
That is, even a 1ms timeout would make this by far our slowest instruction 🐢 |
|
Oh sure, but I don't think we should actually implement them with a wait. No execution environment our interpreter runs in actually cares about wall clock time, right? I would expect that we would return a 2 immediately for any non-negative timeout and and return a host limit exception that the fuzzer can ignore for any negative timeout. |
|
Sorry, I should have said: The problem isn't in our interpreter but when we run V8 in the fuzzer. V8 does obey the timeouts. So it seems best to mark any timeout as a host limit for us (which then gets skipped in the fuzzer). |
|
Oh, I see the issue. Yes, this makes sense, then. |
The interpreter does not run multiple threads, and it was returning 0 from
atomic.wait, which means it was woken up. But it is more correct for it toreturn 2, which means it timed out - which is actually the case, as no other
thread exists that can wake it up. However, even that is not good for fuzzing
as the timeout may be infinite or large, so just emit a host limit error on any
timeout for now, until we actually implement threads.
This was noticed by the fuzzer, as now we compare atomics testcases with V8.