[webkit-changes] [153284] trunk/Source/JavaScriptCore

Wed, 24 Jul 2013 21:04:07 -0700

Title: [153284] trunk/Source/_javascript_Core
Revision
153284
Author
[email protected]
Date
2013-07-24 21:05:07 -0700 (Wed, 24 Jul 2013)

Log Message

fourthTier: Structure should be able to tell you if it's valid to load at a given offset from any object with that structure
https://bugs.webkit.org/show_bug.cgi?id=118878

Reviewed by Oliver Hunt.

- Change Structure::isValidOffset() to actually answer the question "If I attempted
  to load from an object of this structure, at this offset, would I commit suicide
  or would I get back some kind of value?"

- Change StorageAccessData::offset to use a PropertyOffset. It should have been that
  way from the start.

- Fix PutStructure so that it sets haveStructures in all of the cases that it should.

- Make GetByOffset also reference the base object in addition to the butterfly.

The future use of this power will be to answer questions like "If I hoisted this
GetByOffset or PutByOffset to this point, would it cause crashes, or would it be
fine?"

I don't currently plan to use this power to perform validation, since the CSE has
the power to eliminate CheckStructure's that the CFA wouldn't be smart enough to
remove - both in the case of StructureSets where size >= 2 and in the case of
CheckStructures that match across PutStructures. At first I tried to write a
validator that was aware of this, but the validation code got way too complicated
and I started having nightmares of spurious assertion bugs being filed against me.

This also changes some of the code for how we hash FunctionExecutable's for debug
dumps, since that code still had some thread-safety issues. Basically, the
concurrent JIT needs to use the CodeBlock's precomputed hash and never call anything
that could transitively try to compute the hash from the source code. The source
code is a string that may be lazily computed, and that involves all manner of thread
unsafe things.

* bytecode/CodeOrigin.cpp:
(JSC::InlineCallFrame::hash):
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::::executeEffects):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleGetByOffset):
(JSC::DFG::ByteCodeParser::handlePutByOffset):
(JSC::DFG::ByteCodeParser::parseBlock):
* dfg/DFGCFAPhase.cpp:
(JSC::DFG::CFAPhase::performBlockCFA):
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::foldConstants):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGGraph.h:
(StorageAccessData):
* dfg/DFGNode.h:
(JSC::DFG::Node::convertToGetByOffset):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::LowerDFGToLLVM::compileGetByOffset):
(JSC::FTL::LowerDFGToLLVM::compilePutByOffset):
* runtime/FunctionExecutableDump.cpp:
(JSC::FunctionExecutableDump::dump):
* runtime/Structure.h:
(Structure):
(JSC::Structure::isValidOffset):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (153283 => 153284)


