Heavyweight function inlining had to be disabled because of an issue with inline frame iteration and non-const callees. The issue was this: The most common use of heavyweight function inlining was for callbacks, e.g. code such as: function outer() { var v = ...; array.forEach(function () { /*accesses V*/ ... }); } The inner function accesses, and requires a call object, making it heavyweight. However, the callee in this case is the result of a JSOP_LAMBDA clone of the original inner function. When traversing Ion inlined stack frames, the callee function for an inlined frame is retreived from the snapshotted call-parameters slots from the parent frame. However, if the callee was non-constant, and stored in a register, and never used after the inlined call, then the register allocator would assume that after entering the inlined function code, the register holding the callee value could be clobbered at will. This screwed up inline frame walking, since the caller frame's callee-arg-slot would refer to registers that were clobbered. The solution ultimately is to force the regalloc to keep the value alive, by emitting a "dummy" instruction whose sole purpose is to keep the callee value alive until after the inlined function's dynamic scope has been exited.

Re-enables heavyweight function inlining. Adds an MForceUse instruction that is added in the return block of any inlined function which has a non-constant callee. MForceUse generates no code, and aliases nothing, but is non movable, and marked effectful so DCE doesn't eliminate it. It also takes the callee value as an input, forcing the callee to be kept alive. I replicated the original crash in 883973, and the MForceUse addition here fixes it. Running through try now.

(In reply to Kannan Vijayan [:djvj] from comment #1) > MForceUse generates no code, and aliases nothing, but is non movable, and > marked effectful so DCE doesn't eliminate it. You can call setGuard() in the constructor to avoid the virtual isEffectful.

Ah, thanks! Did that. Running through try one last time and I'll also verify that the page crash is gone again.

Makes change suggested by Jan. Also fixes some issues that caused orange in previous try run. Latest try run is reasonably clean on linux and linux64: https://tbpl.mozilla.org/?tree=Try&rev=438586caeddf

Comment on attachment 781267 [details] [diff] [review] Re-enable heavyweight inlining (rev 2) Review of attachment 781267 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch. What surprised me is that the register allocator does not consider that the snapshot are capturing allocations which needs to remain live (even if spilled on the stack). I think this is a bug which needs to be fixed as this would be a blocker for any branch/instruction removal optimization which needs to bailout with the full & correct state. In fact, I think addressing the register allocator side of the problem would also cover this issue at the same time. I will still give an r+ here, as this patch might be easier to backport. Please add a comment to mention that MForceUse can be removed as soon as the register allocator can guarantee that all slots captured in a snapshot must remain live, and unless you want to fix the register allocator here, I recommend to open a follow-up bug and mention the bug number in the comment. ::: js/src/ion/Lowering.cpp @@ +69,5 @@ > + return false; > + return add(lir); > + } > + > + LForceUseT *lir = new LForceUseT(useAnyOrConstant(ins->input())); Q: would that also give us stack allocations? or only registers/constant?

The push message had the wrong bug in it (the duplicate instead of this one). Changing the duplicate pointers on the bug so as to avoid a backout and repush.