Scopes that are not under TDZ should still push their variables onto the TDZ stack...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 2 Jul 2016 17:43:56 +0000 (17:43 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 2 Jul 2016 17:43:56 +0000 (17:43 +0000)
https://bugs.webkit.org/show_bug.cgi?id=159332
rdar://problem/27018958

Reviewed by Saam Barati.

This fixes an instacrash in this code:

    try{}catch(e){}print(e);let e;

We lift TDZ for "e" in "catch (e){}", but since that scope doesn't push anything onto the
TDZ stack, we lift TDZ from "let e".

The problem is that we weren't tracking the set of variables that do not have TDZ. We need
to track them to "block" the traversal that lifts TDZ. This change fixes this issue by
using a map that tracks all known variables, and tells you if they are under TDZ or not.

* bytecode/CodeBlock.h:
(JSC::CodeBlock::numParameters):
* bytecode/CodeOrigin.h:
* bytecompiler/BytecodeGenerator.cpp:
(JSC::Label::setLocation):
(JSC::Variable::dump):
(JSC::BytecodeGenerator::generate):
(JSC::BytecodeGenerator::BytecodeGenerator):
(JSC::BytecodeGenerator::pushLexicalScopeInternal):
(JSC::BytecodeGenerator::popLexicalScope):
(JSC::BytecodeGenerator::popLexicalScopeInternal):
(JSC::BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration):
(JSC::BytecodeGenerator::variable):
(JSC::BytecodeGenerator::needsTDZCheck):
(JSC::BytecodeGenerator::liftTDZCheckIfPossible):
(JSC::BytecodeGenerator::pushTDZVariables):
(JSC::BytecodeGenerator::getVariablesUnderTDZ):
(JSC::BytecodeGenerator::endGenerator):
(WTF::printInternal):
* bytecompiler/BytecodeGenerator.h:
(JSC::Variable::isConst):
(JSC::Variable::setIsReadOnly):
* interpreter/CallFrame.h:
(JSC::ExecState::topOfFrame):
* tests/stress/lift-tdz-bypass-catch.js: Added.
(foo):
(catch):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@202778 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.h
Source/JavaScriptCore/bytecode/CodeOrigin.h
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h
Source/JavaScriptCore/interpreter/CallFrame.h
Source/JavaScriptCore/tests/stress/lift-tdz-bypass-catch.js [new file with mode: 0644]
Source/WTF/benchmarks/LockFairnessTest.cpp

index 19b70fe..92a08c4 100644 (file)
@@ -1,3 +1,50 @@
+2016-06-30  Filip Pizlo  <fpizlo@apple.com>
+
+        Scopes that are not under TDZ should still push their variables onto the TDZ stack so that lifting TDZ doesn't bypass that scope
+        https://bugs.webkit.org/show_bug.cgi?id=159332
+        rdar://problem/27018958
+
+        Reviewed by Saam Barati.
+        
+        This fixes an instacrash in this code:
+        
+            try{}catch(e){}print(e);let e;
+        
+        We lift TDZ for "e" in "catch (e){}", but since that scope doesn't push anything onto the
+        TDZ stack, we lift TDZ from "let e".
+        
+        The problem is that we weren't tracking the set of variables that do not have TDZ. We need
+        to track them to "block" the traversal that lifts TDZ. This change fixes this issue by
+        using a map that tracks all known variables, and tells you if they are under TDZ or not.
+
+        * bytecode/CodeBlock.h:
+        (JSC::CodeBlock::numParameters):
+        * bytecode/CodeOrigin.h:
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::Label::setLocation):
+        (JSC::Variable::dump):
+        (JSC::BytecodeGenerator::generate):
+        (JSC::BytecodeGenerator::BytecodeGenerator):
+        (JSC::BytecodeGenerator::pushLexicalScopeInternal):
+        (JSC::BytecodeGenerator::popLexicalScope):
+        (JSC::BytecodeGenerator::popLexicalScopeInternal):
+        (JSC::BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration):
+        (JSC::BytecodeGenerator::variable):
+        (JSC::BytecodeGenerator::needsTDZCheck):
+        (JSC::BytecodeGenerator::liftTDZCheckIfPossible):
+        (JSC::BytecodeGenerator::pushTDZVariables):
+        (JSC::BytecodeGenerator::getVariablesUnderTDZ):
+        (JSC::BytecodeGenerator::endGenerator):
+        (WTF::printInternal):
+        * bytecompiler/BytecodeGenerator.h:
+        (JSC::Variable::isConst):
+        (JSC::Variable::setIsReadOnly):
+        * interpreter/CallFrame.h:
+        (JSC::ExecState::topOfFrame):
+        * tests/stress/lift-tdz-bypass-catch.js: Added.
+        (foo):
+        (catch):
+
 2016-07-01  Benjamin Poulain  <bpoulain@apple.com>
 
         [JSC] RegExp.compile is not returning the regexp when it succeed
