Last Comment Bug 714109 - Add missing post write barriers on Generators
: Add missing post write barriers on Generators
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Terrence Cole [:terrence]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 714913 (view as bug list)
Depends on: 715943
Blocks: 673454 699594 714913
  Show dependency treegraph
 
Reported: 2011-12-29 10:54 PST by Terrence Cole [:terrence]
Modified: 2012-02-14 08:41 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (9.85 KB, patch)
2011-12-30 13:59 PST, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v2: Removed duplicate call to Debug_SetValueRangeToCrashOnTouch. (9.91 KB, patch)
2012-01-03 11:51 PST, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v3: Use genfp instead of gen->floating (9.88 KB, patch)
2012-01-03 16:37 PST, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v4: rewritten as a template and merged with 714913 (11.46 KB, patch)
2012-01-06 10:57 PST, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v5: With review comments addressed. (12.26 KB, patch)
2012-02-08 13:12 PST, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review
v6: With manual template expansion. (12.83 KB, patch)
2012-02-10 12:02 PST, Terrence Cole [:terrence]
terrence.d.cole: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2011-12-29 10:54:37 PST
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.
Comment 1 Terrence Cole [:terrence] 2011-12-30 13:59:42 PST
Created attachment 585043 [details] [diff] [review]
v1

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.
Comment 2 Terrence Cole [:terrence] 2011-12-30 15:23:07 PST
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?
Comment 3 Terrence Cole [:terrence] 2012-01-03 11:51:09 PST
Created attachment 585489 [details] [diff] [review]
v2: Removed duplicate call to Debug_SetValueRangeToCrashOnTouch.

This is now ready for review, thanks for waiting!
Comment 4 Terrence Cole [:terrence] 2012-01-03 16:37:35 PST
Created attachment 585599 [details] [diff] [review]
v3: Use genfp instead of gen->floating

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 5 Terrence Cole [:terrence] 2012-01-05 11:35:27 PST
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.
Comment 6 Terrence Cole [:terrence] 2012-01-06 10:57:15 PST
Created attachment 586484 [details] [diff] [review]
v4: rewritten as a template and merged with 714913

This is much, much nicer.  Currently running a try build:
https://tbpl.mozilla.org/?tree=Try&rev=64017c8f2d46
Comment 7 Terrence Cole [:terrence] 2012-01-06 11:07:09 PST
*** Bug 714913 has been marked as a duplicate of this bug. ***
Comment 8 [PTO to Dec5] Bill McCloskey (:billm) 2012-02-06 12:08:55 PST
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.
Comment 9 Andrew McCreight [:mccr8] 2012-02-06 13:08:01 PST
(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.
Comment 10 [PTO to Dec5] Bill McCloskey (:billm) 2012-02-06 13:09:04 PST
(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.
Comment 11 Terrence Cole [:terrence] 2012-02-08 13:12:37 PST
Created attachment 595502 [details] [diff] [review]
v5: With review comments addressed.

Running it through try again:
https://tbpl.mozilla.org/?tree=Try&rev=27fe295675ec
Comment 12 [PTO to Dec5] Bill McCloskey (:billm) 2012-02-08 13:25:17 PST
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.
Comment 13 Terrence Cole [:terrence] 2012-02-08 16:18:57 PST
Android's gcc appears to have choked on the templates.  Re-trying with explicit instantiation:
https://tbpl.mozilla.org/?tree=Try&rev=4739efe91a1b
Comment 14 Terrence Cole [:terrence] 2012-02-09 18:25:18 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/710220a5ac33
Comment 16 Terrence Cole [:terrence] 2012-02-10 10:08:23 PST
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
Comment 17 Terrence Cole [:terrence] 2012-02-10 10:45:39 PST
Whoops, prior try push was patched with --dry-run:
https://tbpl.mozilla.org/?tree=Try&rev=4b2c7f02564a
Comment 18 Terrence Cole [:terrence] 2012-02-10 12:02:16 PST
Created attachment 596134 [details] [diff] [review]
v6: With manual template expansion.

And I've tried the wrong patch.  This will teach me to skip steps.
https://tbpl.mozilla.org/?tree=Try&rev=caafc725c109
Comment 19 Terrence Cole [:terrence] 2012-02-13 11:14:59 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f29587aa8965
Comment 20 Marco Bonardo [::mak] 2012-02-14 02:25:20 PST
https://hg.mozilla.org/mozilla-central/rev/f29587aa8965
Comment 21 Andrew McCreight [:mccr8] 2012-02-14 07:05:54 PST
Does this mean that MarkStackRangeConservatively isn't used anywhere?  MXR can't find any uses of it, except as a comment in Stack.cpp.
Comment 22 [PTO to Dec5] Bill McCloskey (:billm) 2012-02-14 08:41:58 PST
Yes, it's not used anywhere. And I forgot to fix the comment.

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