-
Notifications
You must be signed in to change notification settings - Fork 786
Figure out wasm2asm testing #7
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
Comments
If it helps, I just support it. It means that in the end you have one testsuite that is shared by all instead of forking that part. |
Yeah, probably we should. What's annoying though is that we do support it in the shell for spec tests, but supporting it here would require support in other places. I suppose I should refactor the code into somewhere it can be used more generally. Alternatively, I am considering a python script that splits up a wast into separate files, and emits a js file with the asserts. |
So if it helps, I actually do exactly that. Though the input accepts multiple modules and the assert syntax, I end of generating one module per file and a separate file for all the asserts. |
Oh, at what level do you do it? Maybe I can reuse that code. I looked here but I guess that's the wrong place? What can be done for the expected-output files though? It's not clear how to split those since which outputs go to which module in the |
For sexpr-wasm, I have a Since some assertions require things that JavaScript can't handle (int64, checking nan bits), I write all assertions as exported functions in the module, each returning 1 if they succeed and 0 if they fail. assert_trap instead is wrapped in a try/catch block. assert_return_nan is handled by running |
Interesting, thanks for the info. A question about int64 - is the issue that v8-native doesn't have int64 yet, or that you can't represent an int64 outside of the wasm module in normal JS (or something else)? |
So it' s done inside my code. Basically I raise everything to a WasmModule then each WasmModule gets called via Generate and that creates a new file for each. Assertions for me return 0 on success or an integer for failure. This allows me to get it to return the line number of the assertion failure (I suppose it will never be 0 :)). I do not support yet assert_traps, ignoring them and assert_nan is handled internally by dong something similar to what binji is doing. I then have somehing similar to what binji has: a big function that LLVM generates for me that calls each of these asserts and bails if one returns a non-0 value (returning it). Then my driver will catch that return and say: hey you have an issue at this line |
I see, thanks. I think for the purposes of wasm2asm testing, I'll have a python script split out the wast into modules + assertions. Then I'll translate the module using wasm2asm, and let python convert the assertions to JS. Then I'll run them and concat the output before comparing to the expected output (which should handle not knowing which parts of the expected output is for which module in a file with more than one). |
@kripken I mean JavaScript can't handle int64, so if you call an exported wasm function that returns int64, v8-native currently just truncates the value to an int32. So you can't actually do the assertion checks in JavaScript, even though that would be nice. Similarly, AIUI you can return NaN to JavaScript, but the value will be canonicalized, so you can't do the check in JavaScript. |
Right, yeah. I was thinking about returning a JS array of It seems we need to address this for polyfill purposes (which is the main goal of wasm2asm), that is, once we have a polyfill, and someone converts a wasm into some JS, and they get a return value that can't be expressed in JS, we need to do something. iow this seems to be a serious problem not just for testing, but for polyfills in real life, it now appears to me. |
wasm2asm is no longer in development, closing. |
The spec repo has tests that we should get wasm2asm working on. But it has multiple modules per file and a special assert syntax. Should we support that, or create something simpler?
The text was updated successfully, but these errors were encountered: