923717 - IonMonkey: Add shell CLI for IC fuzzing

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Eric Faust [:efaust]]

At present, specifically in getprop and setprop, it's really difficult and arcane to write specific testcases that test IC robustness, because many of the optimization passes IM implements shadow IC generation at the IonBuilder level.

We should add a pref for IC fuzzing.

The current idea is something like --ion-ic-fuzzing=none,all,ICNAME, which could then be leveraged.

Comment 1

[Nicolas B. Pierron [:nbp]]

I think we should make this option a use the SetJITCompilerOption mechanism, such as this flag can be switch at runtime before any Ion compilation, and thus can be useful for Jesse's fuzzer.

the CLI can be emulated by running this function with the -e option of the shell.

Comment 2

[Nicolas B. Pierron [:nbp]]

Mark as s-s because of unknown security issues related to the patch that efaust will attach.

Comment 3

[Eric Faust [:efaust]]

Attachment: icFuzzingV1.patch

This adds a compiler flag, and the ability to set it from JS. 

Setting the flag to true affects future compilations. It does not invalidate current code. 

We should probably also add a CLI flag for IC devs, but that's still to come.

Since this reveals a couple of current issues, we have marked the bug hidden.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 815078 [details] [diff] [review]
icFuzzingV1.patch

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

::: js/src/jit/Ion.h
@@ +199,5 @@
> +    // Whether IonBuilder should prefer IC generation above optimization passes
> +    // for testing purposes.
> +    //
> +    // Default: false
> +    bool icCompilation;

nit: IC are always used during the compilation, they are just not the preferred mode of execution.
  s/icCompilation/preferICs/

::: js/src/jit/IonBuilder.cpp
@@ +6463,5 @@
>  
>      bool emitted = false;
>  
> +    if (JS_UNLIKELY(js_IonOptions.icCompilation))
> +        goto do_GetElemCache;

nit: I think it is ok to remove the JS_UNLIKELY.

::: js/src/jsapi.cpp
@@ +6149,5 @@
>          if (value == 0)
>              jit::js_IonOptions.setEagerCompilation();
>          break;
> +      case JSJITCOMPILER_ION_IC_COMPILATION:
> +        jit::js_IonOptions.icCompilation = (bool) value;

nit: use a C++ coercion: bool(value)

Comment 5

[Jan de Mooij [:jandem]]

Eric, all dependent bugs are fixed. Can we land this now? This would be great to have for fuzzing.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Add IC fuzzing mode.

Running an optimized build with JS_OPTION_forceInlineCaches=true highlights:
  tests:    53 failures over 6204 tests.
  jit-test: 317 failures over 9081 tests.

I will open follow-up bugs for fixing each of these issues, or update this patch.

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Created attachment 8601467 [details] [diff] [review]
> Add IC fuzzing mode.
> 
> Running an optimized build with JS_OPTION_forceInlineCaches=true highlights:

Oops, this is JIT_OPTION_forceInlineCaches=true , an not JS_…

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Add IC fuzzing mode.

Delta:
  - In jsop_getgname, I moved the check to forceInlineCaches under
    getStaticName, as the BaselineCompiler does the same optimization, which
    cause the NameCache to be unable to find a baseline IC / pc offset when
    bailing out.

This reduce the number of failures to:
  - tests: 0 failures
  - jit-test: 7 failures.

2 of the jit-tests issues are optimizations tests which are checking that we
are removing load/stores, which are not able to verify anything if we only
have ICs, instead of precised slots within the objects.

Comment 9

[Nicolas B. Pierron [:nbp]]

Ok, there is still one remaining issue with

jit-test/tests/baseline/bug945223.js:12:6 TypeError: x[i] is not a function
Exit code: 3
FAIL - baseline/bug945223.js

Based on the backlog of patches related to this test case, I guess this is related Bug 964574, as Eric explained in the description[1].  This differential behaviour should not prevent fuzzing for crashes.

In the mean time, I'll just patch the test case to disable this mode of execution.
I will upload a new version of the patch tomorrow, and send this to try to see if we have any crashes.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=964574#c0

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Add IC fuzzing mode.

Delta:
 - Fix SetElementIC, to properly produce type barrier before producing ICs.
 - Add {get,set}JitCompilerOption
 - Disable this mode in test cases which are dependent on compiler
   optimizations (assertFloat32, assertRecoveredOnBailout)
 - Disable this mode in a test case which rely on __noSuchMethod__, as the
   intent is to remove such feature in the future. (as commented in the test
   case)

