alexcrichton opened PR #6736 from alexcrichton:fix-fuzz-oom
to bytecodealliance:main
:
This commit is a fix for a fuzz failure that came in the other day where a module took more than 2GB of memory to compile, triggering the fuzzer to fail. Investigating locally it looks like the problem lies in the basic-block padding added recently. Turning on that option increases the memory usage of Cranelift from ~70M to ~2.5G. The setting in the test was to add
1<<14
bytes of padding between all basic blocks, and with a lot of basic blocks that can add up quite quickly.The solution implemented here is to implement a per-function limit of the amount of padding that can be injected. I've set it to 1MB for now which is hopefully enough to continue to stress the various bits of the
MachBuffer
while not blowing limits accidentally while fuzzing.After this commit the fuzz test case in question jumps from 70M to 470M with basic block padding enabled, which while still large is a bit more modest and sticks within the limits of the fuzzer.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6736.
alexcrichton requested abrown for a review on PR #6736.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I think this would limit it to a single conditional branch where the branch destination doesn't fit and thus a veneer needs to be inserted on AArch64. Maybe bump it to 1 << 21?
alexcrichton requested pchickey for a review on PR #6736.
alexcrichton updated PR #6736.
alexcrichton requested wasmtime-core-reviewers for a review on PR #6736.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
That's a good point yeah and was something I was worried about as well. I'm a bit worried though that bumping this to 21 (predictably) doubles the memory usage.
I've instead opted for a different strategy now:
- Each function gets at most 64M of padding
- Padding is only enabled for fuzz test cases with <10 functions
That should help continue to explore interesting cases while avoiding the large memory increases seen prior I think
bjorn3 submitted PR review.
bjorn3 created PR review comment:
*doesn't
alexcrichton updated PR #6736.
abrown submitted PR review:
I'm fine with this but wonder if @cfallin might have an opinion: limiting the Cranelift functions to a padded 64MB makes it possible that some of the larger islands never get fuzzed? IIRC, don't we have 2GB islands on x86?
alexcrichton updated PR #6736.
alexcrichton has enabled auto merge for PR #6736.
alexcrichton merged PR #6736.
Last updated: Dec 23 2024 at 13:07 UTC