Skip to content
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

Raising the limit on the number of locals in input files #6968

Closed
vouillon opened this issue Sep 25, 2024 · 3 comments · Fixed by #6973
Closed

Raising the limit on the number of locals in input files #6968

vouillon opened this issue Sep 25, 2024 · 3 comments · Fixed by #6973

Comments

@vouillon
Copy link
Contributor

The wasm_of_ocaml compiler is generating code using a fresh local variable for each intermediate value. We rely on wasm-opt to optimize this. Since #6677, the binary parser will reject functions with more then 50,000 locals. We are currently reaching this limit on some large programs. Maybe it would make sense to raise this limit since wasm-opt is expected to take as input unoptimized code. Or maybe there should be an option to lift the limit?

@kripken
Copy link
Member

kripken commented Sep 25, 2024

Good point, I agree we should change this. I think the check should error on 2^32 which is the absolute limit in the spec, and which has a spec test, while for the Web limit of 50,000 we should just warn like we do here:

if (func->getParams().size() > WebLimitations::MaxFunctionParams) {
std::cerr << "Some VMs may not accept this binary because it has a large "
<< "number of parameters in function " << func->name << ".\n";
}

And that warning should be during writing, not reading.

Would you like to open a PR for that?

@vouillon
Copy link
Contributor Author

Ok, I'll do that.

@tlively mentioned an issue with allocation failures on CI as a justification for this limit. I'm not sure what he meant.

@kripken
Copy link
Member

kripken commented Sep 25, 2024

Oh, we do allocate a vector for locals (whose elements are pointers, so 8 bytes in 64-bit), so trying to allocate one of length 2^32 might indeed hit a CI limit. And it does look like the test has 2^32 - 1 locals of one type and then one of another, so we allocate almost 2^32 before the error. That could be fixed by first reading the counts and types, validating, and only then adding to currFunction->vars if valid.

vouillon added a commit to vouillon/binaryen that referenced this issue Sep 26, 2024
This raises the number of locals accepted by the binary parser to the
absolute limit in the spec. A warning is now printed when writing a
binary file if the Web limit of 50,000 locals is exceeded. This fixes WebAssembly#6968.
vouillon added a commit to vouillon/binaryen that referenced this issue Sep 26, 2024
This raises the number of locals accepted by the binary parser to the
absolute limit in the spec. A warning is now printed when writing a
binary file if the Web limit of 50,000 locals is exceeded. This fixes WebAssembly#6968.
kripken pushed a commit that referenced this issue Sep 30, 2024
This raises the number of locals accepted by the binary parser to the
absolute limit in the spec. A warning is now printed when writing a
binary file if the Web limit of 50,000 locals is exceeded.

Fixes #6968.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants