-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Check for too many arguments to convertJsFunctionToWasm signature #16653
Conversation
To be clear, I checked that 122 is the max args on emscripten v2.0.27, so probably should double check that it hasn't changed on tot. |
Which project does Which line actually throws here? (I'd guess the VM, perhaps? if so the emscripten version should not matter) |
Yeah it's the VM. Can be reproduced by dumping the following into the browser console on Reproduction
In Chrome:
In Firefox:
|
Yeah v2.0.27 is the Emscripten version. Pyodide is stuck on v2.0.27 because of a problem with dynamic linking in Firefox versions 96, 97, and 98 (but not 95 or 99beta) #16538 |
I see, thanks! How about putting this in a try-catch around the VM command that fails? That would make it clear what the source of the issue is. We could print the VM error and also show this new warning in the case that the number of parameters is this high (maybe something like "the VM errored on trying to wrap a function. Perhaps due to too many arguments?"). That way if browsers increase the limit, things would just start to work automatically. Also, please ifdef this behind |
Actually I think this not a platform limitation but rather a bug in |
Yeah so the type section generated is invalid. If I generate a valid wasm file with a 123-argument function and run
If I run
If I try it on the 123 argument output I get:
Note that it thinks there are 96 = 0x60 types. 0x60 is the prefix of a function type. |
Well it looks like if I put an extra |
Aha the problem is that you aren't correctly leb128-encoding the length of the type section. |
Closed in favor of #16658. It turns out the VM generates a perfectly fine error message when we give it too many arguments |
Research shows that giving
convertJsFunctionToWasm
a signature of length > 122 triggers a compiler error likethis puts in a more intelligible error message in this case.