IonMonkey: Add shell CLI for IC fuzzing

RESOLVED FIXED in Firefox 41

Status

()

Core
JavaScript Engine: JIT
--
enhancement
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

({sec-want})

unspecified
mozilla41
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main41-])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
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.

Updated

4 years ago
Severity: normal → enhancement
Keywords: sec-want
OS: Windows 7 → All
Hardware: x86_64 → All

Updated

4 years ago
Component: JavaScript Engine → JavaScript Engine: JIT
Mark as s-s because of unknown security issues related to the patch that efaust will attach.
Group: core-security
(Assignee)

Comment 3

4 years ago
Created attachment 815078 [details] [diff] [review]
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.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #815078 - Flags: review?(nicolas.b.pierron)
(Assignee)

Updated

4 years ago
Depends on: 925201
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)
Attachment #815078 - Flags: review?(nicolas.b.pierron) → review+

Updated

4 years ago
Depends on: 925305
(Assignee)

Updated

4 years ago
Depends on: 929261
Eric, all dependent bugs are fixed. Can we land this now? This would be great to have for fuzzing.
Flags: needinfo?(efaustbmo)
(Assignee)

Updated

4 years ago
Depends on: 964574
Created attachment 8601467 [details] [diff] [review]
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.
Attachment #8601467 - Flags: feedback?(efaustbmo)
(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_…
Created attachment 8601490 [details] [diff] [review]
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.
Attachment #8601467 - Attachment is obsolete: true
Attachment #8601467 - Flags: feedback?(efaustbmo)
Depends on: 1161584
Depends on: 1161605
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
Created attachment 8602721 [details] [diff] [review]
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
Attachment #8601490 - Attachment is obsolete: true
Attachment #8602721 - Flags: review?(efaustbmo)
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 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.
Attachment #8602721 - Flags: feedback?(jruderman)
Attachment #8602721 - Flags: feedback?(gary)
Attachment #8602721 - Flags: feedback?(choller)
(Assignee)

Comment 13

3 years ago
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.
Attachment #8602721 - Flags: review?(efaustbmo) → review+
Depends on: 1165262
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.
Attachment #8602721 - Flags: feedback?(gary) → feedback+
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?
https://hg.mozilla.org/mozilla-central/rev/ea9608e33abe
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(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.)
Created attachment 8620417 [details] [diff] [review]
remove gotos
Attachment #8620417 - Flags: review?(nicolas.b.pierron)
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?
(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.
Attachment #8620417 - Flags: review?(nicolas.b.pierron) → review+

Updated

3 years ago
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41-]
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.
Attachment #8602721 - Flags: feedback?(choller)
(Assignee)

Updated

2 years ago
Flags: needinfo?(efaustbmo)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.