1461689 - Create a text file containing all the shell flags combinations we'd like to see tested by fuzzers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

This is a request from :decoder. We should have a list of shell flag combinations we'd like to see used by fuzzers which are trying to find bugs in our code base. 

This is nice because:

- when we add a flag, we don't have to use a manual process of letting our fuzzing people know, if they use a tool that automatically parses this file. This process we currently use is cumbersome, and sometimes we forget telling them, and it silently breaks fuzzing.
- We could even probably remove flags and not morph them into no-ops, as long as we'd remove the flag from the flag file too.
- This is also nice for fuzzer people external to Mozilla, who would want to fuzz our code base but wouldn't know what the interesting flags are.
- We can have time-based fuzzing experiments, when we can request a certain flag to be used more, for some amount of time, by adding new combinations using it. For what it's worth, Google has some file like this, if I remember correctly, in which they map probabilities to given flags, and the flags get randomly used in fuzzing with the set probabilities.

I wondered if we could just have a JS shell CLI switch that would display all these flag combinations, but it would need to be manually maintained in the same way as a simple text file would be, so I'm not sure it's better overall.

Now, I obviously don't know all the flags we'd like to test, so we'll need to crowdsource this process. I've added flags that seemed interesting for wasm, at least.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fuzzflags.patch

Please read comment 0 first.

Jan, Jon: what do you think about this? which other flags would you like to see in this file (as tech leads of JIT and GC) ?

Gary, Christian: would such a file be simple and useful enough for you to parse and generate the interesting flag combinations?

Comment 2

[Jan de Mooij [:jandem]]

Which flags do the fuzzers use right now? Maybe we can start with that list?

Benjamin, why is --test-wasm-await-tier2 (for instance) not on that list?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Which flags do the fuzzers use right now? Maybe we can start with that list?

Good question. I'm hoping Christian and Gary can tell us.
 
> Benjamin, why is --test-wasm-await-tier2 (for instance) not on that list?

This particular flag didn't find many fuzzbugs in the past, if I'm correct, and my understanding was that it's been useful when developing tiering, and that its utility has lowered a lot since then. I could be wrong here, of course.

Maybe we'd want to keep it. Maybe we could have a weight associated to each flag combination (as the Google system seems to have), so that we can indicate which combinations are more important?

Comment 4

[Christian Holler (:decoder)]

On 64-bit:

# Mandatory options
binaryOptions: &binaryOptions
  - --fuzzing-safe
  - --cpu-count=2

# Options that can be used randomly in any builds
randomOptions: &randomOptions
  - --ion-eager
  - --baseline-eager
  - --ion-offthread-compile=off
  - --ion-extra-checks
  - --ion-aa=flow-sensitive
  - --test-wasm-await-tier2
  - --spectre-mitigations=on
  - --spectre-mitigations=off
  - --nursery-strings=on
  - --execute=gcstress=1
  - --wasm-gc
  

# This extends the options above with some options
# that are only available in debug builds
randomDebugOptions: &randomDebugOptions
  - *randomOptions
  - --ion-check-range-analysis


On ARM simulator we add:

  - --arm-sim-icache-checks
  - --arm-asm-nop-fill=1
  - --arm-hwcap=vfp


In some configurations, I also add --no-threads. Note that LangFuzz currently has no notion of grouping flags together (as in, these flags should be used, but only together). So I would simply parse the file and take all of the flags for this list.

However, note that some flags are only available in certain build types, we need to figure out a smart way to account for that. Maybe I can also write a script that figures out if the shell accepts a certain flag, as part of the extraction logic.

Comment 5

[Jan de Mooij [:jandem]]

Benjamin, was your idea that the fuzzer would pick a random *line* from this file and use that? Or do we have a list of options that the fuzzer can pick multiple from? I'd prefer the latter because then we can add --ion-eager and --ion-offthread-compile=off on separate lines, and the fuzzers will automatically test all 4 combinations of these 2 flags (they're all useful).

Assigning weights makes sense especially for options like --no-baseline and --no-ion, because these basically turn off huge parts of the code so we don't want to use these flags too often. Maybe we just shouldn't fuzz with these flags at all because we run jit-tests with them in automation and fuzzing is unlikely to find bugs that only show up with these flags.

As for certain flags being only available in some builds, I think it makes sense either for the fuzzer to detect this or for us to make these flags no-ops in builds where they're not supported (we could print a warning to stderr).

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the list!

