The generator object stores aside values from the stack of the generator function when the generator is not running. These values need to properly root objects in the nursery.

This is much less straightforward than I initially thought. The layout of the stack is: - args (may contain two copies, one which we need to poison) - StackFrame - slots We cannot just copy the full range as HeapValues because StackFrame does not contain value data. If we try to treat random segments of it as a HeapValues, kaboom. However, we do need to copy the StackFrame, it just needs to be separate, and separate our Value copying. Secondly, we cannot just copy numSlots values after the StackFrame, because this is the total reserved slots, which may not yet be initialized when we yield or resume. Instead we have to compute the count using the (correct) stack pointer. With these as well, touch the wrong ones as a HeapValue and kaboom. When we handle all of these edges, we get something much less elegant than a simple PodCopy, but hopefully, not too ugly to live. The attached patch passes all tests in the interpreter and in JaegerMonkey with -Z 3,100.

I just realized that the current patch did not remove the SetValueRangeToCrash in ensureFrameInvarients that we are now duplicating (with minor changes) outside that code. The relevant change is that when pushing, we set the HeapValues on the floating generator to Undefined, rather than a crashing value. My thought was that the .set we do on these HeapValues when we pop would crash, but this does not appear to happen. Is this simply something that isn't being tested, or is this not a bug for some other reason that I'm not seeing?

This is now ready for review, thanks for waiting!

Trivial change to use *genfp (instead of *gen->floating) to match all of our other accesses to the genfp in the blocks where we use it.

Comment on attachment 585599 [details] [diff] [review] v3: Use genfp instead of gen->floating On Luke's advise, I should be able to templatize stealFrameAndSlots to make this typecheck correctly so we can keep the guts hidden. Will resubmit this in a bit.

This is much, much nicer. Currently running a try build: https://tbpl.mozilla.org/?tree=Try&rev=64017c8f2d46

Comment on attachment 586484 [details] [diff] [review] v4: rewritten as a template and merged with 714913 Review of attachment 586484 [details] [diff] [review]: ----------------------------------------------------------------- This looks good aside from the stuff below. ::: js/src/jsiter.cpp @@ +1202,5 @@ > * pushed by the mjit and need to be conservatively marked. Technically, the > * formal args and generator slots are safe for exact marking, but since the > * plan is to eventually mjit generators, it makes sense to future-proof > * this code and save someone an hour later. > */ Please remove the comment. Now that we're not using the conservative scanner, it's no longer relevant. ::: js/src/vm/Stack.cpp @@ +138,4 @@ > > + /* Copy args, StackFrame, and slots as raw Values. */ > + PodCopy((Value *)vp, (Value *)othervp, othersp - (Value *)othervp); > + JS_ASSERT((Value *)vp == this->actualArgs() - 2); Now that it's templated, we can take advantage of assignment operators instead of using PodCopy. Something like this, maybe: U *srcend = (U *)otherfp->formalArgsEnd(); T *dst = vp; for (U *src = othervp; src < srcend; src++, dst++) *dst = *src; *fp = *otherfp; if (doPostBarrier) fp->writeBarrierPost(); srcend = (U *)othersp; dst = (T *)fp->slots(); for (U *src = (U *)otherfp->slots(); src < srcend; src++, dst++) *dst = *src; @@ +155,5 @@ > > /* Catch bad-touching of non-canonical args (e.g., generator_trace). */ > if (otherfp->hasOverflowArgs()) > + Debug_SetValueRangeToCrashOnTouch((Value *)othervp, > + (Value *)othervp + 2 + otherfp->numFormalArgs()); I'm pretty sure we don't want to do Debug_SetValueRangeToCrashOnTouch anymore. It's writing bogus values to the source frame, which could be either the generator frame or the VM stack frame. This patch makes us mark the generator frame non-conservatively, so we would crash after several iterations of this when overflow args are used. And I have a patch that makes us scan VM stack slots non-conservatively. So let's just take this out.

(In reply to Bill McCloskey (:billm) from comment #8) > Please remove the comment. Now that we're not using the conservative > scanner, it's no longer relevant. The conservative scanner isn't being used at all for generators? Is that done in this patch or has it already landed? There's a bug around somewhere I should close if that is removed.

(In reply to Andrew McCreight [:mccr8] from comment #9) > The conservative scanner isn't being used at all for generators? Is that > done in this patch or has it already landed? There's a bug around somewhere > I should close if that is removed. This patch removes it for generators.

Running it through try again: https://tbpl.mozilla.org/?tree=Try&rev=27fe295675ec

Comment on attachment 595502 [details] [diff] [review] v5: With review comments addressed. Review of attachment 595502 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks! ::: js/src/vm/Stack.cpp @@ +191,5 @@ > + if (isFunctionFrame()) { > + JSFunction::writeBarrierPost((JSObject *)exec.fun, (void *)&exec.fun); > + if (isEvalFrame()) { > + JSScript::writeBarrierPost(u.evalScript, (void *)&u.evalScript); > + } One nit: the braces are not needed here, since this is a one-line conditional.

Android's gcc appears to have choked on the templates. Re-trying with explicit instantiation: https://tbpl.mozilla.org/?tree=Try&rev=4739efe91a1b

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1b57eff04967 - betting on GC crashes is high-risk, but I'm betting that the ones during browser-chrome in https://tbpl.mozilla.org/php/getParsedLog.php?id=9227022&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=9229046&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=9227272&tree=Mozilla-Inbound were from this.

Lets just give this a full try run as is, to see if it is indeed the cause: https://tbpl.mozilla.org/?tree=Try&rev=9e6922c3ca10

Whoops, prior try push was patched with --dry-run: https://tbpl.mozilla.org/?tree=Try&rev=4b2c7f02564a

And I've tried the wrong patch. This will teach me to skip steps. https://tbpl.mozilla.org/?tree=Try&rev=caafc725c109

Does this mean that MarkStackRangeConservatively isn't used anywhere? MXR can't find any uses of it, except as a comment in Stack.cpp.

Yes, it's not used anywhere. And I forgot to fix the comment.