Question:
 - Would it be better to test instead each phase if this mode is enabled,
   instead of skipping startegies with goto?

This patch works well locally (x64), without any additional fix, so I sent
it to the try server:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=9941271db2d1

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8602721 [details] [diff] [review]
Add IC fuzzing mode.

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

::: js/src/jit/JitOptions.cpp
@@ +113,5 @@
>      // Whether functions are compiled immediately.
>      SET_DEFAULT(eagerCompilation, false);
>  
> +    // Whether IonBuilder should prefer IC generation above specialized MIR.
> +    SET_DEFAULT(forceInlineCaches, true);

self-nit: s/true/false/
(I squashed too many patches)

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8602721 [details] [diff] [review]
Add IC fuzzing mode.

Ok, this patch (with the unintended typo) enables by default a mode which forces IonMonkey to compile with inline caches.

The push to try does not seems to highlight any crash which might be related to the Jit / Assembly code produced.

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> This patch works well locally (x64), without any additional fix, so I sent
> it to the try server:
> 
>   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9941271db2d1

Except for many timeouts, some of which might be related to differential behaviour caused by __noSuchMethod__.  I triggered again Linux debug M-e10s bc1, and Linux x64 debug M-e10s bc1.  I did so because I wonder if the 2 non-JS related crashes are reproducible, in which case this might be a memory corruption.

Comment 13

[Eric Faust [:efaust]]

Comment on attachment 8602721 [details] [diff] [review]
Add IC fuzzing mode.

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

Looks good to me. I remember having problems with magic arguments when I jumped over trying arguments first, but this doesn't seem to have that issue.

Comment 14

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

Comment on attachment 8602721 [details] [diff] [review]
Add IC fuzzing mode.

We will set setJitCompilerOption("ion.forceinlineCaches", 1) some 10% of the time, and will get it added into jsfunfuzz when this lands. In tests, this didn't seem to blow up anything.

Comment 15

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63bf4b7766c4

Comment 16

[Brian Hackett [Laid off!]]

The use of gotos in this patch seems gratuitous and unnecessary.  Can they be removed by using 'if' statements or helper functions or something, like we do everywhere else in IonBuilder?

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ea9608e33abe

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #16)
> The use of gotos in this patch seems gratuitous and unnecessary.  Can they
> be removed by using 'if' statements or helper functions or something, like
> we do everywhere else in IonBuilder?

Agreed. An 'if' requires indenting some code but should work fine in these cases.

(FWIW, goto is pretty rare in newer parts of the engine; there are maybe a handful in jit/ and these are all dealing with much more complicated control flow.)

Comment 19

[Brian Hackett [Laid off!]]

Attachment: remove gotos

Comment 20

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8620417 [details] [diff] [review]
remove gotos

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

Before making the patch with gotos, I was hesitating between either adding gotos, or adding the forceInlineCaches tests inside each function.
Doing so would increase the number of calls/reads to forceInlineCaches but will avoid adding indentation levels.

What do you think?

Comment 21

[Brian Hackett [Laid off!]]

(In reply to Nicolas B. Pierron [:nbp] from comment #20)
> Before making the patch with gotos, I was hesitating between either adding
> gotos, or adding the forceInlineCaches tests inside each function.
> Doing so would increase the number of calls/reads to forceInlineCaches but
> will avoid adding indentation levels.
> 
> What do you think?

I think that having the forceInlineCaches() tests where they are is better.  If the tests are moved into other functions (e.g. getPropTry*) then it will be easy to forget to add those tests when adding new optimized paths.

There isn't really anything wrong with increasing the indentation level (unless they get out of control), especially when the alternative is to obfuscate the control flow like a 'goto' does.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ceef070c92b2

Comment 23

[Christian Holler (:decoder)]

Comment on attachment 8602721 [details] [diff] [review]
Add IC fuzzing mode.

Clearing old feedback? as this has already landed. LangFuzz is using this at least since we've been testing shared stubs. It looks like the variant with forceinlineCaches set to 1 is not used in tests, so I added

> setJitCompilerOption("ion.forceinlineCaches", 1);

as a manual fragment to LangFuzz just to be sure.