index ff89fd0..b288365 100644 (file)
@@ -141,7 +141,7 @@ public:
     CString sourceCodeForTools() const; // Not quite the actual source we parsed; this will do things like prefix the source for a function with a reified signature.
     CString sourceCodeOnOneLine() const; // As sourceCodeForTools(), but replaces all whitespace runs with a single space.
     void dumpAssumingJITType(PrintStream&, JITCode::JITType) const;
-    void dump(PrintStream&) const;
+    JS_EXPORT_PRIVATE void dump(PrintStream&) const;
 
     int numParameters() const { return m_numParameters; }
     void setNumParameters(int newValue);
index 66ab427..39dd464 100644 (file)
@@ -109,7 +109,7 @@ struct CodeOrigin {
     // Get the inline stack. This is slow, and is intended for debugging only.
     Vector<CodeOrigin> inlineStack() const;
     
-    void dump(PrintStream&) const;
+    JS_EXPORT_PRIVATE void dump(PrintStream&) const;
     void dumpInContext(PrintStream&, DumpContext*) const;
 
 private:
index ebe8618..356dbd2 100644 (file)
@@ -45,6 +45,7 @@
 #include "StrongInlines.h"
 #include "UnlinkedCodeBlock.h"
 #include "UnlinkedInstructionStream.h"
+#include <wtf/CommaPrinter.h>
 #include <wtf/StdLibExtras.h>
 #include <wtf/text/WTFString.h>
 
@@ -61,6 +62,18 @@ void Label::setLocation(unsigned location)
         m_generator.instructions()[m_unresolvedJumps[i].second].u.operand = m_location - m_unresolvedJumps[i].first;
 }
 
