Stream: git-wasmtime

Topic: wasmtime / PR #3780 Add the instance allocation strategy ...

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 22:37):

peterhuene opened PR #3780 from fuzz-pooling-allocator to main:

This PR adds support for generating configs with arbitrary instance
allocation strategies.

With this, the pooling allocator will be fuzzed as part of the existing fuzz
targets.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 22:37):

peterhuene requested alexcrichton for a review on PR #3780.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 22:48):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 22:53):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 22:53):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 22:53):

cfallin created PR review comment:

MAX_TABLES here?

In general it seems maybe there should be a MAX_* constant for each kind of entity (functions, tables, memories, globals) and then imported_[entity] and [entity] both lie in that range -- or otherwise, maybe a comment why some limits have specific bounds and others are generic MAXIMUM?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 23:05):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 23:05):

peterhuene created PR review comment:

I'll make this more consistent.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 23:16):

peterhuene updated PR #3780 from fuzz-pooling-allocator to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 23:16):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 23:16):

peterhuene created PR review comment:

I've added constants for each type to make it clearer.

Note that for limits like functions, it denotes the _defined_ function limit and not the total function limit, thus imported_functions isn't bound in the range (0..=MAX_FUNCTIONS), for example.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 08 2022 at 23:17):

peterhuene updated PR #3780 from fuzz-pooling-allocator to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 04:16):

peterhuene updated PR #3780 from fuzz-pooling-allocator to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 04:17):

peterhuene updated PR #3780 from fuzz-pooling-allocator to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 04:18):

peterhuene requested fitzgen for a review on PR #3780.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 04:18):

peterhuene requested cfallin for a review on PR #3780.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 04:26):

peterhuene edited PR #3780 from fuzz-pooling-allocator to main:

This PR adds support for generating configs with arbitrary instance
allocation strategies.

With this, the pooling allocator will be fuzzed as part of the existing fuzz
targets and also with the new pooling fuzz target.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 15:38):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 15:38):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 15:38):

alexcrichton created PR review comment:

Was this necessary to split out the memory configuration like this? I figured we'd just ignore irrelevant settings with the pooling allocator but I wouldn't be surprised if sanity checks prevented some of this

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 15:38):

alexcrichton created PR review comment:

I think that we should be able to relatively easily control the limits on the wasm-smith-generated-module, could one of the methods on Config automatically apply the configuration of the pooling allocator to the wasm-smith module limits perhaps?

(that'd avoid the need to duplicate this error handling below as well)

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 15:38):

alexcrichton created PR review comment:

I would naively expect that fuzzing here with the on-demand allocator would be fine, so could this be removed to naturally fuzz with both given the config generated above?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 17:50):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 17:50):

peterhuene created PR review comment:

The pooling allocator does check if memory_pages in the configured module limit exceeds the static memory bound and will fail the creation of the engine when that is the case.

I can make this less verbose, at least, by only setting static_memory_maximum_size in either branch based on the strategy being on-demand.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 19:24):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 19:24):

alexcrichton created PR review comment:

Would it be possible to massage the configuration settings to be correct for cases like that? Such as tweaking the instantiation limit bounds or these arguments to be the minimum of each one or something like that.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 22:57):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 22:57):

peterhuene created PR review comment:

So we can't completely avoid this check as currently there's no way to constrain table type minimums in wasm-smith like we can with memory types. The pooling instance allocator may place limits on the maximum elements in a table and will error if a table type has a minimum that exceeds the allocator's limit.

I have some changes that makes hitting this error much less likely where we constraint the generated module based on the arbitrary pooling limits (if arbitrarily configured to use pooling), but we'll need to update wasm-smith to fully remove these two checks, I believe.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 23:04):

peterhuene edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 23:05):

peterhuene edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 23:31):

peterhuene created PR review comment:

This will be removed and we'll fuzz on any arbitrary strategy (the target has subsequently been renamed to instantiate-many).

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 23:31):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 09 2022 at 23:48):

peterhuene edited PR #3780 from fuzz-pooling-allocator to main:

This PR adds support for generating configs with arbitrary instance
allocation strategies.

With this, the pooling allocator will be fuzzed as part of the existing fuzz
targets and also with the new instantiate-many fuzz target.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 04:53):

peterhuene updated PR #3780 from fuzz-pooling-allocator to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 04:54):

peterhuene requested alexcrichton for a review on PR #3780.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 06:57):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 07:38):

peterhuene updated PR #3780 from fuzz-pooling-allocator to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 15:15):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 15:15):

alexcrichton created PR review comment:

I'm a little worried about this change in that above there's a .min(0x1000..) to I think make the cap still the same but randomly choosing a 64-bit number seems like it'll almost always use the 0x1000.. limit instead of something smaller? Overall I'm wondering if this 64-bit space heavily weights the fuzzing to always choose the same result and whether that has an affect.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 15:15):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 18:25):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 18:25):

peterhuene created PR review comment:

That's an excellent point. Let me fix that.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 19:02):

peterhuene updated PR #3780 from fuzz-pooling-allocator to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 19:02):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 19:02):

peterhuene created PR review comment:

Fixed with latest push.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 19:10):

peterhuene updated PR #3780 from fuzz-pooling-allocator to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 10 2022 at 19:55):

peterhuene merged PR #3780.

Last updated: Nov 22 2024 at 16:03 UTC