1069307 - Turn Scalar Replacement on by default.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Enable scalar replacement by default.

This bug is for turning on Scalar Replacement and filing security issues which have to be fixed before landing this patch.

Scalar replcamenet can be turn on by either by applying the current patch, or by running the JavaScript shell with the  --ion-scalar-replacement=on  CLI option.

Comment 1

[Christian Holler (:decoder)]

Comment on attachment 8491471 [details] [diff] [review]
IonMonkey: Enable scalar replacement by default.

I'm hitting multiple assertions, but they all eventually reduce to the same one. I've included three tests that hit this assertion because I wasn't sure if it's really all the same bug. Let me know when this one is fixed, so I can rerun the testing :)



Assertion failure: iter->type() != MIRType_None, at jit/IonAnalysis.cpp:1839
Options: --fuzzing-safe --ion-scalar-replacement=on
Test:

function init() {
    while ( {}, this) !(Object === "Infinity");      
}
eval("init()");

====


Assertion failure: iter->type() != MIRType_None, at jit/IonAnalysis.cpp:1839
Options: --fuzzing-safe --no-threads --ion-scalar-replacement=on --ion-eager
Tests:

function testCallProtoMethod() {
    var a = [new X, new X, __proto__, new new this, new Y];
}
testCallProtoMethod();

====

function testNot() {
    var r;
    for (var i = 0; i < 10; ++i)
        r = [];
}
testNot();

Comment 2

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

Comment on attachment 8491471 [details] [diff] [review]
IonMonkey: Enable scalar replacement by default.

I've been busy lately, so clearing feedback? since :decoder has a feedback-.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8491471 [details] [diff] [review]
IonMonkey: Enable scalar replacement by default.

Asking for feedback again since Bug 1007291 is now riding the train on inbound, and soon on master.

Comment 4

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

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Asking for feedback again since Bug 1007291 is now riding the train on
> inbound, and soon on master.

I guess you mean bug 1072911? (might start testing inbound with the patch in comment 0)

Comment 5

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

Also, I think you might need to refresh your patch here since the following checkin likely changed stuff:

$ hg log -p -r ad1b182f0f51
changeset:   206148:ad1b182f0f51
user:        Marty Rosenberg <[e-mail]>
date:        Fri Sep 19 07:41:08 2014 -0400
summary:     bug 1068857: Allow overriding almost any of the jit options via the UNIX environment. (r=nbp)

Nonetheless the patch is trivial enough for me to make this change manually:

$ hg diff
diff --git a/js/src/jit/JitOptions.cpp b/js/src/jit/JitOptions.cpp
--- a/js/src/jit/JitOptions.cpp
+++ b/js/src/jit/JitOptions.cpp
@@ -62,17 +62,17 @@ JitOptions::JitOptions()
     // Whether to enable extra code to perform dynamic validation of
     // RangeAnalysis results.
     SET_DEFAULT(checkRangeAnalysis, false);

     // Whether Ion should compile try-catch statements.
     SET_DEFAULT(compileTryCatch, true);

     // Toggle whether eager scalar replacement is globally disabled.
-    SET_DEFAULT(disableScalarReplacement, true); // experimental
+    SET_DEFAULT(disableScalarReplacement, false);

     // Toggle whether global value numbering is globally disabled.
     SET_DEFAULT(disableGvn, false);

     // Toggles whether loop invariant code motion is globally disabled.
     SET_DEFAULT(disableLicm, false);

     // Toggles whether inlining is globally disabled.

Comment 6

[Christian Holler (:decoder)]

You don't need any patch for testing, just use the shell flag :)

Comment 7

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

Comment on attachment 8491471 [details] [diff] [review]
IonMonkey: Enable scalar replacement by default.

do {
    ({
        b: a
    })
} while (x)


