Closed Bug 927031 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file, 1 obsolete file)

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>
Blocks: 764882
Attached patch browser-jit-failure (obsolete) — Splinter Review
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.
Attachment #817712 - Flags: review?(jdemooij)
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.
Attachment #817712 - Flags: review?(jdemooij) → review+
(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.
(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.
(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()?
(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));
Great, that is much simpler!
Attachment #817712 - Attachment is obsolete: true
Attachment #818414 - Flags: review?(jdemooij)
Comment on attachment 818414 [details] [diff] [review]
bug927031-browser-jit-failure

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

Nice :)
Attachment #818414 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/db52454329c6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Duplicate of this bug: 923187
You need to log in before you can comment on or make changes to this bug.