966912 - Static scope chain should include "with" objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andy Wingo [:wingo]]

We would like to make it so that let scopes can be stored in the fixed part of a stack frame (bug 962599).  However the assumption that a scope pushes onto the operand stack is embedded in some parts of the runtime, particularly those parts that need to unwind the stack for a nonlocal exit.  This situation would be made more clear if with objects were present on the static scope chain (which currently only consists of StaticBlockObject instances).  This patch-set will rename WithObject to DynamicWithObject, and add a new StaticWithObject.  DynamicWithObject is to StaticWithObject as ClonedBlockObject is to StaticBlockObject.  In the future of course it may make sense to give the static and dynamic objects significantly different representations.  StaticWithObject extents will be added to the "block scope note" tree, as static block objects are now.

Comment 1

[Andy Wingo [:wingo]]

Attachment: Part 1: Rename blockChain to staticScope

Comment 2

[Andy Wingo [:wingo]]

Attachment: Part 2: Add StaticWithObject to the static scope chain

Comment 3

[Andy Wingo [:wingo]]

Attachment: Part 3: UnwindScope uses static scope chain, not stack depth

Comment 4

[Andy Wingo [:wingo]]

https://tbpl.mozilla.org/?tree=Try&rev=da4b047b1f51

Comment 5

[Andy Wingo [:wingo]]

Comment on attachment 8369360 [details] [diff] [review]
Part 1: Rename blockChain to staticScope

This first patch is big but fairly mechanical; it finds places that are named blockChain() and converts them to be staticScope() and to return a NestedScopeObject instead of a StaticBlockObject (if needed).

Comment 6

[Andy Wingo [:wingo]]

Comment on attachment 8369361 [details] [diff] [review]
Part 2: Add StaticWithObject to the static scope chain

This one is gnarly.  It splits StaticWithObject from DynamicWithObject, adapts the WithStatement parse node to have an objbox, adapts the emitter to generate block scope notes for with scopes, and adapts the scope iters to expect StaticWithObject instances on the static scope chain.

Comment 7

[Andy Wingo [:wingo]]

Comment on attachment 8369362 [details] [diff] [review]
Part 3: UnwindScope uses static scope chain, not stack depth

This one is straightforward.  When unwinding the stack, we unwind to the scope that was live at a particular program counter instead of to the stack depth that was observed/calculated.  This is possible since all static scopes are on the scope chain.

Comment 8

[Andy Wingo [:wingo]]

Attachment: Part 4: Entering a with statement doesn't push onto the stack

Comment 9

[Andy Wingo [:wingo]]

Comment on attachment 8369369 [details] [diff] [review]
Part 4: Entering a with statement doesn't push onto the stack

While we're at it, we might as well make enterwith/leavewith less trashy

Comment 10

[Andy Wingo [:wingo]]

https://tbpl.mozilla.org/?tree=Try&rev=6fff09ded85f fixes a GCC debug-mode typo, adds attachment 8369369 [details] [diff] [review], just for debug-mode linux64.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8369360 [details] [diff] [review]
Part 1: Rename blockChain to staticScope

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

::: js/src/jsscript.cpp
@@ +780,5 @@
>              /* Code the nested block's enclosing scope. */
>              uint32_t blockEnclosingScopeIndex = 0;
>              if (mode == XDR_ENCODE) {
> +                NestedScopeObject *scope;
> +                if ((scope = (*objp)->as<NestedScopeObject>().enclosingNestedScope()))

Although sometimes it's the best thing, we try to avoid assignments as conditional expressions.  How about:

NestedScopeObject &scope = (*objp)->as<NestedScopeObject>();
if (NestedScopeObject *enclosing = scope.enclosingNestedScope())
    ...

::: js/src/vm/ScopeObject.cpp
@@ +999,5 @@
>          frame_ = NullFramePtr();
>      } else if (cur_->is<WithObject>()) {
>          JS_ASSERT_IF(frame_.isFunctionFrame(), frame_.fun()->isHeavyweight());
> +        JS_ASSERT_IF(staticScope_, staticScope_->is<StaticBlockObject>());
> +        JS_ASSERT_IF(staticScope_, staticScope_->as<StaticBlockObject>().needsClone());

as<StaticBlockObject>() will assert is<StaticBlockObject>(), so seems fine to leave it out here.

@@ +1007,4 @@
>          type_ = With;
>          hasScopeObject_ = true;
> +    } else if (staticScope_) {
> +        JS_ASSERT(staticScope_->is<StaticBlockObject>());

ditto

Comment 12

[Andy Wingo [:wingo]]

https://tbpl.mozilla.org/?tree=Try&rev=13c37c65cf24 for the revised first patch

Comment 13

[Andy Wingo [:wingo]]

Part 1 checked in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c717600bee44

Comment 14

[Andy Wingo [:wingo]]

Attachment: Part 2: Add StaticWithObject to the static scope chain

Comment 15

[Andy Wingo [:wingo]]

Comment on attachment 8370028 [details] [diff] [review]
Part 2: Add StaticWithObject to the static scope chain

Updated to fold in some XDR changes from the last patch and to apply cleanly after the changes to patch 1.

Comment 16

[Andy Wingo [:wingo]]

Attachment: Part 4: Entering a with statement doesn't push onto the stack

Comment 17

[Andy Wingo [:wingo]]

Comment on attachment 8370044 [details] [diff] [review]
Part 4: Entering a with statement doesn't push onto the stack

Updated Part 4 after incorporation of some of its pieces into Part 2.

Comment 18

[Andy Wingo [:wingo]]

Trybuild for the updated part 2 patch is green: https://tbpl.mozilla.org/?tree=Try&rev=83b91ac33bfe

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 8370028 [details] [diff] [review]
Part 2: Add StaticWithObject to the static scope chain

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

Very nice!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +824,5 @@
> +// Like EnterBlockScope, entering a with scope adds a template object to the
> +// static scope chain.  It's sufficiently different that we just duplicate the
> +// functionality here.
> +static bool
> +EnterWithScope(ExclusiveContext *cx, BytecodeEmitter *bce, StmtInfoBCE *stmt, ObjectBox *objbox)

It seems like that, if you shuffled some of the block/with-specific parts around, you could factor out most of the code in EnterWithScope/EnterBlockScope into a new EnterNestedScope.

@@ +907,5 @@
>      return true;
>  }
>  
>  static bool
> +LeaveWithScope(ExclusiveContext *cx, BytecodeEmitter *bce)

