Make JSC_validateExceptionChecks=1 succeed on JSTests/slowMicrobenchmarks/spread...

https://bugs.webkit.org/show_bug.cgi?id=175347

Patch by Robin Morisset <rmorisset@apple.com> on 2017-08-08
Reviewed by Saam Barati.

This is done by making finishCreation explicitely check for exceptions after setConstantRegister and setConstantIdentifiersSetRegisters.
I chose to have this check replace the boolean returned previously by these functions for readability. The performance impact should be
negligible considering how much more finishCreation does.
This fix then caused another issue to appear as it was now clear that finishCreation can throw. And since it is called by ProgramCodeBlock::create(),
FunctionCodeBlock::create() and friends, that are in turn called by ScriptExecutable::newCodeBlockFor, this last function also required a few tweaks.

* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::finishCreation):
(JSC::CodeBlock::setConstantIdentifierSetRegisters):
(JSC::CodeBlock::setConstantRegisters):
* bytecode/CodeBlock.h:
* runtime/ScriptExecutable.cpp:
(JSC::ScriptExecutable::newCodeBlockFor):

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

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 0dc70eb9ccdfe54c9f47117f4a09580d7e354a27..4d61639120047adeff15caa1f87b894ea8427205 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,24 @@
+2017-08-08  Robin Morisset  <rmorisset@apple.com>
+
+        Make JSC_validateExceptionChecks=1 succeed on JSTests/slowMicrobenchmarks/spread-small-array.js.
+        https://bugs.webkit.org/show_bug.cgi?id=175347
+
+        Reviewed by Saam Barati.
+
+        This is done by making finishCreation explicitely check for exceptions after setConstantRegister and setConstantIdentifiersSetRegisters.
+        I chose to have this check replace the boolean returned previously by these functions for readability. The performance impact should be
+        negligible considering how much more finishCreation does.
+        This fix then caused another issue to appear as it was now clear that finishCreation can throw. And since it is called by ProgramCodeBlock::create(),
+        FunctionCodeBlock::create() and friends, that are in turn called by ScriptExecutable::newCodeBlockFor, this last function also required a few tweaks.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::finishCreation):
+        (JSC::CodeBlock::setConstantIdentifierSetRegisters):
+        (JSC::CodeBlock::setConstantRegisters):
+        * bytecode/CodeBlock.h:
+        * runtime/ScriptExecutable.cpp:
+        (JSC::ScriptExecutable::newCodeBlockFor):
+
 2017-08-08  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         Unreviewed, fix Ubuntu LTS build

diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.cpp b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
index 2f8e18c73234da7500a59a6093eacb175f1886b8..28ee727d6c8c49f6e51536b43eda416cfcee1ac0 100644 (file)
--- a/Source/JavaScriptCore/bytecode/CodeBlock.cpp
+++ b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
@@ -401,15 +401,16 @@ bool CodeBlock::finishCreation(VM& vm, ScriptExecutable* ownerExecutable, Unlink
     JSScope* scope)
 {
     Base::finishCreation(vm);
+    auto throwScope = DECLARE_THROW_SCOPE(vm);
 
     if (vm.typeProfiler() || vm.controlFlowProfiler())
         vm.functionHasExecutedCache()->removeUnexecutedRange(ownerExecutable->sourceID(), ownerExecutable->typeProfilingStartOffset(), ownerExecutable->typeProfilingEndOffset());
 
-    if (!setConstantRegisters(unlinkedCodeBlock->constantRegisters(), unlinkedCodeBlock->constantsSourceCodeRepresentation()))
-        return false;
+    setConstantRegisters(unlinkedCodeBlock->constantRegisters(), unlinkedCodeBlock->constantsSourceCodeRepresentation());
+    RETURN_IF_EXCEPTION(throwScope, false);
 
-    if (!setConstantIdentifierSetRegisters(vm, unlinkedCodeBlock->constantIdentifierSets()))
-        return false;
+    setConstantIdentifierSetRegisters(vm, unlinkedCodeBlock->constantIdentifierSets());
+    RETURN_IF_EXCEPTION(throwScope, false);
 
     if (unlinkedCodeBlock->usesGlobalObject())
         m_constantRegisters[unlinkedCodeBlock->globalObjectRegister().toConstantIndex()].set(*m_vm, this, m_globalObject.get());
@@ -869,7 +870,7 @@ CodeBlock::~CodeBlock()
 #endif // ENABLE(JIT)
 }
 
