-
-
Notifications
You must be signed in to change notification settings - Fork 675
fix: avoid to visit nullptr in binaryen #2943
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
Conversation
Module.getExpressionId will be used for nullptr ExpressionRef. It works because deref nullptr in binaryen's wasm is valid. Adding guard can enhance robustness
If we replace guard with assert, the compiler will crash during testing.
▌ Whoops, the AssemblyScript compiler has crashed during compile :-(
▌
▌ Here is the stack trace hinting at the problem, perhaps it's useful?
▌
▌ AssertionError: assertion failed
▌ at Z.assert (/Users/caicongcong/dev/assemblyscript/std/portable/index.js:216:11)
▌ at getExpressionId (/Users/caicongcong/dev/assemblyscript/src/module.ts:3022:3)
▌ at isConstZero (/Users/caicongcong/dev/assemblyscript/src/module.ts:3063:7)
▌ at t.operandsTostack (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:6767:14)
▌ at t.makeCallDirect (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:6910:24)
▌ at t.ensureConstructor (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:8865:18)
▌ at t.compileNewExpression (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:8758:21)
▌ at t.compileExpression (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:3432:21)
▌ at t.compileVariableStatement (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:3011:27)
▌ at t.compileStatement (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:2241:21)
▌ at t.compileStatements (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:2284:23)
▌ at t.compileFunctionBody (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:1733:20)
▌ at t.compileFunction (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:1612:17)
▌ at t.makeCallDirect (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:6841:15)
▌ at t.compileCallDirect (/Users/caicongcong/dev/assemblyscript/src/compiler.ts:6364:17)
▌
▌ If you see where the error is, feel free to send us a pull request. If not,
▌ please let us know: https://github.com/AssemblyScript/assemblyscript/issues
some background: I am trying to revert the relationship between AS and binaryen. it will create a new c++ program integrated binaryen in native as static lib and WASM runtime. Then it will load AS bootstrapped WASM to compile source code.
The benefit is we can reuse the lots of WASM infra in binaryen to improve the performance and code size.
But it is harmful to browser compatibility...
Regarding that compiler crash, maybe we should find out where a null pointer is being passed, and see if that's intended? If it is intended, this change makes more sense.
Regarding your testing with the AS compiler embedded within a C++ executable...I don't think you can upstream that, but it might be interesting to open-source if possible. It would definitely be cool to see how necessary the JS glue in binaryen.js is to AssemblyScript.
it might be interesting to open-source if possible
open source is in progress. it should be finished in this year
Regarding that compiler crash, maybe we should find out where a null pointer is being passed, and see if that's intended? If it is intended, this change makes more sense.
It generated by module.runExpression. I would fix it in caller side.
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.
LGTM. Looks like everyone else in compiler.ts/flow.ts does the same thing too!
Uh oh!
There was an error while loading. Please reload this page.
Module.getExpressionId will be used for nullptr ExpressionRef. It works because deref nullptr in binaryen's wasm is valid.
check BinaryenExprRef non-null before.