Same comment here about factoring out a LeaveNestedScope.

@@ +1188,5 @@
>              while (!b->containsVarAtDepth(depth)) {
>                  if (b->needsClone())
>                      skippedScopes++;
> +                NestedScopeObject *outer = b->enclosingNestedScope();
> +                JS_ASSERT(outer && outer->is<StaticBlockObject>());

The as<StaticBlockObject> will do the assertion (and always crash if null), so it seems fine to remove the assertion.

::: js/src/frontend/SharedContext.h
@@ +483,5 @@
>      if (stmt->linksScope()) {
>          ct->topScopeStmt = stmt->downScope;
> +        if (stmt->isNestedScope) {
> +            JS_ASSERT(stmt->staticScope);
> +            ct->staticScope = stmt->staticScope->enclosingNestedScope();

Again, leaving out null assert seems fine.

::: js/src/vm/Debugger.cpp
@@ +5545,5 @@
>  static bool
>  IsWith(Env *env)
>  {
> +    return env->is<DebugScopeObject>() &&
> +        env->as<DebugScopeObject>().scope().is<DynamicWithObject>();

Can you align 'env->is' with 'env->as'?

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 8369362 [details] [diff] [review]
Part 3: UnwindScope uses static scope chain, not stack depth

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

Beautiful

Comment 21

[Luke Wagner [:luke]]

Comment on attachment 8370044 [details] [diff] [review]
Part 4: Entering a with statement doesn't push onto the stack

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

::: js/src/vm/ScopeObject.h
@@ +382,5 @@
>  
>      static const Class class_;
>  
>      static DynamicWithObject *
> +    create(JSContext *cx, HandleObject object, HandleObject enclosing, HandleObject staticWith);

Can you remove DEPTH_SLOT and shrink RESERVED_SLOTS?  Also, ISTR from the earlier patch that the StaticWithObject also had a DEPTH_SLOT.

Comment 22

[Andy Wingo [:wingo]]

Attachment: Part 2: Add StaticWithObject to the static scope chain

Comment 23

[Andy Wingo [:wingo]]

Comment on attachment 8370128 [details] [diff] [review]
Part 2: Add StaticWithObject to the static scope chain

Thanks for the review.  I updated the patch, refactoring an EnterNestedScope/LeaveNestedScope; will get better once bug 962599 lands and entering block scopes can use the same interface.

Comment 24

[Andy Wingo [:wingo]]

Comment on attachment 8370128 [details] [diff] [review]
Part 2: Add StaticWithObject to the static scope chain

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +759,5 @@
> +        break;
> +      default:
> +        MOZ_ASSUME_UNREACHABLE();
> +    }
> +    

will fix this whitespace before pushing

Comment 25

[Andy Wingo [:wingo]]

Seems I forgot to save a couple of things before updating Part 2, namely the parts where I remove unneeded assertions.  Sorry for the noise, they are included locally.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Emanuel Hoogeveen [:ehoogeveen]]

Only the first part has landed so far :) (Whiteboard didn't have [leave-open])

Comment 28

[Andy Wingo [:wingo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92a2cc62f2f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe176961eae
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6b668f2388

Sorry about the leave-open thing; I looked for it, but it seems I looked in keywords and not in whiteboard.  Now it is indeed fully applied and free to close.

Comment 29

[Andy Wingo [:wingo]]

Attachment: Allocate StaticWithObject into oldspace

Comment 30

[Andy Wingo [:wingo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/963f1738eaa5

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92a2cc62f2f9
https://hg.mozilla.org/mozilla-central/rev/3fe176961eae
https://hg.mozilla.org/mozilla-central/rev/eb6b668f2388
https://hg.mozilla.org/mozilla-central/rev/963f1738eaa5