Attachment 293693 Details for Bug 164100 – WIP

[patch] WIP

bug-164100-20161102135237.patch (text/plain), 17.76 KB, created by Keith Miller on 2016-11-02 13:56:36 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2016-11-02 13:56:36 PDT

Size: 17.76 KB

patch

obsolete

>Subversion Revision: 208292
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 4633e7af5542dbaac63dfa8a3687b11171dcf5c9..cb59e13d99725bafca96fd986b6010e74bf757c2 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,5 +1,26 @@
> 2016-11-02  Keith Miller  <keith_miller@apple.com>
> 
>+        Wasm starts a new stack whenever it adds a new block.
>+        https://bugs.webkit.org/show_bug.cgi?id=164100
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * testWasm.cpp:
>+        (runWasmTests):
>+        * wasm/WasmB3IRGenerator.cpp:
>+        * wasm/WasmFunctionParser.h:
>+        (JSC::Wasm::FunctionParser<Context>::parseBlock):
>+        (JSC::Wasm::FunctionParser<Context>::addReturn):
>+        (JSC::Wasm::FunctionParser<Context>::parseExpression):
>+        (JSC::Wasm::FunctionParser<Context>::parseUnreachableExpression):
>+        (JSC::Wasm::FunctionParser<Context>::popExpressionStack):
>+        * wasm/WasmValidate.cpp:
>+        (JSC::Wasm::Validate::ControlData::hasNonVoidSignature):
>+        (JSC::Wasm::dumpExpressionStack):
>+        (JSC::Wasm::Validate::dump):
>+
>+2016-11-02  Keith Miller  <keith_miller@apple.com>
>+
>         We should not pop from an empty stack in the Wasm function parser.
>         https://bugs.webkit.org/show_bug.cgi?id=164275
> 
>diff --git a/Source/JavaScriptCore/testWasm.cpp b/Source/JavaScriptCore/testWasm.cpp
>index d1b88d8d578f37ca7b53d35632d8ea6467e7c3d3..1eea0bac3b9002066ec4a126e37f9181d6b5a80a 100644
>--- a/Source/JavaScriptCore/testWasm.cpp
>+++ b/Source/JavaScriptCore/testWasm.cpp
>@@ -293,6 +293,87 @@ static void checkPlan(Plan& plan, unsigned expectedNumberOfFunctions)
> // For now we inline the test files.
> static void runWasmTests()
> {
>+    {
>+        // Generated from:
>+        //    (module
>+        //     (func (export "dumb-less-than") (param $x i32) (param $y i32) (result i32)
>+        //      (loop $loop i32
>+        //       (i32.eq (get_local $x) (get_local $y))
>+        //       (if i32
>+        //        (then (return (i32.const 0)))
>+        //        (else
>+        //         (get_local $x)
>+        //         (set_local $x (i32.sub (get_local $x) (i32.const 1)))
>+        //         (i32.const 0)
>+        //         (i32.ne)
>+        //         (br_if $loop)
>+        //         (i32.const 1)
>+        //         )
>+        //        )
>+        //       )
>+        //      )
>+        //     )
>+        Vector<uint8_t> vector = {
>+            0x00, 0x61, 0x73, 0x6d, 0x0c, 0x00, 0x00, 0x00, 0x01, 0x87, 0x80, 0x80, 0x80, 0x00, 0x01, 0x40,
>+            0x02, 0x01, 0x01, 0x01, 0x01, 0x03, 0x82, 0x80, 0x80, 0x80, 0x00, 0x01, 0x00, 0x07, 0x92, 0x80,
>+            0x80, 0x80, 0x00, 0x01, 0x0e, 0x64, 0x75, 0x6d, 0x62, 0x2d, 0x6c, 0x65, 0x73, 0x73, 0x2d, 0x74,
>+            0x68, 0x61, 0x6e, 0x00, 0x00, 0x0a, 0xa7, 0x80, 0x80, 0x80, 0x00, 0x01, 0xa1, 0x80, 0x80, 0x80,
>+            0x00, 0x00, 0x02, 0x01, 0x14, 0x00, 0x14, 0x01, 0x4d, 0x03, 0x01, 0x10, 0x00, 0x09, 0x04, 0x14,
>+            0x00, 0x14, 0x00, 0x10, 0x01, 0x41, 0x15, 0x00, 0x10, 0x00, 0x4e, 0x07, 0x01, 0x10, 0x01, 0x0f,
>+            0x0f, 0x0f
>+        };
>+
>+        Plan plan(*vm, vector);
>+        checkPlan(plan, 1);
>+
>+        // Test this doesn't crash.
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(0), box(1) }), 1);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(1), box(0) }), 0);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(2), box(1) }), 0);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(1), box(2) }), 1);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(2), box(2) }), 0);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(1), box(1) }), 0);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(2), box(6) }), 1);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(100), box(6) }), 0);
>+    }
>+
>+    {
>+        // Generated from:
>+        //    (module
>+        //     (func (export "dumb-less-than") (param $x i32) (param $y i32) (result i32)
>+        //      (loop
>+        //       (if (i32.eq (get_local $x) (get_local $y))
>+        //        (then (return (i32.const 0)))
>+        //        (else (if (i32.eq (get_local $x) (i32.const 0))
>+        //               (return (i32.const 1)))))
>+        //       (set_local $x (i32.sub (get_local $x) (i32.const 1)))
>+        //       (br 0)
>+        //       )
>+        //      )
>+        //     )
>+        Vector<uint8_t> vector = {
>+            0x00, 0x61, 0x73, 0x6d, 0x0c, 0x00, 0x00, 0x00, 0x01, 0x87, 0x80, 0x80, 0x80, 0x00, 0x01, 0x40,
>+            0x02, 0x01, 0x01, 0x01, 0x01, 0x03, 0x82, 0x80, 0x80, 0x80, 0x00, 0x01, 0x00, 0x07, 0x92, 0x80,
>+            0x80, 0x80, 0x00, 0x01, 0x0e, 0x64, 0x75, 0x6d, 0x62, 0x2d, 0x6c, 0x65, 0x73, 0x73, 0x2d, 0x74,
>+            0x68, 0x61, 0x6e, 0x00, 0x00, 0x0a, 0xac, 0x80, 0x80, 0x80, 0x00, 0x01, 0xa6, 0x80, 0x80, 0x80,
>+            0x00, 0x00, 0x02, 0x00, 0x14, 0x00, 0x14, 0x01, 0x4d, 0x03, 0x00, 0x10, 0x00, 0x09, 0x04, 0x14,
>+            0x00, 0x10, 0x00, 0x4d, 0x03, 0x00, 0x10, 0x01, 0x09, 0x0f, 0x0f, 0x14, 0x00, 0x10, 0x01, 0x41,
>+            0x15, 0x00, 0x06, 0x00, 0x0f, 0x00, 0x0f
>+        };
>+
>+        Plan plan(*vm, vector);
>+        checkPlan(plan, 1);
>+
>+        // Test this doesn't crash.
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(0), box(1) }), 1);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(1), box(0) }), 0);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(2), box(1) }), 0);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(1), box(2) }), 1);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(2), box(2) }), 0);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(1), box(1) }), 0);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(2), box(6) }), 1);
>+        CHECK_EQ(invoke<int>(*plan.result(0)->jsEntryPoint, { box(100), box(6) }), 0);
>+    }
> 
>     {
>         // Generated from:
>diff --git a/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp b/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp
>index 1de6e277442c13cae99019e10ac5fe662a120cc1..eaa4208862c00b87d15aca9d31401c8141816a46 100644
>--- a/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp
>+++ b/Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp
>@@ -155,6 +155,8 @@ public:
>             return BlockType::Loop;
>         }
> 
>+        bool hasNonVoidSignature() const { return result.size(); }
>+
>         BasicBlock* targetBlockForBranch(Procedure& proc)
>         {
>             if (type() == BlockType::Loop)
>@@ -210,7 +212,7 @@ public:
> 
>     bool isContinuationReachable(ControlData&);
> 
>-    void dump(const Vector<ControlType>& controlStack, const ExpressionList& expressionStack);
>+    void dump(const Vector<FunctionParser<B3IRGenerator>::ControlEntry>& controlStack, const ExpressionList& expressionStack);
> 
>     void setErrorMessage(String&&) { UNREACHABLE_FOR_PLATFORM(); }
> 
>@@ -643,17 +645,29 @@ void B3IRGenerator::unifyValuesWithBlock(const ExpressionList& resultStack, Resu
>         unify(result[i], resultStack[i]);
> }
> 
>-void B3IRGenerator::dump(const Vector<ControlType>& controlStack, const ExpressionList& expressionStack)
>+static void dumpExpressionStack(const CommaPrinter& comma, const B3IRGenerator::ExpressionList& expressionStack)
>+{
>+    dataLogLn(comma, "ExpressionStack:");
>+    for (const auto& expression : expressionStack)
>+        dataLogLn(comma, *expression);
>+}
>+
>+void B3IRGenerator::dump(const Vector<FunctionParser<B3IRGenerator>::ControlEntry>& controlStack, const ExpressionList& expressionStack)
> {
>     dataLogLn("Processing Graph:");
>     dataLog(m_proc);
>     dataLogLn("With current block:", *m_currentBlock);
>     dataLogLn("Control stack:");
>-    for (const ControlType& data : controlStack)
>-        dataLogLn("  ", data);
>-    dataLogLn("ExpressionStack:");
>-    for (const ExpressionType& expression : expressionStack)
>-        dataLogLn("  ", *expression);
>+    for (auto& data : controlStack) {
>+        dataLogLn("  ", data.controlData);
>+        if (data.expressionStack.size()) {
>+            CommaPrinter comma("    ", "  with ");
>+            dumpExpressionStack(comma, data.expressionStack);
>+        }
>+    }
>+
>+    CommaPrinter comma("  ", "");
>+    dumpExpressionStack(comma, expressionStack);
>     dataLogLn("\n");
> }
> 
>diff --git a/Source/JavaScriptCore/wasm/WasmFunctionParser.h b/Source/JavaScriptCore/wasm/WasmFunctionParser.h
>index 4238696564b1623d0ba69c017973d0d0c7d8762f..e8fb9a8cfd390acc1c2a75a573ff3ce837dcad61 100644
>--- a/Source/JavaScriptCore/wasm/WasmFunctionParser.h
>+++ b/Source/JavaScriptCore/wasm/WasmFunctionParser.h
>@@ -43,24 +43,32 @@ class FunctionParser : public Parser {
> public:
>     typedef typename Context::ExpressionType ExpressionType;
>     typedef typename Context::ControlType ControlType;
>+    typedef typename Context::ExpressionList ExpressionList;
> 
>     FunctionParser(Context&, const uint8_t* functionStart, size_t functionLength, const Signature*, const Vector<FunctionInformation>& functions);
> 
>     bool WARN_UNUSED_RETURN parse();
> 
>+    struct ControlEntry {
>+        // This is the expression stack for the enclosing scope.
>+        ExpressionList expressionStack;
>+        ControlType controlData;
>+    };
>+
> private:
>-    static const bool verbose = false;
>+    static const bool verbose = true;
> 
>     bool WARN_UNUSED_RETURN parseBlock();
>     bool WARN_UNUSED_RETURN parseExpression(OpType);
>     bool WARN_UNUSED_RETURN parseUnreachableExpression(OpType);
>+    bool WARN_UNUSED_RETURN addReturn();
>     bool WARN_UNUSED_RETURN unifyControl(Vector<ExpressionType>&, unsigned level);
> 
>     bool WARN_UNUSED_RETURN popExpressionStack(ExpressionType& result);
> 
>     Context& m_context;
>-    Vector<ExpressionType, 1> m_expressionStack;
>-    Vector<ControlType> m_controlStack;
>+    ExpressionList m_expressionStack;
>+    Vector<ControlEntry> m_controlStack;
>     const Signature* m_signature;
>     const Vector<FunctionInformation>& m_functions;
>     unsigned m_unreachableBlocks { 0 };
>@@ -120,7 +128,7 @@ bool FunctionParser<Context>::parseBlock()
>         }
> 
>         if (op == OpType::End && !m_controlStack.size())
>-            break;
>+            return addReturn();
> 
>         if (m_unreachableBlocks) {
>             if (!parseUnreachableExpression(static_cast<OpType>(op))) {
>@@ -136,9 +144,24 @@ bool FunctionParser<Context>::parseBlock()
> 
>     }
> 
>-    // I'm not sure if we should check the expression stack here...
>-    return true;
>+    RELEASE_ASSERT_NOT_REACHED();
>+}
>+
>+template<typename Context>
>+bool FunctionParser<Context>::addReturn()
>+{
>+    ExpressionList returnValues;
>+    if (m_signature->returnType != Void) {
>+        ExpressionType returnValue;
>+        if (!popExpressionStack(returnValue))
>+            return false;
>+        returnValues.append(returnValue);
>+    }
>+
>+    m_unreachableBlocks = 1;
>+    return m_context.addReturn(returnValues);
> }
>+
> #define CREATE_CASE(name, id, b3op) case name:
> 
> template<typename Context>
>@@ -288,7 +311,8 @@ bool FunctionParser<Context>::parseExpression(OpType op)
>         if (!parseValueType(inlineSignature))
>             return false;
> 
>-        m_controlStack.append(m_context.addBlock(inlineSignature));
>+        m_controlStack.append({ WTFMove(m_expressionStack), m_context.addBlock(inlineSignature) });
>+        m_expressionStack = ExpressionList();
>         return true;
>     }
> 
>@@ -297,7 +321,8 @@ bool FunctionParser<Context>::parseExpression(OpType op)
>         if (!parseValueType(inlineSignature))
>             return false;
> 
>-        m_controlStack.append(m_context.addLoop(inlineSignature));
>+        m_controlStack.append({ WTFMove(m_expressionStack), m_context.addLoop(inlineSignature) });
>+        m_expressionStack = ExpressionList();
>         return true;
>     }
> 
>@@ -314,7 +339,8 @@ bool FunctionParser<Context>::parseExpression(OpType op)
>         if (!m_context.addIf(condition, inlineSignature, control))
>             return false;
> 
>-        m_controlStack.append(control);
>+        m_controlStack.append({ WTFMove(m_expressionStack), control });
>+        m_expressionStack = ExpressionList();
>         return true;
>     }
> 
>@@ -323,7 +349,11 @@ bool FunctionParser<Context>::parseExpression(OpType op)
>             m_context.setErrorMessage("Attempted to use else block at the top-level of a function");
>             return false;
>         }
>-        return m_context.addElse(m_controlStack.last(), m_expressionStack);
>+
>+        if (!m_context.addElse(m_controlStack.last().controlData, m_expressionStack))
>+            return false;
>+        m_expressionStack.shrink(m_expressionStack.size());
>+        return true;
>     }
> 
>     case OpType::Br:
>@@ -339,26 +369,21 @@ bool FunctionParser<Context>::parseExpression(OpType op)
>         } else
>             m_unreachableBlocks = 1;
> 
>-        ControlType& data = m_controlStack[m_controlStack.size() - 1 - target];
>+        ControlType& data = m_controlStack[m_controlStack.size() - 1 - target].controlData;
> 
>         return m_context.addBranch(data, condition, m_expressionStack);
>     }
> 
>     case OpType::Return: {
>-        Vector<ExpressionType, 1> returnValues;
>-        if (m_signature->returnType != Void) {
>-            ExpressionType returnValue;
>-            if (!popExpressionStack(returnValue))
>-            returnValues.append(returnValue);
>-        }
>-
>-        m_unreachableBlocks = 1;
>-        return m_context.addReturn(returnValues);
>+        return addReturn();
>     }
> 
>     case OpType::End: {
>-        ControlType data = m_controlStack.takeLast();
>-        return m_context.endBlock(data, m_expressionStack);
>+        ControlEntry data = m_controlStack.takeLast();
>+        if (!m_context.endBlock(data.controlData, m_expressionStack))
>+            return false;
>+        m_expressionStack.swap(data.expressionStack);
>+        return true;
>     }
> 
>     case OpType::Unreachable:
>@@ -386,17 +411,20 @@ bool FunctionParser<Context>::parseUnreachableExpression(OpType op)
>         if (m_unreachableBlocks > 1)
>             return true;
> 
>-        ControlType& data = m_controlStack.last();
>-        ASSERT(data.type() == BlockType::If);
>+        ControlEntry& data = m_controlStack.last();
>         m_unreachableBlocks = 0;
>-        return m_context.addElse(data, m_expressionStack);
>+        if (!m_context.addElse(data.controlData, m_expressionStack))
>+            return false;
>+        m_expressionStack.shrink(m_expressionStack.size());
>+        return true;
>     }
> 
>     case OpType::End: {
>         if (m_unreachableBlocks == 1) {
>-            ControlType data = m_controlStack.takeLast();
>-            if (!m_context.isContinuationReachable(data))
>+            ControlEntry data = m_controlStack.takeLast();
>+            if (!m_context.isContinuationReachable(data.controlData))
>                 return true;
>+            m_expressionStack.swap(data.expressionStack);
>         }
>         m_unreachableBlocks--;
>         return true;
>@@ -439,12 +467,12 @@ bool FunctionParser<Context>::parseUnreachableExpression(OpType op)
> template<typename Context>
> bool FunctionParser<Context>::popExpressionStack(ExpressionType& result)
> {
>-    if (!m_expressionStack.size()) {
>+    if (m_expressionStack.size()) {
>         result = m_expressionStack.takeLast();
>         return true;
>     }
> 
>-    m_context.setErrorMessage("Attempted to use an stack value when none existed");
>+    m_context.setErrorMessage("Attempted to use a stack value when none existed");
>     return false;
> }
> 
>diff --git a/Source/JavaScriptCore/wasm/WasmValidate.cpp b/Source/JavaScriptCore/wasm/WasmValidate.cpp
>index b809cdd068b181a1bfe968d8a098e15f1899de0f..aa8489f74d5a3bb6888c9149015618141e2ede84 100644
>--- a/Source/JavaScriptCore/wasm/WasmValidate.cpp
>+++ b/Source/JavaScriptCore/wasm/WasmValidate.cpp
>@@ -30,6 +30,7 @@
> 
> #include "WasmFunctionParser.h"
> #include <wtf/text/StringBuilder.h>
>+#include <wtf/CommaPrinter.h>
> 
> namespace JSC { namespace Wasm {
> 
>@@ -62,6 +63,8 @@ public:
>             }
>         }
> 
>+        bool hasNonVoidSignature() const { return m_signature != Void; }
>+
>         BlockType type() const { return m_blockType; }
>         Type signature() const { return m_signature; }
>     private:
>@@ -102,7 +105,7 @@ public:
>     bool WARN_UNUSED_RETURN addCall(unsigned calleeIndex, const FunctionInformation&, const Vector<ExpressionType>& args, ExpressionType& result);
>     bool WARN_UNUSED_RETURN isContinuationReachable(ControlData&) { return true; }
> 
>-    void dump(const Vector<ControlType>& controlStack, const ExpressionList& expressionStack);
>+    void dump(const Vector<FunctionParser<Validate>::ControlEntry>& controlStack, const ExpressionList& expressionStack);
> 
>     void setErrorMessage(String&& message) { ASSERT(m_errorMessage.isNull()); m_errorMessage = WTFMove(message); }
>     String errorMessage() const { return m_errorMessage; }
>@@ -288,9 +291,27 @@ bool Validate::unify(const ExpressionList& values, const ControlType& block)
>     return false;
> }
> 
>-void Validate::dump(const Vector<ControlType>&, const ExpressionList&)
>+static void dumpExpressionStack(const CommaPrinter& comma, const Validate::ExpressionList& expressionStack)
>+{
>+    dataLogLn(comma, "ExpressionStack:");
>+    for (const auto& expression : expressionStack)
>+        dataLogLn(comma, toString(expression));
>+}
>+
>+void Validate::dump(const Vector<FunctionParser<Validate>::ControlEntry>& controlStack, const ExpressionList& expressionStack)
> {
>-    dataLogLn("Validating");
>+    dataLogLn("Control stack:");
>+    for (auto& data : controlStack) {
>+        dataLogLn("  ", data.controlData);
>+        if (data.expressionStack.size()) {
>+            CommaPrinter comma("    ", "  with ");
>+            dumpExpressionStack(comma, data.expressionStack);
>+        }
>+    }
>+
>+    CommaPrinter comma("  ", "");
>+    dumpExpressionStack(comma, expressionStack);
>+    dataLogLn("\n");
> }
> 
> String validateFunction(const uint8_t* source, size_t length, const Signature* signature, const Vector<FunctionInformation>& functions)

Actions: View | Formatted Diff | Diff

Attachments on bug 164100: 293693 | 293753 | 293771 | 293786 | 293796