636820 - Test for bug 634590 fails with -a -m

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Just noticed this when testing my jsop_case patch.

jit-test/tests/basic/testBug634590.js:12: Error: Assertion failed: got "outerouterouterouterouterouterouterouterouterouter", expected "innerinnerinnerinnerinnerinnerinnerinnerinnerinner"

Bug 634590 is/was a hardblocker, so asking blocking-2.0.

Comment 1

[Andreas Gal :gal]

Need help from the JM guys.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Andreas, the patch in bug 634590 changed jsinterp but not StubCalls.cpp?

Comment 3

[Andreas Gal :gal]

I thought we did, but maybe not right.

Comment 4

[David Mandelin [:dmandelin]]

Attachment: WIP

This fixes the bug. It causes no perf regression on V8, but there is a 3 ms on SunSpider with -m -a and -m -j -p. I'll try to add a fast path for the common case.

Comment 5

[David Mandelin [:dmandelin]]

Attachment: WIP 2

This one seems not to regress perf. I'll post a cleaned-up version for review in a bit.

Comment 6

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 7

[Andreas Gal :gal]

Comment on attachment 515254 [details] [diff] [review]
Patch

Logic seems sound. mjit details dvander has to review.

Comment 8

[Brendan Eich [:brendan]]

Brian noticed this and mentioned it on IRC today -- sorry I didn't report it then.

/be

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 515254 [details] [diff] [review]
Patch

>+    /* If the callee is not an object, jump to the inline fast path. */
>+    MaybeJump isNotObj;
>+    if (!fval->isType(JSVAL_TYPE_OBJECT))
>+        isNotObj = frame.testObject(Assembler::NotEqual, fval);
>+
>+    /*
>+     * If the callee is not a function, jump to OOL slow path.
>+     */
>+    RegisterID objReg = frame.tempRegForData(fval);

If the earlier branch was emitted, then this second tempRegForData call could evict something, causing a register state mismatch at the join point. What we probably want to do is:

> MaybeRegisterID typeReg = frame.maybePinType(fval);
> RegisterID objReg = frame.copyDataIntoReg(fval);
> 
> MaybeJump isNotObj;
> if (!fval->isType(JSVAL_TYPE_OBJECT)) {
>     isNotObj = frame.testObject(Assembler::NotEqual, fval);
>     frame.maybeUnpinType(typeReg);
> }
> 

>+    RegisterID parentReg = frame.allocReg();

Same here, this could evict inside a control-flow arm, but with the copyRegToData() call above, we can just clobber objReg.

Comment 10

[David Mandelin [:dmandelin]]

Attachment: v2

Comment 11

[Andreas Gal :gal]

Is this ready to land?

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yes.

Comment 13

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/tracemonkey/rev/672a84576ca6

Comment 14

[David Mandelin [:dmandelin]]

Backed out due to Unix build failures:

http://hg.mozilla.org/tracemonkey/rev/a7c87d6ee78f

Should be able to sort it out this afternoon.

Comment 15

[David Mandelin [:dmandelin]]

Relanded:

http://hg.mozilla.org/tracemonkey/rev/83242d9362cd

Comment 16

[Nicholas Nethercote [inactive]]

The added test testBug634590ma.js fails for me when run with '-m -a':

/home/njn/moz/ws7/js/src/debug32/shell/js -m -a -e "const platform='linux2'; const libdir='/home/njn/moz/ws7/js/src/jit-test/lib/';" -f /home/njn/moz/ws7/js/src/jit-test/lib/prolog.js -f /home/njn/moz/ws7/js/src/jit-test/tests/basic/testBug634590ma.js
/home/njn/moz/ws7/js/src/jit-test/tests/basic/testBug634590ma.js:14: Error: Assertion failed: got "outerouterouterouterouterouterouterouterouterouter", expected "innerinnerinnerinnerinnerinnerinnerinnerinnerinner"

This is preventing me from landing bug 635155.

Comment 17

[Nicholas Nethercote [inactive]]

(In reply to comment #16)
> The added test testBug634590ma.js fails for me when run with '-m -a':

It's working now.  I think I forgot to rebuild after updating.  Sorry for the noise.

Comment 18

[LegNeato]

(In reply to comment #15)
> Relanded:
> 
> http://hg.mozilla.org/tracemonkey/rev/83242d9362cd

Can we get this on m-c for tonight's nightly?

Comment 19

[David Mandelin [:dmandelin]]

(In reply to comment #18)
> (In reply to comment #15)
> > Relanded:
> > 
> > http://hg.mozilla.org/tracemonkey/rev/83242d9362cd
> 
> Can we get this on m-c for tonight's nightly?

http://hg.mozilla.org/mozilla-central/rev/ce9aafdae381