Fuzzer: Generate TryTables #6987
Conversation
src/tools/fuzzing/fuzzing.cpp
Outdated
| if (funcContext->breakableStack.empty()) { | ||
| // Nothing to break to. | ||
| // TODO: Perhaps generate a block wrapping us? | ||
| return makeTrivial(type); | ||
| } |
There was a problem hiding this comment.
We can actually make a trivial try_table that doesn't need branches (not sure how much it would help fuzzing though)
try_table (result type)
...
end_try_tableThere was a problem hiding this comment.
Oh, good point. I think that is better, as it could catch something. It also makes the code simpler. I updated to do that now.
src/tools/fuzzing/fuzzing.cpp
Outdated
|
|
||
| // If we found nothing, give up. | ||
| if (catchTags.empty()) { | ||
| return makeTrivial(type); |
There was a problem hiding this comment.
Here too we can make a trivial try_table
src/tools/fuzzing/fuzzing.cpp
Outdated
| std::vector<Name> catchTags; | ||
| std::vector<Name> catchDests; | ||
| std::vector<bool> catchRefs; | ||
| auto numTags = upTo(MAX_TRY_CATCHES); |
There was a problem hiding this comment.
How about numCatches, given that some catches don't have tags?
src/tools/fuzzing/fuzzing.cpp
Outdated
| tagName = tag->name; | ||
| tagType = tag->sig.params; | ||
| } else { | ||
| // Look for a catch_all at the end, some of the time (but all of the time |
There was a problem hiding this comment.
What do you mean "Look for"? Aren't we generating a catch_all here, not looking for (an already existing) one?
There was a problem hiding this comment.
Yeah, that was written poorly, thanks. Fixed.
src/tools/fuzzing/fuzzing.cpp
Outdated
| auto valueType = getTargetType(target); | ||
| if (valueType == tagType || valueType == tagTypeWithExn) { | ||
| catchTags.push_back(tagName); | ||
| catchDests.push_back(name); |
There was a problem hiding this comment.
Maybe destName or dest or something to be distinguished from tagName?
src/tools/fuzzing/fuzzing.cpp
Outdated
| for (auto t : tagType) { | ||
| vec.push_back(t); | ||
| } | ||
| vec.push_back(Type(HeapType::exn, Nullable)); // TODO: NonNullable? |
There was a problem hiding this comment.
Isn't exnref ref null exn? Why would we need a nonnullable exnref?
There was a problem hiding this comment.
I wasn't sure what the spec allows. Don't we always send a non-nullable exnref, if an exception is thrown? If so then the target block could be non-nullable too, like this?
(block $block (ref exn) ;; non-nullable
(try_table (catch_all $block)
..
)
)Or does the spec say to use a nullable value for some reason?
There was a problem hiding this comment.
I'm not sure. Given that exnref values generated from try_table will never be null, I think allowing the non-nullable type makes sense though. I asked the question in the EH repo: WebAssembly/exception-handling#336
There was a problem hiding this comment.
It looks the non-nullable version is allowed:
WebAssembly/exception-handling#336 (comment)
There was a problem hiding this comment.
Oh cool, thanks. I added support for that now.
When EH+GC are enabled then wasm has non-nullable types, and the sent exnref should be non-nullable. In BinaryenIR we use the non- nullable type all the time, which we also do for function references and other things; we lower it if GC is not enabled to a nullable type for the binary format (see `WasmBinaryWriter::writeType`, to which comments were added in this PR). That is, this PR makes us handle exnref the same as those other types. A new test verifies that behavior. Various existing tests are updated because ReFinalize will now use the more refined type, so this is an optimization. It is also a bugfix as in #6987 we started to emit the refined form in the fuzzer, and this PR makes us handle it properly in validation and ReFinalization.
Also make Try/TryTables with type none, and not just concrete types as before.