(In reply to Jan de Mooij [:jandem] from comment #5)
> Benjamin, was your idea that the fuzzer would pick a random *line* from this
> file and use that? Or do we have a list of options that the fuzzer can pick
> multiple from? I'd prefer the latter because then we can add --ion-eager and
> --ion-offthread-compile=off on separate lines, and the fuzzers will
> automatically test all 4 combinations of these 2 flags (they're all useful).

My idea was indeed the former. In particular I mentioned --ion-eager as a flag that always wants to have --ion-compile-offthread=off, because to me it seems that it makes test cases much more reliable; but you seem to imply that the opposite is true, and that testing --ion-eager on its own is useful too. Decoder told me (irc) that LangFuzz can only pick a random flag, not a combination. So it seems the latter is more appropriate in both cases. In this case, we can probably just have one flag per line.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> but you seem to imply
> that the opposite is true, and that testing --ion-eager on its own is useful
> too.

Yeah, imagine an off-thread-compilation-specific bug/race :) It's true that --ion-eager *without* off-thread compilation is more deterministic and is very useful.

> In this case, we can probably just have one flag per line.

Sounds good!

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Christian Holler (:decoder) from comment #4)
>   - --execute=gcstress=1

What does this do?

Having this file seems like a good idea so that everyone can see which options are being tested.

Comment 9

[Christian Holler (:decoder)]

(In reply to Jon Coppeard (:jonco) from comment #8)
> (In reply to Christian Holler (:decoder) from comment #4)
> >   - --execute=gcstress=1
> 
> What does this do?

It enables a special gcstress mode in the fuzzing driver that calls gczeal much more. But that also makes everything a lot slower, so I'm not always passing this flag.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8975819 [details] [diff] [review]
fuzzflags.patch

Review of attachment 8975819 [details] [diff] [review]:
-----------------------------------------------------------------

Starting with decoder's list makes sense to me (except --ion-aa=flow-sensitive because that has been removed).

Maybe useful to add:

--no-sse3
--no-sse4
--ion-warmup-threshold=100
--no-native-regexp

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fuzzflags.patch

Updated patch with one flag per line, as discussed before. I checked and all the flags are valid on all the build variants, including architecture specific ones (which might be no-ops in some configurations).

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8976117 [details] [diff] [review]
fuzzflags.patch

Review of attachment 8976117 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks for doing this.

::: js/src/shell/fuzz-flags.txt
@@ +1,3 @@
> +# This lists all the possible flags we'd like to see tested out in the shell by
> +# fuzzers. A non-empty line not starting with should be considered a valid one.
> +# Note the following flag is recommeded in ALL the cases: --fuzzing-safe

Nit: s/with/with #/ and recommended, typo

Comment 13

[Jan de Mooij [:jandem]]

Alex, you run OSS-Fuzz on SpiderMonkey, right? needinfo to make sure you're aware of this change.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bba65880a66
Add a fuzz-flags.txt file containing interesting flags for fuzzing; r=jandem

Comment 15

[Alex Gaynor [:Alex_Gaynor]]

Thanks; filed https://github.com/google/oss-fuzz/issues/1425 on the OSS-Fuzz side to see how we can leverage this.

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5bba65880a66

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Just saw this bugmail from being away on PTO.

The ones that funfuzz uses that aren't present:

--enable-avx
--no-avx
--cache-ir-stubs=on/off
--ion-pgo=on/off
--ion-sincos=on/off
--ion-instruction-reordering=on
--ion-shared-stubs=on
--ion-regalloc=testbed
--non-writable-jitcode
--ion-loop-unrolling=on/off
--ion-scalar-replacement=on/off
--ion-range-analysis=on/off
--ion-edgecase-analysis=on/off
--ion-limit-script-size=on/off
--ion-osr=on/off
--ion-inlining=on/off
--ion-gvn=on/off
--ion-licm=on/off

--no-unboxed-objects
--dump-bytecode

calling --no-baseline when --baseline-eager is not present
calling --no-ion when --ion is not present

GC-related that were separated out from the above:
--no-incremental-gc
--no-ggc
--no-cgc
--gc-zeal=<some variable numbers>

Let me know which ones are also worth adding to fuzz-flags.txt so I'll file a new bug.

===

Reference files/links for the funfuzz harness (jsfunfuzz et. al):

arch-specific: (e.g. ARM stuff)
https://github.com/MozillaSecurity/funfuzz/blob/b4a7f75/src/funfuzz/js/shell_flags.py#L54
ion:
https://github.com/MozillaSecurity/funfuzz/blob/b4a7f75/src/funfuzz/js/shell_flags.py#L91
wasm:
https://github.com/MozillaSecurity/funfuzz/blob/b4a7f75/src/funfuzz/js/shell_flags.py#L172
main chunk:
https://github.com/MozillaSecurity/funfuzz/blob/b4a7f75/src/funfuzz/js/shell_flags.py#L203

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #17)
> --enable-avx
> --no-avx

These would be good to have to make sure we don't regress the AVX codegen (it's currently disabled).

> --cache-ir-stubs=on/off
> --ion-pgo=on/off
> --ion-sincos=on/off
> --ion-instruction-reordering=on
> --ion-shared-stubs=on
> --ion-regalloc=testbed
> --non-writable-jitcode
> --ion-loop-unrolling=on/off
> --ion-scalar-replacement=on/off
> --ion-range-analysis=on/off
> --ion-edgecase-analysis=on/off
> --ion-limit-script-size=on/off
> --ion-osr=on/off
> --ion-inlining=on/off
> --ion-gvn=on/off
> --ion-licm=on/off
> --no-unboxed-objects

The --non-writable-jitcode flag is a no-op we kept for fuzzing so we shouldn't add it. It might make sense to add both the on and off versions of the rest of these. Most of them are enabled by default, but at the same time it shouldn't hurt to enable explicitly.

> GC-related that were separated out from the above:
> --no-incremental-gc
> --no-ggc
> --no-cgc
> --gc-zeal=<some variable numbers>

Probably fine too. The --gc-zeal one is more complicated because it wants a random number but we also have gczeal() so I don't know if it's necessary.