946284 - Kraken stanford-crypto-ccm fails with --ion-eager

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

With --ion-eager, the stanford-crypto-ccm test in Kraken 1.1 in the AWFY repo fails for me on x64 Linux:

$ js --ion-eager -f arewefastyet/benchmarks/kraken/tests/kraken-1.1/stanford-crypto-ccm-data.js -f arewefastyet/benchmarks/kraken/tests/kraken-1.1/stanford-crypto-ccm.js 
*** FAIL *** CCM mode tests: aes-1024-ccm-encrypt #0
*** FAIL *** CCM mode tests: aes-ccm-decrypt #0 (exn CORRUPT: ccm: tag doesn't match)

Comment 1

[Jan de Mooij [:jandem]]

Gary, would it be easy to run autoBisect and/or lithium on this? :)

Comment 2

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

It might be possible, if one could have the testcase attached here.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: stanford-crypto-ccm-combined.js

Attached is the testcase. I concatenated the two source files into a single file for convenience. Run with --ion-eager to reproduce the failure.

Comment 4

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

sjcl = {}
browserUtil = {
    cpsIterate: function(f, start) {
        f(start, function() {})
    },
    cpsMap: function(map, list, k) {
        browserUtil.cpsIterate(function(i) {
            map([], i, list)
        }, 0)
    }
}
sjcl.test = {
    all: {}
}
sjcl.test.TestCase = function(name, doRun) {
    this.doRun = doRun
    sjcl.test.all[name] = this
}
sjcl.test.TestCase.prototype = {
    run: function() {
        this.doRun(function() {})
    }
}
new sjcl.test.TestCase("", function() {
    browserUtil.cpsIterate(function(j) {
        for (var i = 0; i < 3; i++) {
            try {
                sjcl.y.l
            } catch (e) {
                print(e)
            }
        }
    }, 0)
})
tests = []
for (t in sjcl.test.all) {
    tests.push(t)
}
browserUtil.cpsMap(function(t, i) {
    sjcl.test.all[tests[i]].run()
}, true)

without any CLI arguments, shows:

TypeError: sjcl.y is undefined
TypeError: sjcl.y is undefined
TypeError: sjcl.y is undefined

with --ion-eager, shows:

TypeError: sjcl.y is undefined
TypeError: sjcl.y is undefined
TypeError: sjcl.y.l is undefined

(I think this can be reduced a little further, but I won't dwell on it)

Tested on m-c rev 526e12792fc8.

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --with-ccache --disable-threadsafe

Bisection is next.

Comment 5

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/e473c952d233
user:        Jan de Mooij
date:        Wed Aug 28 13:13:11 2013 +0200
summary:     Bug 909389 - Enable IonMonkey try-catch compilation by default. r=djvj

Jan, is bug 909389 a likely regressor?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #5)
> Jan, is bug 909389 a likely regressor?

Thank you for looking at this! However, it looks like this was reduced to a different, harmless, issue: the expression decompiler we use for error messages can produce slightly different error messages for Ion code. Can you try a more-deterministic build? There shouldn't be different error messages there, I think.

Comment 7

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

function f(x) {
    return x.length / 2
}
f("")
try {
    f(undefined + "")()
} catch (e) {
    print(e)
}


$ ./js stanford-crypto-ccm-combined.js
TypeError: 4.5 is not a function
$ ./js --ion-eager stanford-crypto-ccm-combined.js
TypeError: 4 is not a function

(Take 2, this time with a deterministic build)

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/e14cd09c843e
user:        Dan Gohman
date:        Tue Nov 26 14:40:55 2013 -0800
summary:     Bug 942236 - IonMonkey: Unsigned optimizations for MMod, MDiv, and MUrsh. r=nbp

Dan, is bug 942236 a likely regressor?

Comment 8

[Jan de Mooij [:jandem]]

Thanks a lot Gary. Here's a testcase that's a bit simpler:

function f(x) {
    return x.length / 2
}
f("");
assertEq(f(undefined + ""), 4.5);

This fails with --ion-eager because f returns 4 instead of 4.5

Comment 9

[Dan Gohman [:sunfish]]

Attachment: bug946284.patch

Thanks for the reduced testcases!

This reverts the part of bug 942236 that optimized MDiv to unsigned based on range analysis information, because range analysis doesn't know when the division will produce a fractional result.

Also, it corrects a minor issue from the recent patch for bug 869473 which this fix exposes; LiveRangeAllocator.cpp doen't like LInstructions which claim to have more operands than they really do.

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8342980 [details] [diff] [review]
bug946284.patch

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

::: js/src/jit/RangeAnalysis.cpp
@@ -1332,5 @@
> -        // Also, we can optimize by converting this to an unsigned div.
> -        if (specialization() == MIRType_Int32 &&
> -            !lhs.canHaveFractionalPart() && !rhs.canHaveFractionalPart())
> -        {
> -            unsigned_ = true;

This is funny how names can be miss-leading even on small reviews.
Maybe we should renamed unsigned_ to uint32Specialization?

Comment 11

[Dan Gohman [:sunfish]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> Comment on attachment 8342980 [details] [diff] [review]
> bug946284.patch
> 
> Review of attachment 8342980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/RangeAnalysis.cpp
> @@ -1332,5 @@
> > -        // Also, we can optimize by converting this to an unsigned div.
> > -        if (specialization() == MIRType_Int32 &&
> > -            !lhs.canHaveFractionalPart() && !rhs.canHaveFractionalPart())
> > -        {
> > -            unsigned_ = true;
> 
> This is funny how names can be miss-leading even on small reviews.
> Maybe we should renamed unsigned_ to uint32Specialization?

Perhaps, but "specialization" doesn't seem quite right either, since the specialization() == MIRType_Int32 in this code doesn't imply a non-fractional result either. I'll think about it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c60951f9964

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c60951f9964