Log Message
Asking for a value profile prediction should be defensive against not finding a value profile https://bugs.webkit.org/show_bug.cgi?id=164306
Patch by Saam Barati <[email protected]> on 2016-11-03 Reviewed by Mark Lam. Currently, the code that calls CodeBlock::valueProfilePredictionForBytecodeOffset in the DFG assumes it will always be at a value producing node. However, this isn't true if we tail call from an inlined setter. When we're at a tail call, we try to find the first caller that isn't a tail call to see what value the tail_call produces. If we inline a setter, however, we will end up finding the put_by_id as our first non-tail-called "caller", and that won't have a value profile associated with it since it's not a value producing node. CodeBlock::valueProfilePredictionForBytecodeOffset should be defensive against finding a null value profile. * bytecode/CodeBlock.h: (JSC::CodeBlock::valueProfilePredictionForBytecodeOffset): * dfg/DFGByteCodeParser.cpp: (JSC::DFG::ByteCodeParser::getPredictionWithoutOSRExit):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (208340 => 208341)
--- trunk/Source/_javascript_Core/ChangeLog 2016-11-03 19:31:07 UTC (rev 208340)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-11-03 20:47:57 UTC (rev 208341)
@@ -2792,6 +2792,46 @@
2016-11-02 Keith Miller <[email protected]>
+ Wasm starts a new stack whenever it adds a new block and has return types for blocks.
+ https://bugs.webkit.org/show_bug.cgi?id=164100
+
+ Reviewed by Saam Barati.
+
+ This patch overhauls much of the Wasm function parser, validator, and B3 IR generator
+ to work with block return types. In Wasm, blocks can act as expressions and have a
+ return value. Most of the control flow operators needed to be rewritten in order to
+ support this feature. To enable return types the function parser needed to be able
+ to save and restore the _expression_ stack from previous blocks, which is done via the
+ control stack.
+
+ This patch also removes the lazy continuation block system added previously. It's
+ not clear if there would be any performance win from such a system. There are likely
+ many other things with orders of magnitude more impact on B3 IR generation. The
+ complexity cost of such a system is not worth the effort without sufficient evidence
+ otherwise.
+
+ * 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::Validate::addElse):
+ (JSC::Wasm::Validate::addElseToUnreachable):
+ (JSC::Wasm::Validate::addBranch):
+ (JSC::Wasm::Validate::endBlock):
+ (JSC::Wasm::Validate::addEndToUnreachable):
+ (JSC::Wasm::Validate::dump):
+ (JSC::Wasm::validateFunction):
+ (JSC::Wasm::Validate::isContinuationReachable): Deleted.
+
+2016-11-02 Keith Miller <[email protected]>
+
We should not pop from an empty stack in the Wasm function parser.
https://bugs.webkit.org/show_bug.cgi?id=164275
Modified: trunk/Source/_javascript_Core/testWasm.cpp (208340 => 208341)
--- trunk/Source/_javascript_Core/testWasm.cpp 2016-11-03 19:31:07 UTC (rev 208340)
+++ trunk/Source/_javascript_Core/testWasm.cpp 2016-11-03 20:47:57 UTC (rev 208341)
@@ -293,10 +293,111 @@
// For now we inline the test files.
static void runWasmTests()
{
+ {
+ // Generated from:
+ // (module
+ // (func (export "if-then-both-fallthrough") (param $x i32) (param $y i32) (result i32)
+ // (block $block i32
+ // (if i32 (i32.eq (get_local $x) (i32.const 0))
+ // (then (i32.const 1))
+ // (else
+ // (i32.const 2)
+ // (br $block))
+ // )
+ // )
+ // )
+ // )
+ 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, 0x9c, 0x80,
+ 0x80, 0x80, 0x00, 0x01, 0x18, 0x69, 0x66, 0x2d, 0x74, 0x68, 0x65, 0x6e, 0x2d, 0x62, 0x6f, 0x74,
+ 0x68, 0x2d, 0x66, 0x61, 0x6c, 0x6c, 0x74, 0x68, 0x72, 0x6f, 0x75, 0x67, 0x68, 0x00, 0x00, 0x0a,
+ 0x9a, 0x80, 0x80, 0x80, 0x00, 0x01, 0x94, 0x80, 0x80, 0x80, 0x00, 0x00, 0x01, 0x01, 0x14, 0x00,
+ 0x10, 0x00, 0x4d, 0x03, 0x01, 0x10, 0x01, 0x04, 0x10, 0x02, 0x06, 0x01, 0x0f, 0x0f, 0x0f
+ };
+ Plan plan(*vm, vector);
+ checkPlan(plan, 1);
+
+ // Test this doesn't crash.
+ CHECK(isIdentical(invoke<int>(*plan.result(0)->jsEntryPoint, { boxf(0), boxf(32) }), 1));
+ CHECK(isIdentical(invoke<int>(*plan.result(0)->jsEntryPoint, { boxf(1), boxf(32) }), 2));
+ }
+
{
// Generated from:
// (module
+ // (func (export "if-then-both-fallthrough") (param $x i32) (param $y i32) (result i32)
+ // (if i32 (i32.eq (get_local $x) (i32.const 0))
+ // (then (i32.const 1))
+ // (else (i32.const 2))
+ // )
+ // )
+ // )
+ 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, 0x9c, 0x80,
+ 0x80, 0x80, 0x00, 0x01, 0x18, 0x69, 0x66, 0x2d, 0x74, 0x68, 0x65, 0x6e, 0x2d, 0x62, 0x6f, 0x74,
+ 0x68, 0x2d, 0x66, 0x61, 0x6c, 0x6c, 0x74, 0x68, 0x72, 0x6f, 0x75, 0x67, 0x68, 0x00, 0x00, 0x0a,
+ 0x95, 0x80, 0x80, 0x80, 0x00, 0x01, 0x8f, 0x80, 0x80, 0x80, 0x00, 0x00, 0x14, 0x00, 0x10, 0x00,
+ 0x4d, 0x03, 0x01, 0x10, 0x01, 0x04, 0x10, 0x02, 0x0f, 0x0f
+ };
+
+ Plan plan(*vm, vector);
+ checkPlan(plan, 1);
+
+ // Test this doesn't crash.
+ CHECK(isIdentical(invoke<int>(*plan.result(0)->jsEntryPoint, { boxf(0), boxf(32) }), 1));
+ CHECK(isIdentical(invoke<int>(*plan.result(0)->jsEntryPoint, { boxf(1), boxf(32) }), 2));
+ }
+
+ {
+ // 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 $f32-sub (export "f32-sub") (param f32) (param f32) (result f32) (return (f32.sub (get_local 0) (get_local 1))))
// (func (export "indirect-f32-sub") (param f32) (param f32) (result f32) (return (call $f32-sub (get_local 0) (get_local 1))))
// )
Modified: trunk/Source/_javascript_Core/wasm/WasmB3IRGenerator.cpp (208340 => 208341)
--- trunk/Source/_javascript_Core/wasm/WasmB3IRGenerator.cpp 2016-11-03 19:31:07 UTC (rev 208340)
+++ trunk/Source/_javascript_Core/wasm/WasmB3IRGenerator.cpp 2016-11-03 20:47:57 UTC (rev 208341)
@@ -79,43 +79,11 @@
}
class B3IRGenerator {
-private:
- class LazyBlock {
- public:
- LazyBlock(BasicBlock* block)
- : m_block(block)
- {
- }
-
- LazyBlock()
- {
- }
-
- explicit operator bool() const { return !!m_block; }
-
- BasicBlock* get(Procedure& proc)
- {
- if (!m_block)
- m_block = proc.addBlock();
- return m_block;
- }
-
- void dump(PrintStream& out) const
- {
- if (m_block)
- out.print(*m_block);
- else
- out.print("Uninitialized");
- }
-
- private:
- BasicBlock* m_block { nullptr };
- };
-
public:
struct ControlData {
- ControlData(Procedure& proc, Type signature, BasicBlock* special = nullptr, BasicBlock* continuation = nullptr)
- : continuation(continuation)
+ ControlData(Procedure& proc, Type signature, BlockType type, BasicBlock* continuation, BasicBlock* special = nullptr)
+ : blockType(type)
+ , continuation(continuation)
, special(special)
{
if (signature != Void)
@@ -139,7 +107,7 @@
out.print("Loop: ");
break;
}
- out.print("Continuation: ", continuation, ", Special: ");
+ out.print("Continuation: ", *continuation, ", Special: ");
if (special)
out.print(*special);
else
@@ -146,28 +114,28 @@
out.print("None");
}
- BlockType type() const
- {
- if (!special)
- return BlockType::Block;
- if (continuation)
- return BlockType::If;
- return BlockType::Loop;
- }
+ BlockType type() const { return blockType; }
- BasicBlock* targetBlockForBranch(Procedure& proc)
+ bool hasNonVoidSignature() const { return result.size(); }
+
+ BasicBlock* targetBlockForBranch()
{
if (type() == BlockType::Loop)
return special;
- return continuation.get(proc);
+ return continuation;
}
+ void convertIfToBlock()
+ {
+ ASSERT(type() == BlockType::If);
+ blockType = BlockType::Block;
+ special = nullptr;
+ }
+
private:
friend class B3IRGenerator;
- // We use a LazyBlock for the continuation since B3::validate does not like orphaned blocks. Note,
- // it's possible to create an orphaned block by doing something like (block (return (...))). In
- // that example, if we eagerly allocate a BasicBlock for the continuation it will never be reachable.
- LazyBlock continuation;
+ BlockType blockType;
+ BasicBlock* continuation;
BasicBlock* special;
Vector<Variable*, 1> result;
};
@@ -176,6 +144,8 @@
typedef ControlData ControlType;
typedef Vector<ExpressionType, 1> ExpressionList;
typedef Vector<Variable*, 1> ResultList;
+ typedef FunctionParser<B3IRGenerator>::ControlEntry ControlEntry;
+
static constexpr ExpressionType emptyExpression = nullptr;
B3IRGenerator(Memory*, Procedure&, Vector<UnlinkedCall>& unlinkedCalls);
@@ -201,17 +171,17 @@
ControlData WARN_UNUSED_RETURN addLoop(Type signature);
bool WARN_UNUSED_RETURN addIf(ExpressionType condition, Type signature, ControlData& result);
bool WARN_UNUSED_RETURN addElse(ControlData&, const ExpressionList&);
+ bool WARN_UNUSED_RETURN addElseToUnreachable(ControlData&);
bool WARN_UNUSED_RETURN addReturn(const ExpressionList& returnValues);
bool WARN_UNUSED_RETURN addBranch(ControlData&, ExpressionType condition, const ExpressionList& returnValues);
- bool WARN_UNUSED_RETURN endBlock(ControlData&, ExpressionList& expressionStack);
+ bool WARN_UNUSED_RETURN endBlock(ControlEntry&, ExpressionList& expressionStack);
+ bool WARN_UNUSED_RETURN addEndToUnreachable(ControlEntry&);
bool WARN_UNUSED_RETURN addCall(unsigned calleeIndex, const FunctionInformation&, Vector<ExpressionType>& args, ExpressionType& result);
- bool isContinuationReachable(ControlData&);
+ void dump(const Vector<ControlEntry>& controlStack, const ExpressionList& expressionStack);
- void dump(const Vector<ControlType>& controlStack, const ExpressionList& expressionStack);
-
void setErrorMessage(String&&) { UNREACHABLE_FOR_PLATFORM(); }
private:
@@ -498,16 +468,17 @@
B3IRGenerator::ControlData B3IRGenerator::addBlock(Type signature)
{
- return ControlData(m_proc, signature);
+ return ControlData(m_proc, signature, BlockType::Block, m_proc.addBlock());
}
B3IRGenerator::ControlData B3IRGenerator::addLoop(Type signature)
{
BasicBlock* body = m_proc.addBlock();
+ BasicBlock* continuation = m_proc.addBlock();
m_currentBlock->appendNewControlValue(m_proc, Jump, Origin(), body);
body->addPredecessor(m_currentBlock);
m_currentBlock = body;
- return ControlData(m_proc, signature, body);
+ return ControlData(m_proc, signature, BlockType::Loop, continuation, body);
}
bool B3IRGenerator::addIf(ExpressionType condition, Type signature, ControlType& result)
@@ -524,17 +495,22 @@
notTaken->addPredecessor(m_currentBlock);
m_currentBlock = taken;
- result = ControlData(m_proc, signature, notTaken, continuation);
+ result = ControlData(m_proc, signature, BlockType::If, continuation, notTaken);
return true;
}
-bool B3IRGenerator::addElse(ControlData& data, const ExpressionList&)
+bool B3IRGenerator::addElse(ControlData& data, const ExpressionList& currentStack)
{
- ASSERT(data.continuation);
+ unifyValuesWithBlock(currentStack, data.result);
+ m_currentBlock->appendNewControlValue(m_proc, Jump, Origin(), data.continuation);
+ return addElseToUnreachable(data);
+}
+
+bool B3IRGenerator::addElseToUnreachable(ControlData& data)
+{
+ ASSERT(data.type() == BlockType::If);
m_currentBlock = data.special;
- // Clear the special pointer so that when we parse the end we don't think that this block is an if block.
- data.special = nullptr;
- ASSERT(data.type() == BlockType::Block);
+ data.convertIfToBlock();
return true;
}
@@ -550,8 +526,10 @@
bool B3IRGenerator::addBranch(ControlData& data, ExpressionType condition, const ExpressionList& returnValues)
{
- BasicBlock* target = data.targetBlockForBranch(m_proc);
- unifyValuesWithBlock(returnValues, data.result);
+ if (data.type() != BlockType::Loop)
+ unifyValuesWithBlock(returnValues, data.result);
+
+ BasicBlock* target = data.targetBlockForBranch();
if (condition) {
BasicBlock* continuation = m_proc.addBlock();
m_currentBlock->appendNew<Value>(m_proc, B3::Branch, Origin(), condition);
@@ -567,24 +545,31 @@
return true;
}
-bool B3IRGenerator::endBlock(ControlData& data, ExpressionList& expressionStack)
+bool B3IRGenerator::endBlock(ControlEntry& entry, ExpressionList& expressionStack)
{
- if (!data.continuation)
- return true;
+ ControlData& data = ""
- BasicBlock* continuation = data.continuation.get(m_proc);
+ unifyValuesWithBlock(expressionStack, data.result);
+ m_currentBlock->appendNewControlValue(m_proc, Jump, Origin(), data.continuation);
+ data.continuation->addPredecessor(m_currentBlock);
+
+ return addEndToUnreachable(entry);
+}
+
+
+bool B3IRGenerator::addEndToUnreachable(ControlEntry& entry)
+{
+ ControlData& data = ""
+ m_currentBlock = data.continuation;
+
if (data.type() == BlockType::If) {
- ASSERT(!data.special->size() && !data.special->successors().size());
- // Since we don't have any else block we need to point the notTaken branch to the continuation.
- data.special->appendNewControlValue(m_proc, Jump, Origin());
- data.special->setSuccessors(FrequentedBlock(continuation));
- continuation->addPredecessor(data.special);
+ data.special->appendNewControlValue(m_proc, Jump, Origin(), m_currentBlock);
+ m_currentBlock->addPredecessor(data.special);
}
- unifyValuesWithBlock(expressionStack, data.result);
- m_currentBlock->appendNewControlValue(m_proc, Jump, Origin(), continuation);
- continuation->addPredecessor(m_currentBlock);
- m_currentBlock = continuation;
+ for (Variable* result : data.result)
+ entry.enclosedExpressionStack.append(m_currentBlock->appendNew<VariableValue>(m_proc, B3::Get, Origin(), result));
+
return true;
}
@@ -614,22 +599,6 @@
return true;
}
-bool B3IRGenerator::isContinuationReachable(ControlData& data)
-{
- // If nothing targets the continuation of the current block then we don't want to create
- // an orphaned BasicBlock since it can't be reached by fallthrough.
- if (!data.continuation)
- return false;
-
- m_currentBlock = data.continuation.get(m_proc);
- if (data.type() == BlockType::If) {
- data.special->appendNewControlValue(m_proc, Jump, Origin(), m_currentBlock);
- m_currentBlock->addPredecessor(data.special);
- }
-
- return true;
-}
-
void B3IRGenerator::unify(Variable* variable, ExpressionType source)
{
m_currentBlock->appendNew<VariableValue>(m_proc, Set, Origin(), variable, source);
@@ -637,23 +606,35 @@
void B3IRGenerator::unifyValuesWithBlock(const ExpressionList& resultStack, ResultList& result)
{
- ASSERT(result.size() >= resultStack.size());
+ ASSERT(result.size() <= resultStack.size());
- for (size_t i = 0; i < resultStack.size(); ++i)
- unify(result[i], resultStack[i]);
+ for (size_t i = 0; i < result.size(); ++i)
+ unify(result[result.size() - 1 - i], resultStack[resultStack.size() - 1 - 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<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.enclosedExpressionStack.size()) {
+ CommaPrinter comma(" ", " with ");
+ dumpExpressionStack(comma, data.enclosedExpressionStack);
+ }
+ }
+
+ CommaPrinter comma(" ", "");
+ dumpExpressionStack(comma, expressionStack);
dataLogLn("\n");
}
Modified: trunk/Source/_javascript_Core/wasm/WasmFunctionParser.h (208340 => 208341)
--- trunk/Source/_javascript_Core/wasm/WasmFunctionParser.h 2016-11-03 19:31:07 UTC (rev 208340)
+++ trunk/Source/_javascript_Core/wasm/WasmFunctionParser.h 2016-11-03 20:47:57 UTC (rev 208341)
@@ -43,11 +43,17 @@
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 {
+ ExpressionList enclosedExpressionStack;
+ ControlType controlData;
+ };
+
private:
static const bool verbose = false;
@@ -54,13 +60,14 @@
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 +127,7 @@
}
if (op == OpType::End && !m_controlStack.size())
- break;
+ return m_unreachableBlocks ? true : addReturn();
if (m_unreachableBlocks) {
if (!parseUnreachableExpression(static_cast<OpType>(op))) {
@@ -136,9 +143,24 @@
}
- // 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 +310,8 @@
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 +320,8 @@
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 +338,8 @@
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 +348,11 @@
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(0);
+ return true;
}
case OpType::Br:
@@ -339,29 +368,31 @@
} else
m_unreachableBlocks = 1;
- ControlType& data = "" - 1 - target];
+ ControlType& data = "" - 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 = ""
- return m_context.endBlock(data, m_expressionStack);
+ ControlEntry data = ""
+ // FIXME: This is a little weird in that it will modify the expressionStack for the result of the block.
+ // That's a little too effectful for me but I don't have a better API right now.
+ // see: https://bugs.webkit.org/show_bug.cgi?id=164353
+ if (!m_context.endBlock(data, m_expressionStack))
+ return false;
+ m_expressionStack.swap(data.enclosedExpressionStack);
+ return true;
}
- case OpType::Unreachable:
+ case OpType::Unreachable: {
+ m_unreachableBlocks = 1;
+ return true;
+ }
+
case OpType::Select:
case OpType::BrTable:
case OpType::Nop:
@@ -386,17 +417,20 @@
if (m_unreachableBlocks > 1)
return true;
- ControlType& data = ""
- ASSERT(data.type() == BlockType::If);
+ ControlEntry& data = ""
m_unreachableBlocks = 0;
- return m_context.addElse(data, m_expressionStack);
+ if (!m_context.addElseToUnreachable(data.controlData))
+ return false;
+ m_expressionStack.shrink(0);
+ return true;
}
case OpType::End: {
if (m_unreachableBlocks == 1) {
- ControlType data = ""
- if (!m_context.isContinuationReachable(data))
- return true;
+ ControlEntry data = ""
+ if (!m_context.addEndToUnreachable(data))
+ return false;
+ m_expressionStack.swap(data.enclosedExpressionStack);
}
m_unreachableBlocks--;
return true;
@@ -439,12 +473,12 @@
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;
}
Modified: trunk/Source/_javascript_Core/wasm/WasmValidate.cpp (208340 => 208341)
--- trunk/Source/_javascript_Core/wasm/WasmValidate.cpp 2016-11-03 19:31:07 UTC (rev 208340)
+++ trunk/Source/_javascript_Core/wasm/WasmValidate.cpp 2016-11-03 20:47:57 UTC (rev 208341)
@@ -29,6 +29,7 @@
#if ENABLE(WEBASSEMBLY)
#include "WasmFunctionParser.h"
+#include <wtf/CommaPrinter.h>
#include <wtf/text/StringBuilder.h>
namespace JSC { namespace Wasm {
@@ -62,6 +63,8 @@
}
}
+ bool hasNonVoidSignature() const { return m_signature != Void; }
+
BlockType type() const { return m_blockType; }
Type signature() const { return m_signature; }
private:
@@ -71,6 +74,8 @@
typedef Type ExpressionType;
typedef ControlData ControlType;
typedef Vector<ExpressionType, 1> ExpressionList;
+ typedef FunctionParser<Validate>::ControlEntry ControlEntry;
+
static const ExpressionType emptyExpression = Void;
bool WARN_UNUSED_RETURN addArguments(const Vector<Type>&);
@@ -94,15 +99,17 @@
ControlData WARN_UNUSED_RETURN addLoop(Type signature);
bool WARN_UNUSED_RETURN addIf(ExpressionType condition, Type signature, ControlData& result);
bool WARN_UNUSED_RETURN addElse(ControlData&, const ExpressionList&);
+ bool WARN_UNUSED_RETURN addElseToUnreachable(ControlData&);
bool WARN_UNUSED_RETURN addReturn(const ExpressionList& returnValues);
bool WARN_UNUSED_RETURN addBranch(ControlData&, ExpressionType condition, const ExpressionList& returnValues);
- bool WARN_UNUSED_RETURN endBlock(ControlData&, ExpressionList& expressionStack);
+ bool WARN_UNUSED_RETURN endBlock(ControlEntry&, ExpressionList& expressionStack);
+ bool WARN_UNUSED_RETURN addEndToUnreachable(ControlEntry&);
+
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<ControlEntry>& controlStack, const ExpressionList& expressionStack);
void setErrorMessage(String&& message) { ASSERT(m_errorMessage.isNull()); m_errorMessage = WTFMove(message); }
String errorMessage() const { return m_errorMessage; }
@@ -184,13 +191,18 @@
bool Validate::addElse(ControlType& current, const ExpressionList& values)
{
- if (current.type() != BlockType::If) {
- m_errorMessage = makeString("Attempting to add else block to something other than an if");
+ if (!unify(values, current)) {
+ ASSERT(errorMessage());
return false;
}
- if (!unify(values, current)) {
- ASSERT(errorMessage());
+ return addElseToUnreachable(current);
+}
+
+bool Validate::addElseToUnreachable(ControlType& current)
+{
+ if (current.type() != BlockType::If) {
+ m_errorMessage = makeString("Attempting to add else block to something other than an if");
return false;
}
@@ -219,7 +231,7 @@
return false;
}
- if (target.type() == BlockType::If)
+ if (target.type() == BlockType::Loop)
return true;
if (target.signature() == Void)
@@ -233,20 +245,33 @@
return false;
}
-bool Validate::endBlock(ControlType& block, ExpressionList& stack)
+bool Validate::endBlock(ControlEntry& entry, ExpressionList& stack)
{
+ ControlData& block = entry.controlData;
if (block.signature() == Void)
return true;
+ if (!stack.size()) {
+ m_errorMessage = makeString("Block fallthough expected type: ", toString(block.signature()), " but the stack was empty");
+ return false;
+ }
- ASSERT(stack.size() == 1);
- if (block.signature() == stack[0])
+ if (block.signature() == stack.last()) {
+ entry.enclosedExpressionStack.append(block.signature());
return true;
+ }
m_errorMessage = makeString("Block fallthrough has expected type: ", toString(block.signature()), " but produced type: ", toString(block.signature()));
return false;
}
+bool Validate::addEndToUnreachable(ControlEntry& entry)
+{
+ if (entry.controlData.signature() != Void)
+ entry.enclosedExpressionStack.append(entry.controlData.signature());
+ return true;
+}
+
bool Validate::addCall(unsigned, const FunctionInformation& info, const Vector<ExpressionType>& args, ExpressionType& result)
{
if (info.signature->arguments.size() != args.size()) {
@@ -288,9 +313,10 @@
return false;
}
-void Validate::dump(const Vector<ControlType>&, const ExpressionList&)
+void Validate::dump(const Vector<ControlEntry>&, const ExpressionList&)
{
- dataLogLn("Validating");
+ // If you need this then you should fix the validator's error messages instead...
+ // Think of this as penance for the sin of bad error messages.
}
String validateFunction(const uint8_t* source, size_t length, const Signature* signature, const Vector<FunctionInformation>& functions)
@@ -299,6 +325,10 @@
FunctionParser<Validate> validator(context, source, length, signature, functions);
if (!validator.parse()) {
// FIXME: add better location information here. see: https://bugs.webkit.org/show_bug.cgi?id=164288
+ // FIXME: We should never not have an error message if we return false.
+ // see: https://bugs.webkit.org/show_bug.cgi?id=164354
+ if (context.errorMessage().isNull())
+ return "Unknown error";
return context.errorMessage();
}
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
