-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
Fix #54 misbehaving integers #97
Conversation
tims-bsquare
commented
Nov 20, 2023
- Modify build.sh to use 64 bit floats but 32 bit ints wasm supports this but not 64 bit ints
- When pushing a number see if it's within 32 bits as well as being an int if it's not then push it as a 64 bit float
- Add tests for negative numbers and numbers greater than 32 bit
* Modify build.sh to use 64 bit floats but 32 bit ints wasm supports this but not 64 bit ints * When pushing a number see if it's within 32 bits as well as being an int if it's not then push it as a 64 bit float * Add tests for negative numbers and numbers greater than 32 bit * Fixed build issue with current emscripten emscripten-core/emscripten#20721 * Updated docs with edge cases to hopefully close off ceifa#22 ceifa#23 and ceifa#54
5abe229
to
6272005
Compare
I was just testing wasmoon out today and hit some unexpected behavior caused by 32-bit ints, so I am excited to see this issue being worked on. Unfortunately, I'm hitting a segfault on your branch (which doesn't occur on main). I'm still working on trying to find a minimal script that reproduces this, but let me know if you have any ideas or suggestions on how to get a more useful stack trace. I have bisected it down to being caused by the
|
Turns out that crash is triggered by me wrapping const origRequire = lua.global.get('require');
lua.global.set('require', path => origRequire(path)); |
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.
While testing this, I made the following update to the if/else block at the bottom to avoid leaving local changes to the lua submodule.:
if [[ "$OSTYPE" == "darwin"* ]]; then
sed -E -i '' "s/#define LUA_INT_DEFAULT\s+LUA_INT_INT/#define LUA_INT_DEFAULT\t\tLUA_INT_LONGLONG/" ./lua/luaconf.h
else
sed -E -i "s/#define LUA_INT_DEFAULT\s+LUA_INT_INT/#define LUA_INT_DEFAULT\t\tLUA_INT_LONGLONG/" ./lua/luaconf.h
fi
That's a funky error, I'll take a look |
I've tried to write a test to reproduce it but it passes, could you try and tweak it @Sapu94 until it fails please? it('wrap require function', async () => {
const factory = getFactory()
await factory.mountFile('test.lua', 'return 10')
const engine = await factory.createEngine()
const origRequire = engine.global.get('require');
engine.global.set('require', path => origRequire(path));
await engine.doString(`require('test')`)
}) |
It could also be related to this emscripten-core/emscripten#13131 |
Just getting back to this after being away for the past week. It seems like this can only be reproduced by getting the Lua VM / wasm stack into some specific state. Sorry in advance for the huge can of worms this seems to be 😅. Here's as far as I've gotten in coming up with a minimal set of reproduction steps (I'm doing all this in WSL for what it's worth):
Here's the latest error:
Let me know if you're able to reproduce the issue with these steps or not. Here are a few of the things I've tried in my test repo that make me the crash go away:
I don't actually think any of the above are super meaningful, other than to point to the issue being caused by things getting into a specific (corrupted) state as opposed to just one faulty function / line of code somewhere. |
That's a great repro repo, thank you for putting that together! :) I think what you encountered was an actual proper segfault where the library was trying to reference a free'd value and I suspect sometimes the memory may not yet have been corrupted depending on those small changes you listed. I've pushed another change to this branch, unrelated to the misbehaving integers hopefully fixing this for you (it does for me locally). Something to be aware of is that there's a potential memory leak with doFile/callByteCode that's necessary to avoid segfaults, which was and is done by moving the result value from the execution thread to the global. The code was returning a reference to the value in the thread which had been cleaned up rather than the copy in the global state. |
2b77a59
to
6bcda76
Compare
That fix works great, thanks! Good to know about the memory leak. |
else | ||
extension="$extension -O3 --closure 1" | ||
# ASSERTIONS=1 required with optimisations and strict mode. https://github.com/emscripten-core/emscripten/issues/20721 |
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.
Actually this is a bug on emscripten? Assertions are probably unnecessary and just increase the code size and slow down a little bit. Should have a TODO to remove it later, wdyt?
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.
As far as I can tell it's a bug in emscripten, the one linked. I'd be quite happy if you found time to take a second look though just in case. I'll add that TODO in
Great job @tims-bsquare! Can I merge it or are you still working on this PR? |
No problem, should be ready to merge @ceifa :) Let me add that TODO first though |