$ ./js-dbg-opt-64-dm-nsprBuild-linux-82d07982831c-1069307-c5-d32d700d4756 --ion-eager --no-threads w6107-reduced.js
Assertion failure: !succ->phisEmpty(), at jit/ScalarReplacement.cpp:340

and crashes js opt shell at replaceOperand.

Debug configure options:

AR=ar sh /home/fuzz2lin/trees/mozilla-inbound/js/src/configure --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Comment 8

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

Attachment: stack for testcase in comment 7

Comment 9

[Christian Holler (:decoder)]

Comment on attachment 8491471 [details] [diff] [review]
IonMonkey: Enable scalar replacement by default.

mozilla-central rev 2ae57957e4bb


Assertion failure: !succ->phisEmpty(), at jit/ScalarReplacement.cpp:340

Options: --ion-scalar-replacement=on
Test:

for (var i =0; 1; i++)
  ({ set: Math.w });



Assertion failure: !hasFlags(1 << Discarded), at jit/MIR.h:561

Options: --ion-scalar-replacement=on --ion-offthread-compile=off --ion-eager
Test:

test('\
var items = new Array( "one", "two", "three" );\
var itemsRef = items;\
if(itemsRef[1])\
  (itemsRef[1])\
');
test('');
function test(x) {
try { new Function(x)(); } catch(exc1) {}
}

Comment 10

[Christian Holler (:decoder)]

Saw another one here, that looks different:


Assertion failure: iter->isInterruptCheck() || iter->isInterruptCheckPar(), at jit/shared/CodeGenerator-shared.cpp:1257


Options: --fuzzing-safe --thread-count=2 --ion-offthread-compile=off --ion-eager --ion-check-range-analysis --ion-regalloc=backtracking --ion-scalar-replacement=on
Test:

new Function("\
var o1 = {};\
  o1.a = 3;\
var o5 = { a: 2 };\
for (var i = 0; i < 5; i++)\
  o5.a = 3;\
var o6 = Object.preventExtensions({ a: 2 });\
for (var i = 0; i < 5; i++)\
  o6.a = 3;\
")();

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: Enable scalar replacement by default.

The last remaining patches landed on inbound, it would be good for fuzzing
after the next merge (likely tonight or tomorrow).

Comment 12

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

Comment on attachment 8499653 [details] [diff] [review]
Enable scalar replacement by default.

for (var j = 0; j < 1; j++) {
    (function() {
        [Math.fround(0), ""]
    })()
}

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-138e0dba47e8 --ion-scalar-replacement=on --no-threads --ion-eager testcase.js
Assertion failure: consumer->isConsistentFloat32Use(use.use()), at /Users/skywalker/trees/mozilla-inbound/js/src/jit/IonAnalysis.cpp:1369
Segmentation fault: 11

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-inbound/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Comment 13

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

Tested comment 12 with m-i tip rev 138e0dba47e8:

https://hg.mozilla.org/integration/mozilla-inbound/rev/138e0dba47e8

Comment 14

[Christian Holler (:decoder)]

Comment on attachment 8499653 [details] [diff] [review]
Enable scalar replacement by default.

Haven't seen more failures so far. feedback+ for now :)

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8499653 [details] [diff] [review]
Enable scalar replacement by default.

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

Nice! Might want to give it a few more fuzzing days or wait for the merge, but we can also disable it on Aurora if we see any problems.

Comment 16

[Nicolas B. Pierron [:nbp]]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ea34a3e44c20

https://hg.mozilla.org/integration/mozilla-inbound/rev/baf095348b7e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Does this require any sort of manual testing?

Comment 19

[Nicolas B. Pierron [:nbp]]

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #18)
> Does this require any sort of manual testing?

No, fuzzers are already doing a lot of automated testing way faster than any manual testing.

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Thank you Nicolas!

Comment 21

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

Comment on attachment 8499653 [details] [diff] [review]
Enable scalar replacement by default.

Scalar replacement has landed turned on by default, so setting feedback+ and other issues will be filed as follow-up bugs.