Closed Bug 636820 Opened 9 years ago Closed 9 years ago

Test for bug 634590 fails with -a -m

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jandem, Assigned: dmandelin)

References

Details

(Whiteboard: [hardblocker][has patch][fixed-in-tracemonkey])

Attachments

(1 file, 3 obsolete files)

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.
Need help from the JM guys.
Andreas, the patch in bug 634590 changed jsinterp but not StubCalls.cpp?
blocking2.0: ? → final+
Whiteboard: [hardblocker]
I thought we did, but maybe not right.
Assignee: general → dmandelin
Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch WIP 2 (obsolete) — Splinter Review
This one seems not to regress perf. I'll post a cleaned-up version for review in a bit.
Attachment #515185 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Attachment #515223 - Attachment is obsolete: true
Attachment #515254 - Flags: review?(dvander)
Attachment #515254 - Flags: review?(gal)
Comment on attachment 515254 [details] [diff] [review]
Patch

Logic seems sound. mjit details dvander has to review.
Attachment #515254 - Flags: review?(gal) → review+
Brian noticed this and mentioned it on IRC today -- sorry I didn't report it then.

/be
Whiteboard: [hardblocker] → [hardblocker][has patch]
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.
Attachment #515254 - Flags: review?(dvander)
Attached patch v2Splinter Review
Attachment #515254 - Attachment is obsolete: true
Attachment #515281 - Flags: review?(dvander)
Attachment #515281 - Flags: review?(dvander) → review+
Is this ready to land?
http://hg.mozilla.org/tracemonkey/rev/672a84576ca6
Status: NEW → ASSIGNED
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][fixed-in-tracemonkey]
Backed out due to Unix build failures:

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

Should be able to sort it out this afternoon.
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.
Blocks: 635155
(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.
(In reply to comment #15)
> Relanded:
> 
> http://hg.mozilla.org/tracemonkey/rev/83242d9362cd

Can we get this on m-c for tonight's nightly?
(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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 671947
You need to log in before you can comment on or make changes to this bug.