--- trunk/Source/_javascript_Core/ChangeLog	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/ChangeLog	2013-07-25 04:05:07 UTC (rev 153284)
@@ -1,3 +1,68 @@
+2013-07-19  Filip Pizlo  <[email protected]>
+
+        fourthTier: Structure should be able to tell you if it's valid to load at a given offset from any object with that structure
+        https://bugs.webkit.org/show_bug.cgi?id=118878
+
+        Reviewed by Oliver Hunt.
+        
+        - Change Structure::isValidOffset() to actually answer the question "If I attempted
+          to load from an object of this structure, at this offset, would I commit suicide
+          or would I get back some kind of value?"
+        
+        - Change StorageAccessData::offset to use a PropertyOffset. It should have been that
+          way from the start.
+        
+        - Fix PutStructure so that it sets haveStructures in all of the cases that it should.
+        
+        - Make GetByOffset also reference the base object in addition to the butterfly.
+        
+        The future use of this power will be to answer questions like "If I hoisted this
+        GetByOffset or PutByOffset to this point, would it cause crashes, or would it be
+        fine?"
+        
+        I don't currently plan to use this power to perform validation, since the CSE has
+        the power to eliminate CheckStructure's that the CFA wouldn't be smart enough to
+        remove - both in the case of StructureSets where size >= 2 and in the case of
+        CheckStructures that match across PutStructures. At first I tried to write a
+        validator that was aware of this, but the validation code got way too complicated
+        and I started having nightmares of spurious assertion bugs being filed against me.
+        
+        This also changes some of the code for how we hash FunctionExecutable's for debug
+        dumps, since that code still had some thread-safety issues. Basically, the
+        concurrent JIT needs to use the CodeBlock's precomputed hash and never call anything
+        that could transitively try to compute the hash from the source code. The source
+        code is a string that may be lazily computed, and that involves all manner of thread
+        unsafe things.
+
+        * bytecode/CodeOrigin.cpp:
+        (JSC::InlineCallFrame::hash):
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::::executeEffects):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::handleGetByOffset):
+        (JSC::DFG::ByteCodeParser::handlePutByOffset):
+        (JSC::DFG::ByteCodeParser::parseBlock):
+        * dfg/DFGCFAPhase.cpp:
+        (JSC::DFG::CFAPhase::performBlockCFA):
+        * dfg/DFGConstantFoldingPhase.cpp:
+        (JSC::DFG::ConstantFoldingPhase::foldConstants):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        * dfg/DFGGraph.h:
+        (StorageAccessData):
+        * dfg/DFGNode.h:
+        (JSC::DFG::Node::convertToGetByOffset):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::LowerDFGToLLVM::compileGetByOffset):
+        (JSC::FTL::LowerDFGToLLVM::compilePutByOffset):
+        * runtime/FunctionExecutableDump.cpp:
+        (JSC::FunctionExecutableDump::dump):
+        * runtime/Structure.h:
+        (Structure):
+        (JSC::Structure::isValidOffset):
+
 2013-07-18  Filip Pizlo  <[email protected]>
 
         fourthTier: AbstractInterpreter should explicitly ask AbstractState to create new AbstractValues for newly born nodes

Modified: trunk/Source/_javascript_Core/bytecode/CodeOrigin.cpp (153283 => 153284)


--- trunk/Source/_javascript_Core/bytecode/CodeOrigin.cpp	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/bytecode/CodeOrigin.cpp	2013-07-25 04:05:07 UTC (rev 153284)
@@ -84,7 +84,8 @@
 
 CodeBlockHash InlineCallFrame::hash() const
 {
-    return executable->hashFor(specializationKind());
+    return jsCast<FunctionExecutable*>(executable.get())->generatedBytecodeFor(
+        specializationKind()).hash();
 }
 
 CString InlineCallFrame::inferredName() const

Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h (153283 => 153284)


--- trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h	2013-07-25 04:05:07 UTC (rev 153284)
@@ -1256,6 +1256,7 @@
         }
 
         node->setCanExit(true);
+        m_state.setHaveStructures(true);
 
         // If this structure check is attempting to prove knowledge already held in
         // the futurePossibleStructure set then the constant folding phase should
@@ -1268,7 +1269,6 @@
         }
 
         filter(value, set);
-        m_state.setHaveStructures(true);
         break;
     }

Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (153283 => 153284)


--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp	2013-07-25 04:05:07 UTC (rev 153284)
@@ -1624,14 +1624,10 @@
         propertyStorage = base;
     else
         propertyStorage = addToGraph(GetButterfly, base);
-    // FIXME: It would be far more efficient for load elimination (and safer from
-    // an OSR standpoint) if GetByOffset also referenced the object we were loading
-    // from, and if we could load eliminate a GetByOffset even if the butterfly
-    // had changed. That would be a great success.
-    Node* getByOffset = addToGraph(GetByOffset, OpInfo(m_graph.m_storageAccessData.size()), OpInfo(prediction), propertyStorage);
+    Node* getByOffset = addToGraph(GetByOffset, OpInfo(m_graph.m_storageAccessData.size()), OpInfo(prediction), propertyStorage, base);
 
     StorageAccessData storageAccessData;
-    storageAccessData.offset = indexRelativeToBase(offset);
+    storageAccessData.offset = offset;
     storageAccessData.identifierNumber = identifierNumber;
     m_graph.m_storageAccessData.append(storageAccessData);
 
@@ -1655,7 +1651,7 @@
     Node* result = addToGraph(PutByOffset, OpInfo(m_graph.m_storageAccessData.size()), propertyStorage, base, value);
     
     StorageAccessData storageAccessData;
