The default bug view has changed. See this FAQ.

Add missing post write barriers on Generators

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #585043 - Flags: review?(wmccloskey)
(Assignee)

Comment 2

5 years ago
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?
(Assignee)

Comment 3

5 years ago
Created attachment 585489 [details] [diff] [review]
v2: Removed duplicate call to Debug_SetValueRangeToCrashOnTouch.

This is now ready for review, thanks for waiting!
Attachment #585043 - Attachment is obsolete: true
Attachment #585043 - Flags: review?(wmccloskey)
Attachment #585489 - Flags: review?(wmccloskey)
(Assignee)

Updated

5 years ago
Blocks: 714913
(Assignee)

Comment 4

5 years ago
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.
Attachment #585489 - Attachment is obsolete: true
Attachment #585489 - Flags: review?(wmccloskey)
Attachment #585599 - Flags: review?(wmccloskey)
(Assignee)

Comment 5

5 years ago
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.
Attachment #585599 - Flags: review?(wmccloskey)
(Assignee)

Updated

5 years ago
Depends on: 715943
(Assignee)

Comment 6

5 years ago
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
Attachment #585599 - Attachment is obsolete: true
Attachment #586484 - Flags: review?(wmccloskey)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 714913
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.
Attachment #586484 - Flags: review?(wmccloskey)
(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.
Blocks: 699594
(Assignee)

Comment 11

5 years ago
Created attachment 595502 [details] [diff] [review]
v5: With review comments addressed.

Running it through try again:
https://tbpl.mozilla.org/?tree=Try&rev=27fe295675ec
Attachment #586484 - Attachment is obsolete: true
Attachment #595502 - Flags: review?(wmccloskey)
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.
Attachment #595502 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 13

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

Comment 14

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/710220a5ac33
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.
(Assignee)

Comment 16

5 years ago
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
(Assignee)

Comment 17

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

Comment 18

5 years ago
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
Attachment #595502 - Attachment is obsolete: true
Attachment #596134 - Flags: review+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f29587aa8965
https://hg.mozilla.org/mozilla-central/rev/f29587aa8965
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
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.
You need to log in before you can comment on or make changes to this bug.