1188586 - Assertion failure: base.isNumber() && power.isNumber(), at jit/Recover.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

(function(y) {
    for (var k = 0; k < 9; ++k) {
        try {
            y ** y == a;
        } catch (e) {}
    }
})();

asserts js debug shell on m-c changeset 833403badc41 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: base.isNumber() && power.isNumber(), at jit/Recover.cpp.

Configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 833403badc41

=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150727164442" and the hash "45ad89111683".
The "bad" changeset has the timestamp "20150727165146" and the hash "c0d0135b9860".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=45ad89111683&tochange=c0d0135b9860

Mariusz, is bug 1135708 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x14f2b2, 0x0000000100691e29 js-dbg-64-dm-nsprBuild-darwin-833403badc41`js::jit::RPow::recover(this=<unavailable>, cx=<unavailable>, iter=<unavailable>) const + 281 at Recover.cpp:732, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100691e29 js-dbg-64-dm-nsprBuild-darwin-833403badc41`js::jit::RPow::recover(this=<unavailable>, cx=<unavailable>, iter=<unavailable>) const + 281 at Recover.cpp:732
    frame #1: 0x00000001005cc3c3 js-dbg-64-dm-nsprBuild-darwin-833403badc41`js::jit::SnapshotIterator::computeInstructionResults(this=<unavailable>, cx=0x000000010284c400, results=0x00007fff5fbfe438) const + 307 at JitFrames.cpp:2250
    frame #2: 0x00000001005cb84e js-dbg-64-dm-nsprBuild-darwin-833403badc41`js::jit::SnapshotIterator::initInstructionResults(this=0x00007fff5fbfd5f0, fallback=0x00007fff5fbfd4e0) + 542 at JitFrames.cpp:2204
    frame #3: 0x0000000100476712 js-dbg-64-dm-nsprBuild-darwin-833403badc41`js::jit::BailoutIonToBaseline(JSContext*, js::jit::JitActivation*, js::jit::JitFrameIterator&, bool, js::jit::BaselineBailoutInfo**, js::jit::ExceptionBailoutInfo const*) [inlined] SnapshotIteratorForBailout::init(this=<unavailable>) + 60 at BaselineBailouts.cpp:436
    frame #4: 0x00000001004766d6 js-dbg-64-dm-nsprBuild-darwin-833403badc41`js::jit::BailoutIonToBaseline(cx=0x000000010284c400, activation=0x00007fff5fbfe360, iter=0x00007fff5fbfd728, invalidate=true, bailoutInfo=0x00007fff5fbfd720, excInfo=0x00007fff5fbfdf58) + 598 at BaselineBailouts.cpp:1481
(lldb)

Comment 2

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

Eric mentioned f2f to needinfo? him instead.

Comment 3

[Eric Faust [:efaust]]

Attachment: Fix?

I think this is right. Jan suggested we go with the same approach as we do for other binary arithmetic instructions. This mimics that approach.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8645244 [details] [diff] [review]
Fix?

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

Makes sense, but if we want to copy the binary arithmetic approach, I think we should:

(1) Use the None specialization if one of the inputs is non-numeric.
(2) Box the inputs as Value in the None case (cf. ArithPolicy::adjustInputs, maybe we can use that policy here?)
(3) Fix MPow::getAliasSet() to behave like MBinaryArithInstruction::getAliasSet() in this case
(4) Lower to LPowV, like LBinaryV.

I know it's a bunch of extra work, but I think it's nice to avoid bailing out unconditionally (from MToDouble) when someone does object ** object or something silly.

Comment 5

[Eric Faust [:efaust]]

Landing a spot-fix to lower the fuzzer pressure. More on this to come, as a result of Jan's review.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26b37ffc80f3

Comment 7

[Pulsebot]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26b37ffc80f3
https://hg.mozilla.org/mozilla-central/rev/c32a64e2e4dc

Comment 9

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 6457f01e4bcb).

Comment 10

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

(In reply to Fuzzing Team from comment #9)
> JSBugMon: The testcase found in this bug no longer reproduces (tried
> revision 6457f01e4bcb).

No longer reproduces because the spot fix landed.

Comment 11

[Robin Templeton [:terpri]]

Attachment: Fix exponentiation of non-primitive values.

I've attached a patch following the approach suggested in comment 4. The original test case no longer crashes SpiderMonkey, due to changes from bug 1195588.  However, exponentiation of objects still appears to cause unnecessary bailouts when shared stubs are disabled. This patch adds an MPow specialization for non-numeric operands and re-enables the recover instruction for MPow.

(Also, this is my first SpiderMonkey patch submission! So I would appreciate any and all feedback)

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8808891 [details] [diff] [review]
Fix exponentiation of non-primitive values.

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

Thanks! This looks really nice. I'm clearing the review from efaust as he left Mozilla a few weeks ago.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8808891 [details] [diff] [review]
Fix exponentiation of non-primitive values.

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

::: js/src/jit/MIR.h
@@ +6561,2 @@
>          specialization_ = powerType;
>          setResultType(MIRType::Double);

Actually there's an issue here: I think we should set this to MIRType::Value in the untyped case, because the LIR instruction will return a Value, not a double.

Are you not getting assertion failures somewhere, especially on 32-bit? If not we could add an assertion to LIRGeneratorShared::defineReturn, something like: lir->numDefs() should be BOX_PIECES if we're on 32-bit, else it should be 1.

If anything is unclear, feel free to ping me in #jsapi on irc.mozilla.org

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> something
> like: lir->numDefs() should be BOX_PIECES if we're on 32-bit, else it should
> be 1.

Oh this is for the MIRType::Value case. For most other types (like MIRType::Double here), lir->numDefs() should be 1 everywhere.

Comment 15

[Robin Templeton [:terpri]]

Attachment: Fix exponentiation of non-primitive values.

Thanks for the review! I tested the old patch on i386 and it did cause pow to produce incorrect results, so I changed the MPow result type as you suggested.

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8810745 [details] [diff] [review]
Fix exponentiation of non-primitive values.

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

Thanks again.

Comment 17

[Jan de Mooij [:jandem]]

I pushed this to the try server:

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

Comment 18

[Jan de Mooij [:jandem]]

This is failing the following test: jit-test/tests/ion/dce-with-rinstructions.js That file has some comments like "// POW recovery temporarily disabled. See bug 1188586.". We should probably update these tests.

You can reproduce this locally like this:

  $ jit-test/jit_test.py $SHELL dce-with-rinstructions

Comment 19

[Robin Templeton [:terpri]]

Attachment: 0001-Bug-1188586-Fix-exponentiation-of-non-primitive-valu.patch

Updated test

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25b0168c9cc7
Fix exponentiation of non-primitive values. r=jandem

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/25b0168c9cc7

Comment 22

[areya]

https://hg.mozilla.org/mozilla-central/rev/25b0168c9cc7

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Should we consider backporting this to Aurora/Beta? Also, looks like we didn't include a test for this?

Comment 24

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> Should we consider backporting this to Aurora/Beta? Also, looks like we
> didn't include a test for this?

I don't think we have to backport this, a stopgap for the assertion failure landed more than a year ago (see comment 8).

The patch that landed in 53 fixes this in a nicer way, but does not have to be backported. I'll push the test in comment 0.

Comment 25

[Jan de Mooij [:jandem]]

And sorry for the confusion! We should have used a different bug for the followup work.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58cfccdba9be
Add testcase. r=me

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/58cfccdba9be