-    storageAccessData.offset = indexRelativeToBase(offset);
+    storageAccessData.offset = offset;
     storageAccessData.identifierNumber = identifier;
     m_graph.m_storageAccessData.append(storageAccessData);
 
@@ -2456,7 +2452,7 @@
                     value);
                 
                 StorageAccessData storageAccessData;
-                storageAccessData.offset = indexRelativeToBase(putByIdStatus.offset());
+                storageAccessData.offset = putByIdStatus.offset();
                 storageAccessData.identifierNumber = identifierNumber;
                 m_graph.m_storageAccessData.append(storageAccessData);
             } else {

Modified: trunk/Source/_javascript_Core/dfg/DFGCFAPhase.cpp (153283 => 153284)


--- trunk/Source/_javascript_Core/dfg/DFGCFAPhase.cpp	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/dfg/DFGCFAPhase.cpp	2013-07-25 04:05:07 UTC (rev 153284)
@@ -100,6 +100,8 @@
                 Node* node = block->at(i);
                 dataLogF("      %s @%u: ", Graph::opName(node->op()), node->index());
                 m_interpreter.dump(WTF::dataFile());
+                if (m_state.haveStructures())
+                    dataLog(" (Have Structures)");
                 dataLogF("\n");
             }
             if (!m_interpreter.execute(i)) {

Modified: trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp (153283 => 153284)


--- trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp	2013-07-25 04:05:07 UTC (rev 153284)
@@ -207,7 +207,7 @@
                 node->convertToGetByOffset(m_graph.m_storageAccessData.size(), propertyStorage);
                 
                 StorageAccessData storageAccessData;
-                storageAccessData.offset = indexRelativeToBase(status.offset());
+                storageAccessData.offset = status.offset();
                 storageAccessData.identifierNumber = identifierNumber;
                 m_graph.m_storageAccessData.append(storageAccessData);
                 break;
@@ -318,7 +318,7 @@
                 node->convertToPutByOffset(m_graph.m_storageAccessData.size(), propertyStorage);
                 
                 StorageAccessData storageAccessData;
-                storageAccessData.offset = indexRelativeToBase(status.offset());
+                storageAccessData.offset = status.offset();
                 storageAccessData.identifierNumber = identifierNumber;
                 m_graph.m_storageAccessData.append(storageAccessData);
                 break;

Modified: trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp (153283 => 153284)


--- trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp	2013-07-25 04:05:07 UTC (rev 153284)
@@ -820,6 +820,7 @@
         case GetByOffset: {
             if (!node->child1()->hasStorageResult())
                 setUseKindAndUnboxIfProfitable<KnownCellUse>(node->child1());
+            setUseKindAndUnboxIfProfitable<KnownCellUse>(node->child2());
             break;
         }

Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.h (153283 => 153284)


--- trunk/Source/_javascript_Core/dfg/DFGGraph.h	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.h	2013-07-25 04:05:07 UTC (rev 153284)
@@ -56,7 +56,7 @@
 namespace DFG {
 
 struct StorageAccessData {
-    size_t offset;
+    PropertyOffset offset;
     unsigned identifierNumber;
 };

Modified: trunk/Source/_javascript_Core/dfg/DFGNode.h (153283 => 153284)


--- trunk/Source/_javascript_Core/dfg/DFGNode.h	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/dfg/DFGNode.h	2013-07-25 04:05:07 UTC (rev 153284)
@@ -410,6 +410,8 @@
     {
         ASSERT(m_op == GetById || m_op == GetByIdFlush);
         m_opInfo = storageAccessDataIndex;
+        children.setChild2(children.child1());
+        children.child2().setUseKind(KnownCellUse);
         children.setChild1(storage);
         m_op = GetByOffset;
         m_flags &= ~NodeClobbersWorld;

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (153283 => 153284)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp	2013-07-25 04:05:07 UTC (rev 153284)
@@ -4009,7 +4009,7 @@
         
         StorageAccessData& storageAccessData = m_jit.graph().m_storageAccessData[node->storageAccessDataIndex()];
         
-        m_jit.load64(JITCompiler::Address(storageGPR, storageAccessData.offset * sizeof(EncodedJSValue)), resultGPR);
+        m_jit.load64(JITCompiler::Address(storageGPR, offsetRelativeToBase(storageAccessData.offset)), resultGPR);
         
         jsValueResult(resultGPR, node);
         break;
@@ -4031,7 +4031,7 @@
 
         StorageAccessData& storageAccessData = m_jit.graph().m_storageAccessData[node->storageAccessDataIndex()];
         
-        m_jit.store64(valueGPR, JITCompiler::Address(storageGPR, storageAccessData.offset * sizeof(EncodedJSValue)));
+        m_jit.store64(valueGPR, JITCompiler::Address(storageGPR, offsetRelativeToBase(storageAccessData.offset)));
         
         noResult(node);
         break;

Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToLLVM.cpp (153283 => 153284)


--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToLLVM.cpp	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToLLVM.cpp	2013-07-25 04:05:07 UTC (rev 153284)
@@ -1351,7 +1351,7 @@
                 m_out.address(
                     m_heaps.properties[data.identifierNumber],
                     lowStorage(m_node->child1()),
-                    data.offset * sizeof(EncodedJSValue))));
+                    offsetRelativeToBase(data.offset))));
     }
     
     void compilePutByOffset()
