Closed Bug 946284 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

(Keywords: regression, testcase, Whiteboard: [qa-])

Attachments

(2 files)

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)
Gary, would it be easy to run autoBisect and/or lithium on this? :)
Flags: needinfo?(gary)
It might be possible, if one could have the testcase attached here.
Flags: needinfo?(jdemooij)
Attached is the testcase. I concatenated the two source files into a single file for convenience. Run with --ion-eager to reproduce the failure.
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.
Flags: needinfo?(jdemooij)
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?
Flags: needinfo?(gary) → needinfo?(jdemooij)
(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.
Flags: needinfo?(gary)
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?
Blocks: 942236
Flags: needinfo?(sunfish)
Flags: needinfo?(jdemooij)
Flags: needinfo?(gary)
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
Attached patch bug946284.patchSplinter Review
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.
Attachment #8342980 - Flags: review?(nicolas.b.pierron)
Flags: needinfo?(sunfish)
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?
Attachment #8342980 - Flags: review?(nicolas.b.pierron) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/7c60951f9964
Assignee: nobody → sunfish
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-
Whiteboard: [qa- → [qa-]
You need to log in before you can comment on or make changes to this bug.