+void Variable::dump(PrintStream& out) const
+{
+    out.print(
+        "{ident = ", m_ident,
+        ", offset = ", m_offset,
+        ", local = ", RawPointer(m_local),
+        ", attributes = ", m_attributes,
+        ", kind = ", m_kind,
+        ", symbolTableConstantIndex = ", m_symbolTableConstantIndex,
+        ", isLexicallyScoped = ", m_isLexicallyScoped, "}");
+}
+
 ParserError BytecodeGenerator::generate()
 {
     m_codeBlock->setThisRegister(m_thisRegister.virtualRegister());
@@ -603,7 +616,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
 
     // All "addVar()"s needs to happen before "initializeDefaultParameterValuesAndSetupFunctionScopeStack()" is called
     // because a function's default parameter ExpressionNodes will use temporary registers.
-    pushTDZVariables(*parentScopeTDZVariables, TDZCheckOptimization::DoNotOptimize);
+    pushTDZVariables(*parentScopeTDZVariables, TDZCheckOptimization::DoNotOptimize, TDZRequirement::UnderTDZ);
     initializeDefaultParameterValuesAndSetupFunctionScopeStack(parameters, isSimpleParameterList, functionNode, functionSymbolTable, symbolTableConstantIndex, captures, shouldCreateArgumentsVariableInParameterScope);
     
     // If we don't have  default parameter expression, then loading |this| inside an arrow function must be done
@@ -639,7 +652,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, EvalNode* evalNode, UnlinkedEvalCod
 
     m_codeBlock->setNumParameters(1);
 
-    pushTDZVariables(*parentScopeTDZVariables, TDZCheckOptimization::DoNotOptimize);
+    pushTDZVariables(*parentScopeTDZVariables, TDZCheckOptimization::DoNotOptimize, TDZRequirement::UnderTDZ);
 
     emitEnter();
 
@@ -756,7 +769,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, ModuleProgramNode* moduleProgramNod
     else
         constantSymbolTable = addConstantValue(moduleEnvironmentSymbolTable->cloneScopePart(*m_vm));
 
-    pushTDZVariables(lexicalVariables, TDZCheckOptimization::Optimize);
+    pushTDZVariables(lexicalVariables, TDZCheckOptimization::Optimize, TDZRequirement::UnderTDZ);
     bool isWithScope = false;
     m_symbolTableStack.append(SymbolTableStackEntry { moduleEnvironmentSymbolTable, m_topMostScope, isWithScope, constantSymbolTable->index() });
     emitPrefillStackTDZVariables(lexicalVariables, moduleEnvironmentSymbolTable);
@@ -1933,8 +1946,7 @@ void BytecodeGenerator::pushLexicalScopeInternal(VariableEnvironment& environmen
 
     bool isWithScope = false;
     m_symbolTableStack.append(SymbolTableStackEntry{ symbolTable, newScope, isWithScope, symbolTableConstantIndex });
-    if (tdzRequirement == TDZRequirement::UnderTDZ)
-        pushTDZVariables(environment, tdzCheckOptimization);
+    pushTDZVariables(environment, tdzCheckOptimization, tdzRequirement);
 
     if (tdzRequirement == TDZRequirement::UnderTDZ)
         emitPrefillStackTDZVariables(environment, symbolTable);
@@ -2021,10 +2033,10 @@ void BytecodeGenerator::hoistSloppyModeFunctionIfNecessary(const Identifier& fun
 void BytecodeGenerator::popLexicalScope(VariableEnvironmentNode* node)
 {
     VariableEnvironment& environment = node->lexicalVariables();
-    popLexicalScopeInternal(environment, TDZRequirement::UnderTDZ);
+    popLexicalScopeInternal(environment);
 }
 
-void BytecodeGenerator::popLexicalScopeInternal(VariableEnvironment& environment, TDZRequirement tdzRequirement)
+void BytecodeGenerator::popLexicalScopeInternal(VariableEnvironment& environment)
 {
     // NOTE: This function only makes sense for scopes that aren't ScopeRegisterType::Var (only function name scope right now is ScopeRegisterType::Var).
     // This doesn't make sense for ScopeRegisterType::Var because we deref RegisterIDs here.
@@ -2057,8 +2069,7 @@ void BytecodeGenerator::popLexicalScopeInternal(VariableEnvironment& environment
         stackEntry.m_scope->deref();
     }
 
-    if (tdzRequirement == TDZRequirement::UnderTDZ)
-        m_TDZStack.removeLast();
+    m_TDZStack.removeLast();
 }
 
 void BytecodeGenerator::prepareLexicalScopeForNextForLoopIteration(VariableEnvironmentNode* node, RegisterID* loopSymbolTable)
@@ -2182,7 +2193,7 @@ Variable BytecodeGenerator::variable(const Identifier& property, ThisResolutionT
             result.setIsReadOnly();
         return result;
     }
-
+    
     return Variable(property);
 }
 
@@ -2722,9 +2733,10 @@ void BytecodeGenerator::emitTDZCheck(RegisterID* target)
 bool BytecodeGenerator::needsTDZCheck(const Variable& variable)
 {
     for (unsigned i = m_TDZStack.size(); i--;) {
-        VariableEnvironment& identifiers = m_TDZStack[i].first;
-        if (identifiers.contains(variable.ident().impl()))
-            return true;
+        auto iter = m_TDZStack[i].find(variable.ident().impl());
+        if (iter == m_TDZStack[i].end())
+            continue;
+        return iter->value != TDZNecessityLevel::NotNeeded;
     }
 
     return false;
@@ -2747,41 +2759,54 @@ void BytecodeGenerator::liftTDZCheckIfPossible(const Variable& variable)
 {
     RefPtr<UniquedStringImpl> identifier(variable.ident().impl());
     for (unsigned i = m_TDZStack.size(); i--;) {
-        VariableEnvironment& environment = m_TDZStack[i].first;
-        if (environment.contains(identifier)) {
-            TDZCheckOptimization tdzCheckOptimizationCapability = m_TDZStack[i].second;
-            if (tdzCheckOptimizationCapability == TDZCheckOptimization::Optimize) {
-                bool wasRemoved = environment.remove(identifier);
-                RELEASE_ASSERT(wasRemoved);
-            }
+        auto iter = m_TDZStack[i].find(identifier);
+        if (iter != m_TDZStack[i].end()) {
+            if (iter->value == TDZNecessityLevel::Optimize)
+                iter->value = TDZNecessityLevel::NotNeeded;
             break;
         }
     }
 }
 
-void BytecodeGenerator::pushTDZVariables(VariableEnvironment environment, TDZCheckOptimization optimization)
+void BytecodeGenerator::pushTDZVariables(const VariableEnvironment& environment, TDZCheckOptimization optimization, TDZRequirement requirement)
 {
     if (!environment.size())
         return;
+    
+    TDZNecessityLevel level;
+    if (requirement == TDZRequirement::UnderTDZ) {
+        if (optimization == TDZCheckOptimization::Optimize)
+            level = TDZNecessityLevel::Optimize;
+        else
+            level = TDZNecessityLevel::DoNotOptimize;
+    } else
+        level = TDZNecessityLevel::NotNeeded;
+    
+    TDZMap map;
+    for (const auto& entry : environment)
+        map.add(entry.key, entry.value.isFunction() ? TDZNecessityLevel::NotNeeded : level);
 
-    Vector<UniquedStringImpl*, 4> functionsToRemove;
-    for (const auto& entry : environment) {
-        if (entry.value.isFunction())
-            functionsToRemove.append(entry.key.get());
-    }
-
-    for (UniquedStringImpl* function : functionsToRemove)
-        environment.remove(function);
-
-    m_TDZStack.append(std::make_pair(WTFMove(environment), optimization));
+    m_TDZStack.append(WTFMove(map));
 }
 
 void BytecodeGenerator::getVariablesUnderTDZ(VariableEnvironment& result)
 {
-    for (auto& pair : m_TDZStack) {
-        VariableEnvironment& environment = pair.first;
-        for (auto entry : environment)
-            result.add(entry.key.get());
+    // NOTE: This is conservative. If called at "...", it will report "x" as being under TDZ:
+    //
+    //     {
+    //         {
+    //             let x;
+    //             ...
+    //         }
+    //         let x;
+    //     }
+    //
+    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=159387
+    for (auto& map : m_TDZStack) {
+        for (auto& entry : map)  {
+            if (entry.value != TDZNecessityLevel::NotNeeded)
+                result.add(entry.key.get());
+        }
     }
 }
 
@@ -3836,7 +3861,7 @@ void BytecodeGenerator::emitPushCatchScope(VariableEnvironment& environment)
 
 void BytecodeGenerator::emitPopCatchScope(VariableEnvironment& environment) 
 {
-    popLexicalScopeInternal(environment, TDZRequirement::NotUnderTDZ);
+    popLexicalScopeInternal(environment);
 }
 
 void BytecodeGenerator::beginSwitch(RegisterID* scrutineeRegister, SwitchInfo::SwitchType type)
@@ -4631,3 +4656,21 @@ void BytecodeGenerator::endGenerator(Label* defaultLabel)
 }
 
 } // namespace JSC
+
+namespace WTF {
+
+void printInternal(PrintStream& out, JSC::Variable::VariableKind kind)
+{
+    switch (kind) {
+    case JSC::Variable::NormalVariable:
+        out.print("Normal");
+        return;
+    case JSC::Variable::SpecialVariable:
+        out.print("Special");
+        return;
+    }
+    RELEASE_ASSERT_NOT_REACHED();
+}
+
+} // namespace WTF
+
index 9914b7d..d799b18 100644 (file)
@@ -238,6 +238,8 @@ namespace JSC {
         bool isConst() const { return isReadOnly() && m_isLexicallyScoped; }
         void setIsReadOnly() { m_attributes |= ReadOnly; }
 
+        void dump(PrintStream&) const;
+
     private:
         Identifier m_ident;
         VarOffset m_offset;
@@ -728,7 +730,7 @@ namespace JSC {
         enum class ScopeRegisterType { Var, Block };
         void pushLexicalScopeInternal(VariableEnvironment&, TDZCheckOptimization, NestedScopeType, RegisterID** constantSymbolTableResult, TDZRequirement, ScopeType, ScopeRegisterType);
         void initializeBlockScopedFunctions(VariableEnvironment&, FunctionStack&, RegisterID* constantSymbolTable);
-        void popLexicalScopeInternal(VariableEnvironment&, TDZRequirement);
+        void popLexicalScopeInternal(VariableEnvironment&);
         template<typename LookUpVarKindFunctor>
         bool instantiateLexicalVariables(const VariableEnvironment&, SymbolTable*, ScopeRegisterType, LookUpVarKindFunctor);
         void emitPrefillStackTDZVariables(const VariableEnvironment&, SymbolTable*);
@@ -881,9 +883,15 @@ namespace JSC {
             int m_symbolTableConstantIndex;
         };
         Vector<SymbolTableStackEntry> m_symbolTableStack;
-        Vector<std::pair<VariableEnvironment, TDZCheckOptimization>> m_TDZStack;
+        enum class TDZNecessityLevel {
+            NotNeeded,
+            Optimize,
+            DoNotOptimize
+        };
+        typedef HashMap<RefPtr<UniquedStringImpl>, TDZNecessityLevel, IdentifierRepHash> TDZMap;
+        Vector<TDZMap> m_TDZStack;
         Optional<size_t> m_varScopeSymbolTableIndex;
-        void pushTDZVariables(VariableEnvironment, TDZCheckOptimization);
+        void pushTDZVariables(const VariableEnvironment&, TDZCheckOptimization, TDZRequirement);
 
         ScopeNode* const m_scopeNode;
         Strong<UnlinkedCodeBlock> m_codeBlock;
@@ -965,4 +973,10 @@ namespace JSC {
 
 }
 
+namespace WTF {
+
+void printInternal(PrintStream&, JSC::Variable::VariableKind);
+
+} // namespace WTF
+
 #endif // BytecodeGenerator_h
index cc300d2..96a2387 100644 (file)
@@ -149,7 +149,7 @@ namespace JSC  {
         
         // This will get you a CodeOrigin. It will always succeed. May return
         // CodeOrigin(0) if we're in native code.
-        CodeOrigin codeOrigin();
+        JS_EXPORT_PRIVATE CodeOrigin codeOrigin();
 
         Register* topOfFrame()
         {
diff --git a/Source/JavaScriptCore/tests/stress/lift-tdz-bypass-catch.js b/Source/JavaScriptCore/tests/stress/lift-tdz-bypass-catch.js
new file mode 100644 (file)
index 0000000..2dbdfbe
--- /dev/null
@@ -0,0 +1,10 @@
+//@ runDefault
+
+function foo () {
+try{}catch(e){}print(e);let e;
+}
+
+try {
+    foo();
+} catch (e) {}
+
index e808908..59cf858 100644 (file)
@@ -48,12 +48,13 @@ namespace {
 
 NO_RETURN void usage()
 {
-    printf("Usage: LockFairnessTest yieldspinlock|pausespinlock|wordlock|lock|barginglock|bargingwordlock|thunderlock|thunderwordlock|cascadelock|cascadewordlockhandofflock|mutex|all <num threads> <seconds per test>\n");
+    printf("Usage: LockFairnessTest yieldspinlock|pausespinlock|wordlock|lock|barginglock|bargingwordlock|thunderlock|thunderwordlock|cascadelock|cascadewordlockhandofflock|mutex|all <num threads> <seconds per test> <microseconds in critical section>\n");
     exit(1);
 }
 
 unsigned numThreads;
 double secondsPerTest;
+unsigned microsecondsInCriticalSection;
 
 struct Benchmark {
     template<typename LockType>
@@ -72,9 +73,19 @@ struct Benchmark {
             threads[threadIndex] = createThread(
                 "Benchmark Thread",
                 [&, threadIndex] () {
+                    if (!microsecondsInCriticalSection) {
+                        while (keepGoing) {
+                            lock.lock();
+                            counts[threadIndex]++;
+                            lock.unlock();
+                        }
+                        return;
+                    }
+                    
                     while (keepGoing) {
                         lock.lock();
                         counts[threadIndex]++;
+                        usleep(microsecondsInCriticalSection);
                         lock.unlock();
                     }
                 });
@@ -106,9 +117,10 @@ int main(int argc, char** argv)
 {
     WTF::initializeThreading();
     
-    if (argc != 4
+    if (argc != 5
         || sscanf(argv[2], "%u", &numThreads) != 1
-        || sscanf(argv[3], "%lf", &secondsPerTest) != 1)
+        || sscanf(argv[3], "%lf", &secondsPerTest) != 1
+        || sscanf(argv[4], "%u", &microsecondsInCriticalSection) != 1)
         usage();
     
     runEverything<Benchmark>(argv[1]);