790836 - Don't mark IonJSFrameLayout twice

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

When marking the stack conservatively, we skip regions that are owned by Ion: it marks its stack exactly at a different time.  When copying these exclusions into the rooting analysis -- we were incorrectly poisoning IonMonkey's stack -- Sean noticed that the top we are using for the exclusion does not include the IonJSFrameLayout.  This causes us to mark any objects in the Layout twice.  This is perfectly fine when marking, but messes up the rooting analysis.

Comment 1

[Nicolas B. Pierron [:nbp]]

Comment on attachment 660650 [details] [diff] [review]
v0

Review of attachment 660650 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch, still not correctly fixed yet.

::: js/src/jsgc.cpp
@@ +1173,5 @@
>          ion::IonFrameIterator frames(ion.top());
>          while (!frames.done())
>              ++frames;
>  
>          uintptr_t *ionMin = (uintptr_t *)ion.top();

We recently added a footer to the exit frame.  Everything in the footer should be marked precisely.

    ion::IonFrameIterator frames(ion.top());
    uintptr_t *ionMin = (uintptr_t *) frame.exitFrame()->ionStackBegin();
    while (!frames.done())
        ++frames;

This new function should return « frame.exitFrame()->footer() » when the exit frame is not a VMWrapper or if it does not have an out param. otherwise it should return « frame.exitFrame()->footer()->outVp() ».  Have a look at MarkIonExitFrame (from ionFrames.cpp) to make sure you are doing it right.

@@ +1174,5 @@
>          while (!frames.done())
>              ++frames;
>  
>          uintptr_t *ionMin = (uintptr_t *)ion.top();
> +        uintptr_t *ionEnd = (uintptr_t *)frames.fp() + sizeof(ion::IonJSFrameLayout);

This is still wrong and won't change a thing except that you won't mark the script but you will still mark the arguments of the JS function twice.  Use the following instead:

    uintptr_t *ionEnd = (uintptr_t *) frames.prevFp();

You will have to remove/specialize the assertion inside prevFp such as you can use it here.  It should be safe to remove it now.

@@ +4969,5 @@
> +        while (!frames.done())
> +            ++frames;
> +
> +        uintptr_t *ionMin = (uintptr_t *)ion.top();
> +        uintptr_t *ionEnd = (uintptr_t *)frames.fp() + sizeof(ion::IonJSFrameLayout);

idem.  You might want to promote this computation to a method of IonActivationFrameIterator, such as

    IonActivationFrameIterator::stackRange(uintptr_t &start, uintptr_t &end) const;

and add it near the marking functions, such as we can relate to it.

Comment 2

[Terrence Cole [:terrence]]

Attachment: v1

With fixes applied.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 661044 [details] [diff] [review]
v1

Review of attachment 661044 [details] [diff] [review]:
-----------------------------------------------------------------

Apply nits, and r=me.

::: js/src/ion/IonFrames.cpp
@@ +491,5 @@
>  #endif
>  }
>  
> +void
> +IonFrameIterator::ionStackRange(uintptr_t *&min, uintptr_t *&end)

nit: move it to the IonActivationIterator, otherwise the meaning would be the stack range of one Ion frame.

@@ +495,5 @@
> +IonFrameIterator::ionStackRange(uintptr_t *&min, uintptr_t *&end)
> +{
> +    IonExitFooterFrame *footer = exitFrame()->footer();
> +    const VMFunction *f = footer->function();
> +    if (f == NULL || f->outParam != Type_Handle)

nit: if (exitFrame()->nativeExit() || exitFrame()->DOMExit() || f->outParam != Type_Handle)

Comment 4

[Terrence Cole [:terrence]]

Attachment: v2

Hurm.  |if (exitFrame()->nativeExit() || exitFrame()->DOMExit() || f->outParam != Type_Handle)| trips all the assertions.  After discussing it with Sean, I think we don't need to test the frame type, just whether vp exists.  We always want to skip it, regardless of frame type.  What we came up with is:

    if (f && f->outParam == Type_Handle)
        min = reinterpret_cast<uintptr_t *>(footer->outVp());
    else
        min = reinterpret_cast<uintptr_t *>(footer);

Do you concur?

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Terrence Cole [:terrence] from comment #4)
> Created attachment 661412 [details] [diff] [review]
> v2
> 
> Hurm.  |if (exitFrame()->nativeExit() || exitFrame()->DOMExit() ||
> f->outParam != Type_Handle)| trips all the assertions.  After discussing it
> with Sean, I think we don't need to test the frame type, just whether vp
> exists.  We always want to skip it, regardless of frame type.  What we came
> up with is:
> 
>     if (f && f->outParam == Type_Handle)
>         min = reinterpret_cast<uintptr_t *>(footer->outVp());
>     else
>         min = reinterpret_cast<uintptr_t *>(footer);
> 
> Do you concur?

I agree this would work, but this is likely to be missed by later modifications of the footer unless you can add a comment mentioning this location if this really needs to be optimized.

the best would be:

if (exitFrame()->wrapperExit() && …)

where wrapperExit() is an inlined function which coerces f to a boolean.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 661412 [details] [diff] [review]
v2

Review of attachment 661412 [details] [diff] [review]:
-----------------------------------------------------------------

See previous comment, apply if possible and r=me.

Comment 7

[Terrence Cole [:terrence]]

Try run at:
https://tbpl.mozilla.org/?tree=Try&rev=37b029313684

Checked in at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3bf8035c495

Comment 8

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/a3bf8035c495