Changeset 33541 in webkit

Timestamp:

May 16, 2008 5:27:52 PM (16 years ago)

Author:

oliver@apple.com

Message:

Bug 19076: SquirrelFish: RegisterFile can be corrupted if implictly reenter global scope with no declared vars
<​https://bugs.webkit.org/show_bug.cgi?id=19076>

Reviewed by Geoff

Don't delay allocation of initial global RegisterFile, as we can't guarantee we will be able
to allocate the global 'this' register safely at any point after initialisation of the Global
Object.

Unfortunately this initial allocation caused a regression of 0.2-0.3%, however this patch adds
support for the static slot optimisation for the global Math object which brings it to a 0.3%
progression.

Location:

branches/squirrelfish

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/VM/CodeGenerator.cpp (modified) (8 diffs)
• JavaScriptCore/VM/CodeGenerator.h (modified) (3 diffs)
• JavaScriptCore/VM/Machine.cpp (modified) (1 diff)
• JavaScriptCore/kjs/ExecState.h (modified) (1 diff)
• JavaScriptCore/kjs/JSGlobalObject.cpp (modified) (2 diffs)
• JavaScriptCore/kjs/JSGlobalObject.h (modified) (2 diffs)
• JavaScriptCore/kjs/nodes.cpp (modified) (1 diff)
• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/js/direct-entry-to-function-code-expected.txt (added)
• LayoutTests/fast/js/direct-entry-to-function-code.html (added)

branches/squirrelfish/JavaScriptCore/ChangeLog

@@ +1 @@
+2008-05-16  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Geoff.
+
+        Bug 19076: SquirrelFish: RegisterFile can be corrupted if implictly reenter global scope with no declared vars
+        <https://bugs.webkit.org/show_bug.cgi?id=19076>
+
+        Don't delay allocation of initial global RegisterFile, as we can't guarantee we will be able
+        to allocate the global 'this' register safely at any point after initialisation of the Global
+        Object.
+
+        Unfortunately this initial allocation caused a regression of 0.2-0.3%, however this patch adds
+        support for the static slot optimisation for the global Math object which brings it to a 0.3%
+        progression.
+
+        * VM/CodeGenerator.cpp:
+        (KJS::CodeGenerator::programCodeThis):
+        (KJS::CodeGenerator::CodeGenerator):
+        (KJS::CodeGenerator::addParameter):
+        * VM/CodeGenerator.h:
+        * VM/Machine.cpp:
+        (KJS::Machine::execute):
+        * kjs/ExecState.h:
+        * kjs/JSGlobalObject.cpp:
+        (KJS::JSGlobalObject::reset):
+        * kjs/JSGlobalObject.h:
+        (KJS::JSGlobalObject::GlobalPropertyInfo::GlobalPropertyInfo):
+        (KJS::JSGlobalObject::addStaticGlobals):
+        * kjs/nodes.cpp:
+
 2008-05-16  Cameron Zwarich  <cwzwarich@uwaterloo.ca>
 

branches/squirrelfish/JavaScriptCore/VM/CodeGenerator.cpp

@@ -163 +163 @@
 }
 
+RegisterID* CodeGenerator::programCodeThis()
+{
+    static RegisterID programThis(Machine::ProgramCodeThisRegister);
+    return &programThis;
+}
+
 CodeGenerator::CodeGenerator(ProgramNode* programNode, const Debugger* debugger, const ScopeChain& scopeChain, SymbolTable* symbolTable, CodeBlock* codeBlock, VarStack& varStack, FunctionStack& functionStack, bool canCreateVariables)
     : m_shouldEmitDebugHooks(!!debugger)