-bool CodeBlock::setConstantIdentifierSetRegisters(VM& vm, const Vector<ConstantIndentifierSetEntry>& constants)
+void CodeBlock::setConstantIdentifierSetRegisters(VM& vm, const Vector<ConstantIndentifierSetEntry>& constants)
 {
     auto scope = DECLARE_THROW_SCOPE(vm);
     JSGlobalObject* globalObject = m_globalObject.get();
@@ -877,24 +878,21 @@ bool CodeBlock::setConstantIdentifierSetRegisters(VM& vm, const Vector<ConstantI
 
     for (const auto& entry : constants) {
         Structure* setStructure = globalObject->setStructure();
-        RETURN_IF_EXCEPTION(scope, false);
+        RETURN_IF_EXCEPTION(scope, void());
         JSSet* jsSet = JSSet::create(exec, vm, setStructure);
-        RETURN_IF_EXCEPTION(scope, false);
+        RETURN_IF_EXCEPTION(scope, void());
 
         const IdentifierSet& set = entry.first;
         for (auto setEntry : set) {
             JSString* jsString = jsOwnedString(&vm, setEntry.get()); 
             jsSet->add(exec, jsString);
-            RETURN_IF_EXCEPTION(scope, false);
+            RETURN_IF_EXCEPTION(scope, void());
         }
         m_constantRegisters[entry.second].set(vm, this, jsSet);
     }
-    
-    scope.release();
-    return true;
 }
 
-bool CodeBlock::setConstantRegisters(const Vector<WriteBarrier<Unknown>>& constants, const Vector<SourceCodeRepresentation>& constantsSourceCodeRepresentation)
+void CodeBlock::setConstantRegisters(const Vector<WriteBarrier<Unknown>>& constants, const Vector<SourceCodeRepresentation>& constantsSourceCodeRepresentation)
 {
     auto scope = DECLARE_THROW_SCOPE(*m_vm);
     JSGlobalObject* globalObject = m_globalObject.get();
@@ -921,7 +919,7 @@ bool CodeBlock::setConstantRegisters(const Vector<WriteBarrier<Unknown>>& consta
                 constant = clone;
             } else if (isTemplateRegistryKey(*m_vm, constant)) {
                 auto* templateObject = globalObject->templateRegistry().getTemplateObject(exec, jsCast<JSTemplateRegistryKey*>(constant));
-                RETURN_IF_EXCEPTION(scope, false);
+                RETURN_IF_EXCEPTION(scope, void());
                 constant = templateObject;
             }
         }
@@ -930,8 +928,6 @@ bool CodeBlock::setConstantRegisters(const Vector<WriteBarrier<Unknown>>& consta
     }
 
     m_constantsSourceCodeRepresentation = constantsSourceCodeRepresentation;
-
-    return true;
 }
 
 void CodeBlock::setAlternative(VM& vm, CodeBlock* alternative)

diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.h b/Source/JavaScriptCore/bytecode/CodeBlock.h
index 06eb19cf499797b58fda30faf2a6e34cb0e27532..d60051400375c32645072d7aa45fded82932f722 100644 (file)
--- a/Source/JavaScriptCore/bytecode/CodeBlock.h
+++ b/Source/JavaScriptCore/bytecode/CodeBlock.h
@@ -918,9 +918,9 @@ private:
 
     void updateAllPredictionsAndCountLiveness(unsigned& numberOfLiveNonArgumentValueProfiles, unsigned& numberOfSamplesInProfiles);
 
-    bool setConstantIdentifierSetRegisters(VM&, const Vector<ConstantIndentifierSetEntry>& constants);
+    void setConstantIdentifierSetRegisters(VM&, const Vector<ConstantIndentifierSetEntry>& constants);
 
-    bool setConstantRegisters(const Vector<WriteBarrier<Unknown>>& constants, const Vector<SourceCodeRepresentation>& constantsSourceCodeRepresentation);
+    void setConstantRegisters(const Vector<WriteBarrier<Unknown>>& constants, const Vector<SourceCodeRepresentation>& constantsSourceCodeRepresentation);
 
     void replaceConstant(int index, JSValue value)
     {

diff --git a/Source/JavaScriptCore/runtime/ScriptExecutable.cpp b/Source/JavaScriptCore/runtime/ScriptExecutable.cpp
index 30ac584991d98541705f087edf47b2f9366dce62..5ef01cdd59313220a962ab4139bf6545e7efea38 100644 (file)
--- a/Source/JavaScriptCore/runtime/ScriptExecutable.cpp
+++ b/Source/JavaScriptCore/runtime/ScriptExecutable.cpp
@@ -187,6 +187,7 @@ CodeBlock* ScriptExecutable::newCodeBlockFor(
         auto codeBlock = EvalCodeBlock::create(vm,
             executable, executable->m_unlinkedEvalCodeBlock.get(), scope,
             executable->source().provider());
+        ASSERT(throwScope.exception() || codeBlock);
         if (!codeBlock) {
             exception = throwException(
                 exec, throwScope,
@@ -204,6 +205,7 @@ CodeBlock* ScriptExecutable::newCodeBlockFor(
         auto codeBlock = ProgramCodeBlock::create(vm,
             executable, executable->m_unlinkedProgramCodeBlock.get(), scope,
             executable->source().provider(), startColumn());
+        ASSERT(throwScope.exception() || codeBlock);
         if (!codeBlock) {
             exception = throwException(
                 exec, throwScope,
@@ -221,6 +223,7 @@ CodeBlock* ScriptExecutable::newCodeBlockFor(
         auto codeBlock = ModuleProgramCodeBlock::create(vm,
             executable, executable->m_unlinkedModuleProgramCodeBlock.get(), scope,
             executable->source().provider(), startColumn());
+        ASSERT(throwScope.exception() || codeBlock);
         if (!codeBlock) {
             exception = throwException(
                 exec, throwScope,
@@ -251,6 +254,7 @@ CodeBlock* ScriptExecutable::newCodeBlockFor(
         return nullptr;
     }
 
+    throwScope.release();
     return FunctionCodeBlock::create(vm, executable, unlinkedCodeBlock, scope, 
         source().provider(), source().startOffset(), startColumn());
 }