Last Comment Bug 708303 - Use pinReg/unpinReg more in write barriers
: Use pinReg/unpinReg more in write barriers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on: 718765
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-07 10:26 PST by Bill McCloskey (:billm)
Modified: 2012-01-28 19:01 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.62 KB, patch)
2011-12-07 10:26 PST, Bill McCloskey (:billm)
dmandelin: review+
Details | Diff | Splinter Review
patch v2 (7.90 KB, patch)
2011-12-29 14:13 PST, Bill McCloskey (:billm)
dmandelin: review+
Details | Diff | Splinter Review
patch v3 (5.23 KB, patch)
2012-01-25 17:21 PST, Bill McCloskey (:billm)
dmandelin: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2011-12-07 10:26:25 PST
Created attachment 579736 [details] [diff] [review]
patch

Now that pinReg works with syncFancy, we can use it throughout the write barrier code.

This patch fixes one case that was previously broken, and it removes some unnecessary save/restore code in another place.

I'm a little worried about pinning two registers at once.
Comment 1 David Mandelin [:dmandelin] 2011-12-07 16:29:31 PST
Comment on attachment 579736 [details] [diff] [review]
patch

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

I think pinning 2 is OK. The pin code is simple enough and shouldn't have any problems. I rechecked the reg allocator in ImmutableSync, and if all else fails, it can pick a last resort register, which just has to be in the standard set and not pinned.
Comment 3 Bill McCloskey (:billm) 2011-12-15 10:32:27 PST
Backed out. This fails with --jitflags=amdn.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb1a1208df6
Comment 4 Bill McCloskey (:billm) 2011-12-29 14:13:46 PST
Created attachment 584848 [details] [diff] [review]
patch v2

The original patch was wrong because it was calling pinReg on registers that were not owned by the FrameState. This happened in two places:
1. In jsop_setelem_dense, where pinReg was called on a register that is sometimes obtained from tempRegForData (this one is safe) and sometimes from allocReg (no safe)
2. In jsop_setprop, where pinReg was called on a register obtained by allocReg

For the first problem, since we already save the register in question in VMFrame::scratch, I just restored the saved value when pinReg can't be used. pinReg is still used when the register comes from tempRegForData.

In the second problem, the register was already being allocated from SavedRegs. So I changed syncFancy so that it shouldn't clobber registers from SavedRegs. This further limits the set of registers available to syncFancy, but I suspect this should still be safe.
Comment 6 Justin Wood (:Callek) (Away until Aug 29) 2012-01-16 19:55:04 PST
https://hg.mozilla.org/mozilla-central/rev/60eb0da71cdb
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-18 07:14:00 PST
I backed this patch out as a suspected cause of a major new crash on mobile:
https://hg.mozilla.org/mozilla-central/rev/79e5d0b77d10

See bug 718765
Comment 8 Bill McCloskey (:billm) 2012-01-25 17:21:39 PST
Created attachment 591661 [details] [diff] [review]
patch v3

Marty ran into this problem in bug 718852, which is probably the same thing as was happening with the Spiegel site: ARM only has three temp registers, and we had them all pinned. So there was nothing left for syncFancy to use.

This patch is more conservative. There was only one place where I wanted syncFancy to avoid using a SavedReg. So instead I changed that place to reload the register after the sync. It's a little hackier, but at this point I think that's what's called for.
Comment 10 Joe Drew (not getting mail) 2012-01-28 19:01:14 PST
https://hg.mozilla.org/mozilla-central/rev/9b81bf7d458c

Note You need to log in before you can comment on or make changes to this bug.