@@ -178 +184 @@
 {
     // Global code can inherit previously defined symbols.
-    if (int size = symbolTable->size()) {
-        // re-add "this" to symbol table
-        ASSERT(!symbolTable->contains(m_propertyNames->thisIdentifier.ustring().rep()));
-        symbolTable->add(m_propertyNames->thisIdentifier.ustring().rep(), Machine::ProgramCodeThisRegister);
-        ++size;
-
-        // Add previously defined symbols to bookkeeping.
-        m_locals.resize(size);
-        SymbolTable::iterator end = symbolTable->end();
-        for (SymbolTable::iterator it = symbolTable->begin(); it != end; ++it)
-            m_locals[localsIndex(it->second.getIndex())].setIndex(it->second.getIndex());
-
-        // Shift new symbols so they get stored prior to previously defined symbols.
-        m_nextVar -= size;
-    } else
-        addVar(m_propertyNames->thisIdentifier, false); // No need to make "this" a true parameter, since it's not passed by our caller.
+    int size = symbolTable->size() + 1; // + 1 slot for  "this"
+    m_thisRegister = programCodeThis();
+
+    // Add previously defined symbols to bookkeeping.
+    m_locals.resize(size);
+    SymbolTable::iterator end = symbolTable->end();
+    for (SymbolTable::iterator it = symbolTable->begin(); it != end; ++it)
+        m_locals[localsIndex(it->second.getIndex())].setIndex(it->second.getIndex());
+
+    // Shift new symbols so they get stored prior to previously defined symbols.
+    m_nextVar -= size;
 
     JSGlobalObject* globalObject = static_cast<JSGlobalObject*>(scopeChain.bottom());
@@ -232 +233 @@
     , m_scopeNode(functionBody)
     , m_codeBlock(codeBlock)
+    , m_thisRegister(0)
     , m_finallyDepth(0)
     , m_dynamicScopeDepth(0)
@@ -260 +262 @@
     m_locals.resize(localsIndex(m_nextParameter) + 1);
 
-    addParameter(m_propertyNames->thisIdentifier);
+    m_thisRegister = addParameter(m_propertyNames->thisIdentifier);
     for (size_t i = 0; i < parameters.size(); ++i) {
         addParameter(parameters[i]);
@@ -272 +274 @@
     , m_scopeNode(evalNode)
     , m_codeBlock(codeBlock)
+    , m_thisRegister(0)
     , m_finallyDepth(0)
     , m_dynamicScopeDepth(0)
@@ -279 +282 @@
     , m_propertyNames(CommonIdentifiers::shared())
 {
-    addVar(m_propertyNames->thisIdentifier, false);
+    addVar(m_propertyNames->thisIdentifier, m_thisRegister, false);
     
     for (size_t i = 0; i < varStack.size(); ++i)
@@ -295 +298 @@
 }
 
-void CodeGenerator::addParameter(const Identifier& ident)
+RegisterID* CodeGenerator::addParameter(const Identifier& ident)
 {
     // Parameters overwrite var declarations, but not function declarations,
     // in the symbol table.
+    RegisterID* result = 0;
     UString::Rep* rep = ident.ustring().rep();
     if (!m_functions.contains(rep)) {
         symbolTable().set(rep, m_nextParameter);
         m_locals[localsIndex(m_nextParameter)].setIndex(m_nextParameter);
+        result = &(m_locals[localsIndex(m_nextParameter)]);
     }
     
@@ -309 +314 @@
     ++m_nextParameter;
     ++m_codeBlock->numParameters;
+    return result;
 }
 

branches/squirrelfish/JavaScriptCore/VM/CodeGenerator.h

@@ -94 +94 @@
         RegisterID* registerForLocalConstInit(const Identifier&);
 
+        // Returns the register storing "this"
+        RegisterID* thisRegister() { return m_thisRegister; }
+
         bool isLocalConstant(const Identifier&);
 
@@ -289 +292 @@
         bool addVar(const Identifier&, RegisterID*&, bool isConstant);
 
-        void addParameter(const Identifier&);
+        RegisterID* addParameter(const Identifier&);
 
         unsigned addConstant(FuncDeclNode*);
@@ -312 +315 @@
         
         HashSet<RefPtr<UString::Rep>, IdentifierRepHash> m_functions;
-
+        static RegisterID* programCodeThis();
+        RegisterID* m_thisRegister;
         SegmentedVector<RegisterID, 512> m_locals;
         SegmentedVector<RegisterID, 512> m_temporaries;

branches/squirrelfish/JavaScriptCore/VM/Machine.cpp

@@ -594 +594 @@
 
     RegisterFile* registerFile = registerFileStack->pushGlobalRegisterFile();
+    ASSERT(registerFile->numGlobalSlots());
     CodeBlock* codeBlock = &programNode->code(scopeChain, !registerFileStack->inImplicitCall());
     registerFile->addGlobalSlots(codeBlock->numVars);

branches/squirrelfish/JavaScriptCore/kjs/ExecState.h

@@ -75 +75 @@
         // Global object in which the current script was defined. (Can differ
         // from dynamicGlobalObject() during function calls across frames.)
-        JSGlobalObject* ExecState::lexicalGlobalObject() const
+        JSGlobalObject* lexicalGlobalObject() const
         {
             return m_scopeChain->globalObject();

branches/squirrelfish/JavaScriptCore/kjs/JSGlobalObject.cpp

@@ -258 +258 @@
     _prop.clear();
     registerFileStack().current()->clear();
+    registerFileStack().current()->addGlobalSlots(1);
     symbolTable().clear();
 
@@ -378 +379 @@
 
     // Set global values.
-
-    putDirect("Math", new MathObjectImp(exec, d()->objectPrototype), DontEnum);
-    putDirect("NaN", jsNaN(), DontEnum | DontDelete);
-    putDirect("Infinity", jsNumber(Inf), DontEnum | DontDelete);
-    putDirect("undefined", jsUndefined(), DontEnum | DontDelete);
+    GlobalPropertyInfo staticGlobals[] = {
+        GlobalPropertyInfo("Math", new MathObjectImp(exec, d()->objectPrototype), DontEnum | DontDelete),
+        GlobalPropertyInfo("NaN", jsNaN(), DontEnum | DontDelete),
+        GlobalPropertyInfo("Infinity", jsNumber(Inf), DontEnum | DontDelete),
+        GlobalPropertyInfo("undefined", jsUndefined(), DontEnum | DontDelete)
+    };
+
+    addStaticGlobals(staticGlobals, sizeof(staticGlobals) / sizeof(GlobalPropertyInfo));
 
     // Set global functions.

branches/squirrelfish/JavaScriptCore/kjs/JSGlobalObject.h

@@ -264 +264 @@
         JSGlobalObjectData* d() const { return static_cast<JSGlobalObjectData*>(JSVariableObject::d); }
 
+        struct GlobalPropertyInfo {
+            GlobalPropertyInfo(const Identifier& i, JSValue* v, unsigned a)
+                : identifier(i)
+                , value(v)
+                , attributes(a)
+            {
+            }
+
+            const Identifier& identifier;
+            JSValue* value;
+            unsigned attributes;
+        };
+        void addStaticGlobals(GlobalPropertyInfo*, int count);
+
         bool checkTimeout();
         void resetTimeoutCheck();
@@ -269 +283 @@
         static JSGlobalObject* s_head;
     };
+
+    inline void JSGlobalObject::addStaticGlobals(GlobalPropertyInfo* globals, int count)
+    {
+        RegisterFile* registerFile = registerFileStack().current();
+        ASSERT(registerFile->safeForReentry() && registerFile->isGlobal() && !registerFile->size());
+        int index = -registerFile->numGlobalSlots() - 1;
+        registerFile->addGlobalSlots(count);
+        for (int i = 0; i < count; ++i) {
+            ASSERT(globals[i].attributes & DontDelete);
+            SymbolTableEntry newEntry(index, globals[i].attributes);
+            symbolTable().add(globals[i].identifier.ustring().rep(), newEntry);
+            valueAt(index) = globals[i].value;
+            --index;
+        }
+    }
 
     inline bool JSGlobalObject::timedOut()

branches/squirrelfish/JavaScriptCore/kjs/nodes.cpp

@@ -585 +585 @@
 RegisterID* ThisNode::emitCode(CodeGenerator& generator, RegisterID* dst)
 {
-    RegisterID* r0 = generator.registerForLocal(generator.propertyNames().thisIdentifier);
-    return generator.moveToDestinationIfNeeded(dst, r0);
+    return generator.moveToDestinationIfNeeded(dst, generator.thisRegister());
 }
 

branches/squirrelfish/LayoutTests/ChangeLog

@@ +1 @@
+2008-05-16  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Geoff
+
+        Bug 19076: SquirrelFish: RegisterFile can be corrupted if implictly reenter global scope with no declared vars
+        <https://bugs.webkit.org/show_bug.cgi?id=19076>
+
+        Test that we can re-enter safely.
+
+        * fast/js/direct-entry-to-function-code-expected.txt: Added.
+        * fast/js/direct-entry-to-function-code.html: Added.
+
 2008-05-16  Cameron Zwarich  <cwzwarich@uwaterloo.ca>