Cap the maximum basic block padding in Cranelift #6736
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
bjorn3
reviewed
Jul 17, 2023
| if !bb_padding.is_empty() { | ||
| buffer.put_data(&bb_padding); | ||
| buffer.align_to(I::LabelUse::ALIGN); | ||
| total_bb_padding += bb_padding.len(); | ||
| if total_bb_padding > (1 << 20) { |
There was a problem hiding this 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?
There was a problem hiding this 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
github-actions
bot
added
cranelift
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "fuzzing"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
bjorn3
reviewed
Jul 17, 2023
| // regardless of how many basic blocks there are. | ||
| // * Here it's limited to enable this setting only when there's at most | ||
| // 10 functions to ensure that the overhead for all functions is <1G | ||
| // ish or otherwise dosn't run away. |
|
Ah, yes, we actually do have 26-bit ranges that are relevant on AArch64: direct branches have a range of +/- 128MiB (2^26, times 4 because of PC alignment, signed). A function size up to 128MiB plus epsilon (144MiB, to pick a round number?) would I think give sufficient coverage. |
|
(And x86-64 does have 2GiB ranges for all branches and RIP-relative labels, but we actually don't have façades to extend those; we just don't support a single function body that large!) |
|
Sounds reasonable! I've bumped the per-function limit to 150M and reduced the maximum number of functions to 5 to keep the overhead under 1G here |
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
Aug 7, 2023
This commit removes the option to generate padding between basic blocks when fuzzing Wasmtime. Over the weekend lots of OOMs were discovered related to this option and its most recent update in bytecodealliance#6736. The new OOMs appear to be related to: * If multiple modules are generated then the configured limits in bytecodealliance#6736 aren't relevant because they only cap one module. * Each imported function generates a new trampoline which has its own set of padding which wasn't previously accounted for. * Spec tests have a lot of functions and the limits didn't account for this. While each of these is probably individually fixable I think it's probably not worth the whack-a-mole any more. The `cranelift-fuzzgen` target should cover the relevant cases for padding without the need for Wasmtime's fuzzing to cover it as well.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 9, 2023
This commit removes the option to generate padding between basic blocks when fuzzing Wasmtime. Over the weekend lots of OOMs were discovered related to this option and its most recent update in #6736. The new OOMs appear to be related to: * If multiple modules are generated then the configured limits in #6736 aren't relevant because they only cap one module. * Each imported function generates a new trampoline which has its own set of padding which wasn't previously accounted for. * Spec tests have a lot of functions and the limits didn't account for this. While each of these is probably individually fixable I think it's probably not worth the whack-a-mole any more. The `cranelift-fuzzgen` target should cover the relevant cases for padding without the need for Wasmtime's fuzzing to cover it as well.
eduardomourar
pushed a commit
to eduardomourar/wasmtime
that referenced
this pull request
Aug 18, 2023
This commit removes the option to generate padding between basic blocks when fuzzing Wasmtime. Over the weekend lots of OOMs were discovered related to this option and its most recent update in bytecodealliance#6736. The new OOMs appear to be related to: * If multiple modules are generated then the configured limits in bytecodealliance#6736 aren't relevant because they only cap one module. * Each imported function generates a new trampoline which has its own set of padding which wasn't previously accounted for. * Spec tests have a lot of functions and the limits didn't account for this. While each of these is probably individually fixable I think it's probably not worth the whack-a-mole any more. The `cranelift-fuzzgen` target should cover the relevant cases for padding without the need for Wasmtime's fuzzing to cover it as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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<<14bytes 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
MachBufferwhile 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.