@@ -1364,7 +1364,7 @@
             m_out.address(
                 m_heaps.properties[data.identifierNumber],
                 lowStorage(m_node->child1()),
-                data.offset * sizeof(EncodedJSValue)));
+                offsetRelativeToBase(data.offset)));
     }
     
     void compileGetGlobalVar()

Modified: trunk/Source/_javascript_Core/runtime/FunctionExecutableDump.cpp (153283 => 153284)


--- trunk/Source/_javascript_Core/runtime/FunctionExecutableDump.cpp	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/runtime/FunctionExecutableDump.cpp	2013-07-25 04:05:07 UTC (rev 153284)
@@ -26,11 +26,23 @@
 #include "config.h"
 #include "FunctionExecutableDump.h"
 
+#include "CodeBlock.h"
+
 namespace JSC {
 
 void FunctionExecutableDump::dump(PrintStream& out) const
 {
-    out.print(m_executable->inferredName().string(), "#", m_executable->hashFor(CodeForCall), "/", m_executable->hashFor(CodeForConstruct), ":[", RawPointer(m_executable), "]");
+    out.print(m_executable->inferredName().string(), "#");
+    if (m_executable->isGeneratedForCall())
+        out.print(m_executable->generatedBytecodeForCall().hash());
+    else
+        out.print("<nogen>");
+    out.print("/");
+    if (m_executable->isGeneratedForConstruct())
+        out.print(m_executable->generatedBytecodeForConstruct().hash());
+    else
+        out.print("<nogen>");
+    out.print(":[", RawPointer(m_executable), "]");
 }
 
 } // namespace JSC

Modified: trunk/Source/_javascript_Core/runtime/Structure.h (153283 => 153284)


--- trunk/Source/_javascript_Core/runtime/Structure.h	2013-07-25 04:05:04 UTC (rev 153283)
+++ trunk/Source/_javascript_Core/runtime/Structure.h	2013-07-25 04:05:07 UTC (rev 153284)
@@ -220,20 +220,11 @@
         return outOfLineCapacity() + inlineCapacity();
     }
 
-    PropertyOffset firstValidOffset() const
-    {
-        if (hasInlineStorage())
-            return 0;
-        return firstOutOfLineOffset;
-    }
-    PropertyOffset lastValidOffset() const
-    {
-        return m_offset;
-    }
     bool isValidOffset(PropertyOffset offset) const
     {
-        return offset >= firstValidOffset()
-            && offset <= lastValidOffset();
+        return JSC::isValidOffset(offset)
+            && (offset < m_inlineCapacity
+                || (offset >= firstOutOfLineOffset && offset <= m_offset));
     }
 
     bool masqueradesAsUndefined(JSGlobalObject* lexicalGlobalObject);

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to