Fix handling of waitqueue in ctor-eval - #9172
stevenfontanella wants to merge 2 commits into
Conversation
c384a95 to
7ab93bf
Compare
97dcca4 to
f3388dc
Compare
5409639 to
6b0ff7a
Compare
6b0ff7a to
3a8d486
Compare
Waitqueue instructions only make sense when GC is enabled. e.g. `struct.wait` takes a struct reference which requires GC anyway, and waitqueues live on the GC heap. This is also needed for #9172 to pass [this check](https://github.com/WebAssembly/binaryen/blob/4d8ac549e2ab9b283246ea95e79ebe139ca579ac/src/tools/wasm-ctor-eval.cpp#L598-L599). Part of #8315.
| return isMaybeShared(string) || isMaybeShared(waitqueue) || | ||
| kind == HeapTypeKind::Struct || kind == HeapTypeKind::Array; |
There was a problem hiding this comment.
This change is surprising to me, since waitqueues don't seem like transparent holders of other data the way structs and arrays are. And it seems like this change is the cause of several of the other changes. What's the motivation here?
There was a problem hiding this comment.
I mentioned this in the PR description, the idea is that isData is now true which hits a different code path in ctor-eval: link.
Alternatively we could just add an extra check for waitqueue here in addition to isData, but my thinking is that waitqueues really are GC data, and other passes that look at isData are more likely to want to treat them that way (which this case is evidence of).
From a quick look, it looks like this is the only pass that's affected in terms of real logic at the moment:
https://github.com/search?q=repo%3AWebAssembly%2Fbinaryen+%2F%5CbisData%28%29%5Cb%2F&type=code
There was a problem hiding this comment.
That code path is using !value.isData() || value.isString() to mean "requires special global-creating logic." I think it would make more sense to update that code path to explicitly include waitqueues than it would to include waitqueues in isData.
FWIW, isData is essentially a historical accident. At one point during the development of the GC proposal, there was a data type that was a supertype of structs and arrays. The type was removed, but we kept isData() because it was shorter than writing isStruct() || isArray() everywhere. It's possible that it globally make the codebase clearer if we removed it.
There was a problem hiding this comment.
My thinking is that waitqueue is 'similar' to struct and array that way; they are all stateful GC objects. Sounds good though, I don't have a strong opinion. waitqueue is definitely different in some ways.
FWIW it looks like we still use gcData as the identity of the global that we synthesize link. This looks like a problem with the current PR because we always use nullptr so there's no identity here link. Let me take a look and fix it.
Resolves a crash on
makeConstantExpressionduring ctor-eval when e.g. a waitqueue local is used. See the relevant code path in ctor-eval, which now changes sinceisData()is true for waitqueues.Passes ~12 hours of
monitor_fuzz.py -j 64, then hit a fuzz bug that reproduces frommain.Part of #8315.