Bug 1467052 - Use BindingName tag to distinguish between var and top-level function, instead of offset range. r=shu
authorTooru Fujisawa <arai_a@mac.com>
Fri, 08 Jun 2018 10:06:13 +0900
changeset 478636 3dee28e17417eb3fcb043027393abc7b4f506d78
parent 478635 22daf991f7b705c6849c5597f9794f1ac8f86a8e
child 478637 5b08fd277ce5f750fbec6aaa239f3c7c9806e22a
push id1757
push userffxbld-merge
push dateFri, 24 Aug 2018 17:02:43 +0000
treeherdermozilla-release@736023aebdb1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersshu
bugs1467052
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1467052 - Use BindingName tag to distinguish between var and top-level function, instead of offset range. r=shu
js/src/frontend/Parser.cpp
js/src/vm/Scope.cpp
js/src/vm/Scope.h
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -1783,63 +1783,61 @@ FreshlyInitializeBindings(BindingName* c
     for (const BindingName& binding : bindings)
         new (cursor++) BindingName(binding);
     return cursor;
 }
 
 Maybe<GlobalScope::Data*>
 NewGlobalScopeData(JSContext* context, ParseContext::Scope& scope, LifoAlloc& alloc, ParseContext* pc)
 {
-
-    Vector<BindingName> funs(context);
     Vector<BindingName> vars(context);
     Vector<BindingName> lets(context);
     Vector<BindingName> consts(context);
 
     bool allBindingsClosedOver = pc->sc()->allBindingsClosedOver();
     for (BindingIter bi = scope.bindings(pc); bi; bi++) {
-        BindingName binding(bi.name(), allBindingsClosedOver || bi.closedOver());
+        bool closedOver = allBindingsClosedOver || bi.closedOver();
+
         switch (bi.kind()) {
-          case BindingKind::Var:
-            if (bi.declarationKind() == DeclarationKind::BodyLevelFunction) {
-                if (!funs.append(binding))
-                    return Nothing();
-            } else {
-                if (!vars.append(binding))
-                    return Nothing();
-            }
+          case BindingKind::Var: {
+            bool isTopLevelFunction = bi.declarationKind() == DeclarationKind::BodyLevelFunction;
+            BindingName binding(bi.name(), closedOver, isTopLevelFunction);
+            if (!vars.append(binding))
+                return Nothing();
             break;
-          case BindingKind::Let:
+          }
+          case BindingKind::Let: {
+            BindingName binding(bi.name(), closedOver);
             if (!lets.append(binding))
                 return Nothing();
             break;
-          case BindingKind::Const:
+          }
+          case BindingKind::Const: {
+            BindingName binding(bi.name(), closedOver);
             if (!consts.append(binding))
                 return Nothing();
             break;
+          }
           default:
             MOZ_CRASH("Bad global scope BindingKind");
         }
     }
 
     GlobalScope::Data* bindings = nullptr;
-    uint32_t numBindings = funs.length() + vars.length() + lets.length() + consts.length();
+    uint32_t numBindings = vars.length() + lets.length() + consts.length();
 
     if (numBindings > 0) {
         bindings = NewEmptyBindingData<GlobalScope>(context, alloc, numBindings);
         if (!bindings)
             return Nothing();
 
         // The ordering here is important. See comments in GlobalScope.
         BindingName* start = bindings->trailingNames.start();
         BindingName* cursor = start;
 
-        cursor = FreshlyInitializeBindings(cursor, funs);
-
-        bindings->varStart = cursor - start;
         cursor = FreshlyInitializeBindings(cursor, vars);
 
         bindings->letStart = cursor - start;
         cursor = FreshlyInitializeBindings(cursor, lets);
 
         bindings->constStart = cursor - start;
         cursor = FreshlyInitializeBindings(cursor, consts);
 
@@ -1923,49 +1921,39 @@ Maybe<ModuleScope::Data*>
 ParserBase::newModuleScopeData(ParseContext::Scope& scope)
 {
     return NewModuleScopeData(context, scope, alloc, pc);
 }
 
 Maybe<EvalScope::Data*>
 NewEvalScopeData(JSContext* context, ParseContext::Scope& scope, LifoAlloc& alloc, ParseContext* pc)
 {
-    Vector<BindingName> funs(context);
     Vector<BindingName> vars(context);
 
     for (BindingIter bi = scope.bindings(pc); bi; bi++) {
         // Eval scopes only contain 'var' bindings. Make all bindings aliased
         // for now.
         MOZ_ASSERT(bi.kind() == BindingKind::Var);
-        BindingName binding(bi.name(), true);
-        if (bi.declarationKind() == DeclarationKind::BodyLevelFunction) {
-            if (!funs.append(binding))
-                return Nothing();
-        } else {
-            if (!vars.append(binding))
-                return Nothing();
-        }
+        bool isTopLevelFunction = bi.declarationKind() == DeclarationKind::BodyLevelFunction;
+        BindingName binding(bi.name(), true, isTopLevelFunction);
+        if (!vars.append(binding))
+            return Nothing();
     }
 
     EvalScope::Data* bindings = nullptr;
-    uint32_t numBindings = funs.length() + vars.length();
+    uint32_t numBindings = vars.length();
 
     if (numBindings > 0) {
         bindings = NewEmptyBindingData<EvalScope>(context, alloc, numBindings);
         if (!bindings)
             return Nothing();
 
         BindingName* start = bindings->trailingNames.start();
         BindingName* cursor = start;
 
-        // Keep track of what vars are functions. This is only used in BCE to omit
-        // superfluous DEFVARs.
-        cursor = FreshlyInitializeBindings(cursor, funs);
-
-        bindings->varStart = cursor - start;
         cursor = FreshlyInitializeBindings(cursor, vars);
 
         bindings->length = numBindings;
     }
 
     return Some(bindings);
 }
 
--- a/js/src/vm/Scope.cpp
+++ b/js/src/vm/Scope.cpp
@@ -988,23 +988,21 @@ GlobalScope::XDR(XDRState<mode>* xdr, Sc
     Rooted<Data*> data(cx);
     MOZ_TRY(XDRSizedBindingNames<GlobalScope>(xdr, scope.as<GlobalScope>(), &data));
 
     {
         Maybe<Rooted<UniquePtr<Data>>> uniqueData;
         if (mode == XDR_DECODE)
             uniqueData.emplace(cx, data);
 
-        MOZ_TRY(xdr->codeUint32(&data->varStart));
         MOZ_TRY(xdr->codeUint32(&data->letStart));
         MOZ_TRY(xdr->codeUint32(&data->constStart));
 
         if (mode == XDR_DECODE) {
             if (!data->length) {
-                MOZ_ASSERT(!data->varStart);
                 MOZ_ASSERT(!data->letStart);
                 MOZ_ASSERT(!data->constStart);
             }
 
             scope.set(createWithData(cx, kind, &uniqueData.ref()));
             if (!scope)
                 return xdr->fail(JS::TranscodeResult_Throw);
         }
@@ -1413,82 +1411,78 @@ BindingIter::BindingIter(JSScript* scrip
 void
 BindingIter::init(LexicalScope::Data& data, uint32_t firstFrameSlot, uint8_t flags)
 {
     // Named lambda scopes can only have environment slots. If the callee
     // isn't closed over, it is accessed via JSOP_CALLEE.
     if (flags & IsNamedLambda) {
         // Named lambda binding is weird. Normal BindingKind ordering rules
         // don't apply.
-        init(0, 0, 0, 0, 0, 0,
+        init(0, 0, 0, 0, 0,
              CanHaveEnvironmentSlots | flags,
              firstFrameSlot, JSSLOT_FREE(&LexicalEnvironmentObject::class_),
              data.trailingNames.start(), data.length);
     } else {
         //            imports - [0, 0)
         // positional formals - [0, 0)
         //      other formals - [0, 0)
-        //    top-level funcs - [0, 0)
         //               vars - [0, 0)
         //               lets - [0, data.constStart)
         //             consts - [data.constStart, data.length)
-        init(0, 0, 0, 0, 0, data.constStart,
+        init(0, 0, 0, 0, data.constStart,
              CanHaveFrameSlots | CanHaveEnvironmentSlots | flags,
              firstFrameSlot, JSSLOT_FREE(&LexicalEnvironmentObject::class_),
              data.trailingNames.start(), data.length);
     }
 }
 
 void
 BindingIter::init(FunctionScope::Data& data, uint8_t flags)
 {
     flags = CanHaveFrameSlots | CanHaveEnvironmentSlots | flags;
     if (!(flags & HasFormalParameterExprs))
         flags |= CanHaveArgumentSlots;
 
     //            imports - [0, 0)
     // positional formals - [0, data.nonPositionalFormalStart)
     //      other formals - [data.nonPositionalParamStart, data.varStart)
-    //    top-level funcs - [data.varStart, data.varStart)
     //               vars - [data.varStart, data.length)
     //               lets - [data.length, data.length)
     //             consts - [data.length, data.length)
-    init(0, data.nonPositionalFormalStart, data.varStart, data.varStart, data.length, data.length,
+    init(0, data.nonPositionalFormalStart, data.varStart, data.length, data.length,
          flags,
          0, JSSLOT_FREE(&CallObject::class_),
          data.trailingNames.start(), data.length);
 }
 
 void
 BindingIter::init(VarScope::Data& data, uint32_t firstFrameSlot)
 {
     //            imports - [0, 0)
     // positional formals - [0, 0)
     //      other formals - [0, 0)
-    //    top-level funcs - [0, 0)
     //               vars - [0, data.length)
     //               lets - [data.length, data.length)
     //             consts - [data.length, data.length)
-    init(0, 0, 0, 0, data.length, data.length,
+    init(0, 0, 0, data.length, data.length,
          CanHaveFrameSlots | CanHaveEnvironmentSlots,
          firstFrameSlot, JSSLOT_FREE(&VarEnvironmentObject::class_),
          data.trailingNames.start(), data.length);
 }
 
 void
 BindingIter::init(GlobalScope::Data& data)
 {
     //            imports - [0, 0)
     // positional formals - [0, 0)
     //      other formals - [0, 0)
-    //    top-level funcs - [0, data.varStart)
-    //               vars - [data.varStart, data.letStart)
+    //               vars - [0, data.letStart)
     //               lets - [data.letStart, data.constStart)
     //             consts - [data.constStart, data.length)
-    init(0, 0, 0, data.varStart, data.letStart, data.constStart,
+    init(0, 0, 0, data.letStart, data.constStart,
          CannotHaveSlots,
          UINT32_MAX, UINT32_MAX,
          data.trailingNames.start(), data.length);
 }
 
 void
 BindingIter::init(EvalScope::Data& data, bool strict)
 {
@@ -1503,68 +1497,64 @@ BindingIter::init(EvalScope::Data& data,
         flags = CannotHaveSlots;
         firstFrameSlot = UINT32_MAX;
         firstEnvironmentSlot = UINT32_MAX;
     }
 
     //            imports - [0, 0)
     // positional formals - [0, 0)
     //      other formals - [0, 0)
-    //    top-level funcs - [0, data.varStart)
-    //               vars - [data.varStart, data.length)
+    //               vars - [0, data.length)
     //               lets - [data.length, data.length)
     //             consts - [data.length, data.length)
-    init(0, 0, 0, data.varStart, data.length, data.length,
+    init(0, 0, 0, data.length, data.length,
          flags, firstFrameSlot, firstEnvironmentSlot,
          data.trailingNames.start(), data.length);
 }
 
 void
 BindingIter::init(ModuleScope::Data& data)
 {
     //            imports - [0, data.varStart)
     // positional formals - [data.varStart, data.varStart)
     //      other formals - [data.varStart, data.varStart)
-    //    top-level funcs - [data.varStart, data.varStart)
     //               vars - [data.varStart, data.letStart)
     //               lets - [data.letStart, data.constStart)
     //             consts - [data.constStart, data.length)
-    init(data.varStart, data.varStart, data.varStart, data.varStart, data.letStart, data.constStart,
+    init(data.varStart, data.varStart, data.varStart, data.letStart, data.constStart,
          CanHaveFrameSlots | CanHaveEnvironmentSlots,
          0, JSSLOT_FREE(&ModuleEnvironmentObject::class_),
          data.trailingNames.start(), data.length);
 }
 
 void
 BindingIter::init(WasmInstanceScope::Data& data)
 {
     //            imports - [0, 0)
     // positional formals - [0, 0)
     //      other formals - [0, 0)
-    //    top-level funcs - [0, 0)
     //               vars - [0, data.length)
     //               lets - [data.length, data.length)
     //             consts - [data.length, data.length)
-    init(0, 0, 0, 0, data.length, data.length,
+    init(0, 0, 0, data.length, data.length,
          CanHaveFrameSlots | CanHaveEnvironmentSlots,
          UINT32_MAX, UINT32_MAX,
          data.trailingNames.start(), data.length);
 }
 
 void
 BindingIter::init(WasmFunctionScope::Data& data)
 {
     //            imports - [0, 0)
     // positional formals - [0, 0)
     //      other formals - [0, 0)
-    //    top-level funcs - [0, 0)
     //               vars - [0, data.length)
     //               lets - [data.length, data.length)
     //             consts - [data.length, data.length)
-    init(0, 0, 0, 0, data.length, data.length,
+    init(0, 0, 0, data.length, data.length,
          CanHaveFrameSlots | CanHaveEnvironmentSlots,
          UINT32_MAX, UINT32_MAX,
          data.trailingNames.start(), data.length);
 }
 
 PositionalFormalParameterIter::PositionalFormalParameterIter(JSScript* script)
   : BindingIter(script)
 {
--- a/js/src/vm/Scope.h
+++ b/js/src/vm/Scope.h
@@ -99,40 +99,56 @@ ScopeKindIsInBody(ScopeKind kind)
            kind == ScopeKind::ParameterExpressionVar;
 }
 
 const char* BindingKindString(BindingKind kind);
 const char* ScopeKindString(ScopeKind kind);
 
 class BindingName
 {
-    // A JSAtom* with its low bit used as a tag for whether it is closed over
-    // (i.e., exists in the environment shape).
+    // A JSAtom* with its low bit used as a tag for the:
+    //  * whether it is closed over (i.e., exists in the environment shape)
+    //  * whether it is a top-level function binding in global or eval scope,
+    //    instead of var binding (both are in the same range in Scope data)
     uintptr_t bits_;
 
     static const uintptr_t ClosedOverFlag = 0x1;
-    static const uintptr_t FlagMask = 0x1;
+    // TODO: We should reuse this bit for let vs class distinction to
+    //       show the better redeclaration error message (bug 1428672).
+    static const uintptr_t TopLevelFunctionFlag = 0x2;
+    static const uintptr_t FlagMask = 0x3;
 
   public:
     BindingName()
       : bits_(0)
     { }
 
-    BindingName(JSAtom* name, bool closedOver)
-      : bits_(uintptr_t(name) | (closedOver ? ClosedOverFlag : 0x0))
+    BindingName(JSAtom* name, bool closedOver, bool isTopLevelFunction = false)
+      : bits_(uintptr_t(name) |
+              (closedOver ? ClosedOverFlag : 0x0) |
+              (isTopLevelFunction? TopLevelFunctionFlag : 0x0))
     { }
 
     JSAtom* name() const {
         return reinterpret_cast<JSAtom*>(bits_ & ~FlagMask);
     }
 
     bool closedOver() const {
         return bits_ & ClosedOverFlag;
     }
 
+  private:
+    friend class BindingIter;
+    // This method should be called only for binding names in `vars` range in
+    // BindingIter.
+    bool isTopLevelFunction() const {
+        return bits_ & TopLevelFunctionFlag;
+    }
+
+  public:
     void trace(JSTracer* trc);
 };
 
 /**
  * The various {Global,Module,...}Scope::Data classes consist of always-present
  * bits, then a trailing array of BindingNames.  The various Data classes all
  * end in a TrailingNamesArray that contains sized/aligned space for *one*
  * BindingName.  Data instances that contain N BindingNames, are then allocated
@@ -742,22 +758,22 @@ class GlobalScope : public Scope
     friend class BindingIter;
 
   public:
     // Data is public because it is created by the frontend. See
     // Parser<FullParseHandler>::newGlobalScopeData.
     struct Data
     {
         // Bindings are sorted by kind.
+        // `vars` includes top-level functions which is distinguished by a bit
+        // on the BindingName.
         //
-        // top-level funcs - [0, varStart)
-        //            vars - [varStart, letStart)
+        //            vars - [0, letStart)
         //            lets - [letStart, constStart)
         //          consts - [constStart, length)
-        uint32_t varStart = 0;
         uint32_t letStart = 0;
         uint32_t constStart = 0;
         uint32_t length = 0;
 
         // The array of tagged JSAtom* names, allocated beyond the end of the
         // struct.
         TrailingNamesArray trailingNames;
 
@@ -848,21 +864,21 @@ class EvalScope : public Scope
 
   public:
     // Data is public because it is created by the frontend. See
     // Parser<FullParseHandler>::newEvalScopeData.
     struct Data
     {
         // All bindings in an eval script are 'var' bindings. The implicit
         // lexical scope around the eval is present regardless of strictness
-        // and is its own LexicalScope. However, we need to track top-level
-        // functions specially for redeclaration checks.
+        // and is its own LexicalScope.
+        // `vars` includes top-level functions which is distinguished by a bit
+        // on the BindingName.
         //
-        // top-level funcs - [0, varStart)
-        //            vars - [varStart, length)
+        //            vars - [0, length)
         uint32_t varStart = 0;
         uint32_t length = 0;
 
         // Frame slots [0, nextFrameSlot) are live when this is the innermost
         // scope.
         uint32_t nextFrameSlot = 0;
 
         // The array of tagged JSAtom* names, allocated beyond the end of the
@@ -1147,44 +1163,40 @@ class BindingIter
   protected:
     // Bindings are sorted by kind. Because different Scopes have differently
     // laid out Data for packing, BindingIter must handle all binding kinds.
     //
     // Kind ranges:
     //
     //            imports - [0, positionalFormalStart)
     // positional formals - [positionalFormalStart, nonPositionalFormalStart)
-    //      other formals - [nonPositionalParamStart, topLevelFunctionStart)
-    //    top-level funcs - [topLevelFunctionStart, varStart)
+    //      other formals - [nonPositionalParamStart, varStart)
     //               vars - [varStart, letStart)
     //               lets - [letStart, constStart)
     //             consts - [constStart, length)
     //
     // Access method when not closed over:
     //
     //            imports - name
     // positional formals - argument slot
     //      other formals - frame slot
-    //    top-level funcs - frame slot
     //               vars - frame slot
     //               lets - frame slot
     //             consts - frame slot
     //
     // Access method when closed over:
     //
     //            imports - name
     // positional formals - environment slot or name
     //      other formals - environment slot or name
-    //    top-level funcs - environment slot or name
     //               vars - environment slot or name
     //               lets - environment slot or name
     //             consts - environment slot or name
     MOZ_INIT_OUTSIDE_CTOR uint32_t positionalFormalStart_;
     MOZ_INIT_OUTSIDE_CTOR uint32_t nonPositionalFormalStart_;
-    MOZ_INIT_OUTSIDE_CTOR uint32_t topLevelFunctionStart_;
     MOZ_INIT_OUTSIDE_CTOR uint32_t varStart_;
     MOZ_INIT_OUTSIDE_CTOR uint32_t letStart_;
     MOZ_INIT_OUTSIDE_CTOR uint32_t constStart_;
     MOZ_INIT_OUTSIDE_CTOR uint32_t length_;
 
     MOZ_INIT_OUTSIDE_CTOR uint32_t index_;
 
     enum Flags : uint8_t {
@@ -1206,24 +1218,22 @@ class BindingIter
     MOZ_INIT_OUTSIDE_CTOR uint8_t flags_;
     MOZ_INIT_OUTSIDE_CTOR uint16_t argumentSlot_;
     MOZ_INIT_OUTSIDE_CTOR uint32_t frameSlot_;
     MOZ_INIT_OUTSIDE_CTOR uint32_t environmentSlot_;
 
     MOZ_INIT_OUTSIDE_CTOR BindingName* names_;
 
     void init(uint32_t positionalFormalStart, uint32_t nonPositionalFormalStart,
-              uint32_t topLevelFunctionStart, uint32_t varStart,
-              uint32_t letStart, uint32_t constStart,
+              uint32_t varStart, uint32_t letStart, uint32_t constStart,
               uint8_t flags, uint32_t firstFrameSlot, uint32_t firstEnvironmentSlot,
               BindingName* names, uint32_t length)
     {
         positionalFormalStart_ = positionalFormalStart;
         nonPositionalFormalStart_ = nonPositionalFormalStart;
-        topLevelFunctionStart_ = topLevelFunctionStart;
         varStart_ = varStart;
         letStart_ = letStart;
         constStart_ = constStart;
         length_ = length;
         index_ = 0;
         flags_ = flags;
         argumentSlot_ = 0;
         frameSlot_ = firstFrameSlot;
@@ -1380,17 +1390,17 @@ class BindingIter
         MOZ_ASSERT(isNamedLambda());
         return BindingLocation::NamedLambdaCallee();
     }
 
     BindingKind kind() const {
         MOZ_ASSERT(!done());
         if (index_ < positionalFormalStart_)
             return BindingKind::Import;
-        if (index_ < topLevelFunctionStart_) {
+        if (index_ < varStart_) {
             // When the parameter list has expressions, the parameters act
             // like lexical bindings and have TDZ.
             if (hasFormalParameterExprs())
                 return BindingKind::Let;
             return BindingKind::FormalParameter;
         }
         if (index_ < letStart_)
             return BindingKind::Var;
@@ -1398,17 +1408,19 @@ class BindingIter
             return BindingKind::Let;
         if (isNamedLambda())
             return BindingKind::NamedLambdaCallee;
         return BindingKind::Const;
     }
 
     bool isTopLevelFunction() const {
         MOZ_ASSERT(!done());
-        return index_ >= topLevelFunctionStart_ && index_ < varStart_;
+        bool result = names_[index_].isTopLevelFunction();
+        MOZ_ASSERT_IF(result, kind() == BindingKind::Var);
+        return result;
     }
 
     bool hasArgumentSlot() const {
         MOZ_ASSERT(!done());
         if (hasFormalParameterExprs())
             return false;
         return index_ >= positionalFormalStart_ && index_ < nonPositionalFormalStart_;
     }