Closed Bug 966912 Opened 6 years ago Closed 6 years ago

Static scope chain should include "with" objects

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: wingo, Assigned: wingo)

References

Details

Attachments

(5 files, 3 obsolete files)

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.
Assignee: nobody → 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).
Attachment #8369360 - Flags: review?(luke)
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.
Attachment #8369361 - Flags: review?(luke)
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.
Attachment #8369362 - Flags: review?(luke)
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
Attachment #8369369 - Flags: review?(luke)
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 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
Attachment #8369360 - Flags: review?(luke) → review+
https://tbpl.mozilla.org/?tree=Try&rev=13c37c65cf24 for the revised first patch
Attachment #8369361 - Attachment is obsolete: true
Attachment #8369361 - Flags: review?(luke)
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.
Attachment #8370028 - Flags: review?(luke)
Attachment #8369369 - Attachment is obsolete: true
Attachment #8369369 - Flags: review?(luke)
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.
Attachment #8370044 - Flags: review?(luke)
Trybuild for the updated part 2 patch is green: https://tbpl.mozilla.org/?tree=Try&rev=83b91ac33bfe
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'?
Attachment #8370028 - Flags: review?(luke) → review+
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
Attachment #8369362 - Flags: review?(luke) → review+
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.
Attachment #8370044 - Flags: review?(luke) → review+
Attachment #8370028 - Attachment is obsolete: true
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.
Attachment #8370128 - Flags: review+
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
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.
Blocks: 967649
Blocks: 963879
https://hg.mozilla.org/mozilla-central/rev/c717600bee44
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Only the first part has landed so far :) (Whiteboard didn't have [leave-open])
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Flags: in-testsuite+
Attachment #8370733 - Flags: review?(jdemooij)
Attachment #8370733 - Flags: review?(jdemooij) → review+
You need to log in before you can comment on or make changes to this bug.