Last Comment Bug 717254 - IonMonkey: generateVMWrapper clobbers non-volatile registers
: IonMonkey: generateVMWrapper clobbers non-volatile registers
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 677337
  Show dependency treegraph
Reported: 2012-01-11 08:46 PST by Jan de Mooij [:jandem]
Modified: 2012-01-11 14:40 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (4.40 KB, patch)
2012-01-11 12:58 PST, Jan de Mooij [:jandem]
nicolas.b.pierron: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2012-01-11 08:46:20 PST
The second bug triggered by spectral-norm (bug 717193 is the other one).

The problem is that generateVMWrapper takes registers from the non-volatile set, but OOL VM calls only push/pop the volatile regs. So OOL calls can clobber live registers in the non-volatile set.

I discussed this a bit with bhackett, he suggested to push/pop all live registers for OOL calls, volatile and non-volatile.
Comment 1 Nicolas B. Pierron [:nbp] 2012-01-11 10:05:17 PST
Before a VM call you should always clobber Markable data such as Value, String and Objects and data stored in volatile registers, if generateVMWrapper is restricting it-self to volatile registers or is saving non-volatile registers.  Otherwise all should be clobbered.

This rule does not hold for JS Calls because we do not consider different state of registers, thus all should be clobbered.

The simple thing to do, as we already discuss with dvander, was to clobber everything and to see later if there is any need of optimization.  This optimization should avoid spilling non-volatile registers containing Double or Int around VM calls.

To refer to bug 717193, Move group are not made to work with base offset of memory access.  The point of MoveGroup was to handle some kind instantaneous move from one position to another among stack slots and registers.  One of the (non-implemented) assertion made in the MoveGroup, is that the base register is the stack pointers.  Unfortunately, due to the mis-aligned stack, we cannot rely on the stack pointer (at least on x86 and x64) to copy the arguments, and thus we need an extra register (argBase).  By using volatile registers for argBase we make sure with are working with undefined behavior of the MoveGroup.

One simple solution, is to restrict generateVMWrapper/argBase to non-volatile registers, such as we can avoid this bug.  This is guarantee to work under the Move Resolver, because we have no intersection between argument registers (subset of volatile registers) and non-volatile registers.
Comment 2 Jan de Mooij [:jandem] 2012-01-11 12:58:44 PST
Created attachment 587793 [details] [diff] [review]

As we discussed on IRC, this patch just saves all registers. I'll file a follow-up bug to optimize this.
Comment 3 Jan de Mooij [:jandem] 2012-01-11 13:16:31 PST
(In reply to Jan de Mooij (:jandem) from comment #2)
> As we discussed on IRC, this patch just saves all registers. I'll file a
> follow-up bug to optimize this.

Bug 717374.
Comment 4 Nicolas B. Pierron [:nbp] 2012-01-11 13:45:40 PST
Comment on attachment 587793 [details] [diff] [review]

Review of attachment 587793 [details] [diff] [review]:

Looks good.
Comment 5 Jan de Mooij [:jandem] 2012-01-11 14:40:42 PST

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