-
Notifications
You must be signed in to change notification settings - Fork 747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NFC] Move more logic about unfuzzable tests to a shared location #7175
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice simplification 👍
scripts/test/fuzzing.py
Outdated
# https://github.com/WebAssembly/binaryen/issues/3203 | ||
'simd.wast', | ||
# corner cases of escaping of names is not interesting | ||
'names.wast', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth checking whether these two can be re-enabled now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like names.wast
causes errors,
$ wasm-metadce a.wat -o a.wat --graph-file a.json -all
wasm-metadce: /usr/local/google/home/azakai/Dev/binaryen/src/support/json.h:282: char* json::Value::parse(char*): Assertion `*curr == ','' failed.
Aborted
a.wat
can be an empty wasm module. The json file was created from names in names.wast
, and I guess we are not escaping something there, or json.h
isn't reading escaped names properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That json looks valid, or at least JS engines don't error on it. So json.h
parsing might be the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably needs to be updated to handle arbitrary unicode and possibly null characters in the middle of a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simd.wast
otoh looks fine, I enabled it.
It turns out that #7165 was not enough because we had a second (!?)
list of tests to ignore, and also a condition. Move all that to the shared
location as well, continuing that PR.
Also remove
simd.wast
from the list, as that issue has been fixed.