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 19b70fef6c4d52dab9aa531b5d6b75a2c5a98161..92a08c433aff8757bfd6e5dd1f3c3a4dac53fd1d 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 ff89fd03d5ef110b14a4e93279fdebcb449a06cd..b2883654be345e7ab0110ec1b6f5d697a18c27c2 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 66ab427241796c4d39b4b5330efb81759618e212..39dd46440d4c2ac831a53902bcb5c36e239c02d7 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 ebe8618c22521b084992e670fbb59baf30e8d84b..356dbd24480bb3e4f5440c6bb74989fca5cff520 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 9914b7d22e43f7674d603e75388e2f055a74a602..d799b18940ad820c6c84bdf328b133261394b8dd 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 cc300d246c31f8adf317ac159a6e8c6d6a501a5f..96a23873fbd29174f7b46f0089cc4bc519b8d244 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 e808908418cf150ad75eb35ab4a46b050875f1ab..59cf858a905d365c57c48cb45d0b48d31e4da057 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]);