927031 - GenerationalGC: jit-test auto-regress/bug702915.js crashes in browser builds

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

This crashes in linux debug builds:

FAIL - auto-regress/bug702915.js
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/auto-regress/bug702915.js | --ion-eager
INFO exit-status     : -11
INFO timed-out       : False
INFO stdout          > ReferenceError: jstop is not defined
INFO stdout          > ReferenceError: MyObject is not defined
INFO stdout          > ReferenceError: jsTestDriverEnd is not defined
INFO stdout          > ReferenceError: exitFunc is not defined
INFO stdout          > 
INFO stderr         2> ReferenceError: jstop is not defined
INFO stderr         2> ReferenceError: MyObject is not defined
INFO stderr         2> ReferenceError: jsTestDriverEnd is not defined
INFO stderr         2> ReferenceError: exitFunc is not defined
INFO stderr         2>

Comment 1

[Jon Coppeard (:jonco)]

Attachment: browser-jit-failure

When I added MacroAssembler::maybeCallPostBarrier(), I tried to get away without making it always use a scratch register, but I messed up the ABI call logic with the effect that sometimes one of the argument registers gets used as scratch for this, overwriting it.

In the interest of simplicity, this patch makes it always take a scratch register.  Hopefully this is correct now.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 817712 [details] [diff] [review]
browser-jit-failure

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

This seems fine, but why don't we insert a separate MPostWriteBarrier instruction in IonBuilder like we do for setprop, initelem etc? That seems preferable if it doesn't regress perf.

::: js/src/jit/IonMacroAssembler.h
@@ +683,5 @@
>       * Call the post barrier if necessary when writing value to a slot or
>       * element of object.
>       *
>       * Returns whether the maybeScratch register was used.
>       */

Nit: remove the last line.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #2)

Thanks for the quick review!

> why don't we insert a separate MPostWriteBarrier
> instruction in IonBuilder like we do for setprop, initelem etc?

I think because it's used for generating an IC (in GenerateSetDenseElement() in IonCaches.cpp) we don't have that option.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #3)
> I think because it's used for generating an IC (in GenerateSetDenseElement()
> in IonCaches.cpp) we don't have that option.

That doesn't matter, we also use an IC for SETPROP etc. SetElementIC is the only Ion IC that has its own post-barrier logic.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #4)

Ok, probably because I didn't understand there was a better way.  Are you saying we can just add an MPostWriteBarrier intstruction in IonBuilder::setElemTryCache()?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #5)
> Ok, probably because I didn't understand there was a better way.  Are you
> saying we can just add an MPostWriteBarrier intstruction in
> IonBuilder::setElemTryCache()?

Yup, adding something like this right before MSetElementCache::New should work I think:

    if (NeedsPostBarrier(info(), value))
        current->add(MPostWriteBarrier::New(object, value));

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug927031-browser-jit-failure

Great, that is much simpler!

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 818414 [details] [diff] [review]
bug927031-browser-jit-failure

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

Nice :)

Comment 9

[Jon Coppeard (:jonco)]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db52454329c6