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

Fuzzer: Adjust feature fuzzing frequency #6305

Merged
merged 5 commits into from
Feb 22, 2024
Merged

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 13, 2024

We used to fuzz MVP 1/3, all 1/3, and a mixture 1/3, but that gives far too much
priority to the MVP which is increasingly less important. It is also a good idea to
give "all" more priority as that enables more initial content to run (the fuzzer will
discard initial content if it doesn't validate with the features chosen in the current
iteration).

Comment on lines 130 to 134
# Note that POSSIBLE_FEATURE_OPTS contains --disable-X for each feature, so
# we enable all features when we add nothing from it, etc. (assert on that
# form here to guard against future refactorings making a mess here).
assert '--disable' in POSSIBLE_FEATURE_OPTS[0]
assert '--enable' not in POSSIBLE_FEATURE_OPTS[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about renaming it to POSSIBLE_DISABLE_FEATURE_OPTS or something? Code like

FEATURES_OPTS += POSSIBLE_FEATURE_OPTS

looks like we are enabling all the possible options out there when we are actually doing the very opposite.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, thanks, renamed and refactored.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like @aheejin's renaming idea, but otherwise LGTM

@kripken
Copy link
Member Author

kripken commented Feb 14, 2024

Actually I may need to change the probabilities here, since I forgot that enabling all features disables parts of the fuzzer... for example enabling simd disables running d8 (since v128 on the boundary traps - we can't compare those values). I'll think about some solution to that first maybe.

@kripken
Copy link
Member Author

kripken commented Feb 22, 2024

After recent PRs landing, enabling all features no longer limits V8 fuzzing. It does still limit some things like wasm2js fuzzing, but that is low priority, and so this can land.

@kripken kripken merged commit 2ceff4d into WebAssembly:main Feb 22, 2024
14 checks passed
@kripken kripken deleted the fuzz.feat branch February 22, 2024 19:34
ashleynh pushed a commit that referenced this pull request Feb 26, 2024
We used to fuzz MVP 1/3, all 1/3, and a mixture 1/3, but that gives far too much
priority to the MVP which is increasingly less important. It is also a good idea to
give "all" more priority as that enables more initial content to run (the fuzzer will
discard initial content if it doesn't validate with the features chosen in the current
iteration).

Also (NFC) rename POSSIBLE_FEATURE_OPTS to make the code easier to follow.
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
We used to fuzz MVP 1/3, all 1/3, and a mixture 1/3, but that gives far too much
priority to the MVP which is increasingly less important. It is also a good idea to
give "all" more priority as that enables more initial content to run (the fuzzer will
discard initial content if it doesn't validate with the features chosen in the current
iteration).

Also (NFC) rename POSSIBLE_FEATURE_OPTS to make the code easier to follow.
@gkdn gkdn mentioned this pull request Aug 31, 2024
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 this pull request may close these issues.

3 participants