From 1a20c13b5d315c00eac28b5a8c08a21e52fd38ae Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Wed, 12 Jul 2017 10:44:13 -0700 Subject: [PATCH 1/5] Refactor validation failure and printing, validate atomic alignment --- src/wasm-validator.h | 57 ++++++++++++++++++++++--------------- src/wasm/wasm-validator.cpp | 48 ++++++++++++++++++++++++------- 2 files changed, 72 insertions(+), 33 deletions(-) diff --git a/src/wasm-validator.h b/src/wasm-validator.h index 403a057fe07..b65402de082 100644 --- a/src/wasm-validator.h +++ b/src/wasm-validator.h @@ -38,12 +38,26 @@ #define wasm_wasm_validator_h #include +#include #include "wasm.h" #include "wasm-printing.h" namespace wasm { +// Print anything that can be streamed to an ostream +template +std::ostream& printModuleComponent(T curr, std::ostream& stream) { + stream << curr << std::endl; + return stream; +} +// Specialization for Expressions to print type info too +template <> +inline std::ostream& printModuleComponent(Expression* curr, std::ostream& stream) { + WasmPrinter::printExpression(curr, stream, false, true) << std::endl; + return stream; +} + struct WasmValidator : public PostWalker { bool valid = true; @@ -123,6 +137,8 @@ struct WasmValidator : public PostWalker { void visitSetLocal(SetLocal *curr); void visitLoad(Load *curr); void visitStore(Store *curr); + void visitAtomicRMW(AtomicRMW *curr); + void visitAtomicCmpxchg(AtomicCmpxchg *curr); void visitBinary(Binary *curr); void visitUnary(Unary *curr); void visitSelect(Select* curr); @@ -144,12 +160,14 @@ struct WasmValidator : public PostWalker { // helpers private: - std::ostream& fail(); + template + std::ostream& fail(T curr, S text); + std::ostream& printFailureHeader(); + template bool shouldBeTrue(bool result, T curr, const char* text) { if (!result) { - fail() << "unexpected false: " << text << ", on \n" << curr << std::endl; - valid = false; + fail(curr, "unexpected false: " + std::string(text)); return false; } return result; @@ -157,8 +175,7 @@ struct WasmValidator : public PostWalker { template bool shouldBeFalse(bool result, T curr, const char* text) { if (result) { - fail() << "unexpected true: " << text << ", on \n" << curr << std::endl; - valid = false; + fail(curr, "unexpected true: " + std::string(text)); return false; } return result; @@ -167,18 +184,9 @@ struct WasmValidator : public PostWalker { template bool shouldBeEqual(S left, S right, T curr, const char* text) { if (left != right) { - fail() << "" << left << " != " << right << ": " << text << ", on \n"; - WasmPrinter::printExpression(curr, std::cerr, false, true) << std::endl; - valid = false; - return false; - } - return true; - } - template - bool shouldBeEqual(S left, S right, T curr, U other, const char* text) { - if (left != right) { - fail() << "" << left << " != " << right << ": " << text << ", on \n" << curr << " / " << other << std::endl; - valid = false; + std::ostringstream ss; + ss << left << " != " << right << ": " << text; + fail(curr, ss.str()); return false; } return true; @@ -187,9 +195,9 @@ struct WasmValidator : public PostWalker { template bool shouldBeEqualOrFirstIsUnreachable(S left, S right, T curr, const char* text) { if (left != unreachable && left != right) { - fail() << "" << left << " != " << right << ": " << text << ", on \n"; - WasmPrinter::printExpression(curr, std::cerr, false, true) << std::endl; - valid = false; + std::ostringstream ss; + ss << left << " != " << right << ": " << text; + fail(curr, ss.str()); return false; } return true; @@ -198,14 +206,17 @@ struct WasmValidator : public PostWalker { template bool shouldBeUnequal(S left, S right, T curr, const char* text) { if (left == right) { - fail() << "" << left << " == " << right << ": " << text << ", on \n" << curr << std::endl; - valid = false; + std::ostringstream ss; + ss << left << " == " << right << ": " << text; + fail(curr, ss.str()); return false; } return true; } - void validateAlignment(size_t align, WasmType type, Index bytes); + void validateAlignment(size_t align, WasmType type, Index bytes, bool isAtomic, + Expression* curr); + void validateMemBytes(uint8_t bytes, Expression* curr); void validateBinaryenIR(Module& wasm); }; diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp index 9421070e7bd..f6018a16ab3 100644 --- a/src/wasm/wasm-validator.cpp +++ b/src/wasm/wasm-validator.cpp @@ -214,15 +214,32 @@ void WasmValidator::visitSetLocal(SetLocal *curr) { } } void WasmValidator::visitLoad(Load *curr) { - validateAlignment(curr->align, curr->type, curr->bytes); + validateAlignment(curr->align, curr->type, curr->bytes, curr->isAtomic, curr); shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "load pointer type must be i32"); } void WasmValidator::visitStore(Store *curr) { - validateAlignment(curr->align, curr->type, curr->bytes); + validateAlignment(curr->align, curr->type, curr->bytes, curr->isAtomic, curr); shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "store pointer type must be i32"); shouldBeUnequal(curr->value->type, none, curr, "store value type must not be none"); shouldBeEqualOrFirstIsUnreachable(curr->value->type, curr->valueType, curr, "store value type must match"); } +void WasmValidator::visitAtomicRMW(AtomicRMW* curr) { + //validateAlignment(curr-> +} +void WasmValidator::visitAtomicCmpxchg(AtomicCmpxchg* curr) { +} +void WasmValidator::validateMemBytes(uint8_t bytes, Expression* curr) { + switch (bytes) { + case 1: + case 2: + case 4: + break; + case 8: + shouldBeEqual(getWasmTypeSize(curr->type), 8U, curr, "8-byte mem operations are only allowed with 8-byte wasm types"); + break; + default: fail("Memory operations must be 1,2,4, or 8 bytes", curr); + } +} void WasmValidator::visitBinary(Binary *curr) { if (curr->left->type != unreachable && curr->right->type != unreachable) { shouldBeEqual(curr->left->type, curr->right->type, curr, "binary child types must be equal"); @@ -561,28 +578,32 @@ void WasmValidator::visitModule(Module *curr) { } } -void WasmValidator::validateAlignment(size_t align, WasmType type, Index bytes) { +void WasmValidator::validateAlignment(size_t align, WasmType type, Index bytes, + bool isAtomic, Expression* curr) { + if (isAtomic) { + shouldBeEqual(align, (size_t)bytes, curr, "atomic accesses must have natural alignment"); + return; + } switch (align) { case 1: case 2: case 4: case 8: break; default:{ - fail() << "bad alignment: " << align << std::endl; - valid = false; + fail("bad alignment: " + std::to_string(align), curr); break; } } - shouldBeTrue(align <= bytes, align, "alignment must not exceed natural"); + shouldBeTrue(align <= bytes, curr, "alignment must not exceed natural"); switch (type) { case i32: case f32: { - shouldBeTrue(align <= 4, align, "alignment must not exceed natural"); + shouldBeTrue(align <= 4, curr, "alignment must not exceed natural"); break; } case i64: case f64: { - shouldBeTrue(align <= 8, align, "alignment must not exceed natural"); + shouldBeTrue(align <= 8, curr, "alignment must not exceed natural"); break; } default: {} @@ -609,7 +630,7 @@ void WasmValidator::validateBinaryenIR(Module& wasm) { // The block has an added type, not derived from the ast itself, so it is // ok for it to be either i32 or unreachable. if (!(isConcreteWasmType(oldType) && newType == unreachable)) { - parent.fail() << "stale type found in " << (getFunction() ? getFunction()->name : Name("(global scope)")) << " on " << curr << "\n(marked as " << printWasmType(oldType) << ", should be " << printWasmType(newType) << ")\n"; + parent.printFailureHeader() << "stale type found in " << (getFunction() ? getFunction()->name : Name("(global scope)")) << " on " << curr << "\n(marked as " << printWasmType(oldType) << ", should be " << printWasmType(newType) << ")\n"; parent.valid = false; } curr->type = oldType; @@ -620,7 +641,14 @@ void WasmValidator::validateBinaryenIR(Module& wasm) { binaryenIRValidator.walkModule(&wasm); } -std::ostream& WasmValidator::fail() { +template +std::ostream& WasmValidator::fail(T curr, S text) { + valid = false; + auto& ret = printFailureHeader() << text << ", on \n"; + return printModuleComponent(curr, ret); +} + +std::ostream& WasmValidator::printFailureHeader() { Colors::red(std::cerr); if (getFunction()) { std::cerr << "[wasm-validator error in function "; From e96249f78c43fc3d6370c6c20c7f66adecf8869c Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Wed, 12 Jul 2017 16:19:52 -0700 Subject: [PATCH 2/5] no need for inline --- src/wasm-validator.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wasm-validator.h b/src/wasm-validator.h index b65402de082..79b579ef51e 100644 --- a/src/wasm-validator.h +++ b/src/wasm-validator.h @@ -53,7 +53,7 @@ std::ostream& printModuleComponent(T curr, std::ostream& stream) { } // Specialization for Expressions to print type info too template <> -inline std::ostream& printModuleComponent(Expression* curr, std::ostream& stream) { +std::ostream& printModuleComponent(Expression* curr, std::ostream& stream) { WasmPrinter::printExpression(curr, stream, false, true) << std::endl; return stream; } From 36c7f3367643ab8cfedab96534b43e2952f9f39b Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Wed, 12 Jul 2017 16:21:51 -0700 Subject: [PATCH 3/5] actually make inline match --- src/wasm-validator.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wasm-validator.h b/src/wasm-validator.h index 79b579ef51e..deff6e1b3a6 100644 --- a/src/wasm-validator.h +++ b/src/wasm-validator.h @@ -47,13 +47,13 @@ namespace wasm { // Print anything that can be streamed to an ostream template -std::ostream& printModuleComponent(T curr, std::ostream& stream) { +inline std::ostream& printModuleComponent(T curr, std::ostream& stream) { stream << curr << std::endl; return stream; } // Specialization for Expressions to print type info too template <> -std::ostream& printModuleComponent(Expression* curr, std::ostream& stream) { +inline std::ostream& printModuleComponent(Expression* curr, std::ostream& stream) { WasmPrinter::printExpression(curr, stream, false, true) << std::endl; return stream; } From 8c988212a9e85f374aefce45b6f72ba3c2616b4e Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Wed, 12 Jul 2017 16:34:22 -0700 Subject: [PATCH 4/5] forgot to add the calls to validateMemBytes --- src/wasm-validator.h | 2 +- src/wasm/wasm-validator.cpp | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/wasm-validator.h b/src/wasm-validator.h index deff6e1b3a6..24affdb44c8 100644 --- a/src/wasm-validator.h +++ b/src/wasm-validator.h @@ -216,7 +216,7 @@ struct WasmValidator : public PostWalker { void validateAlignment(size_t align, WasmType type, Index bytes, bool isAtomic, Expression* curr); - void validateMemBytes(uint8_t bytes, Expression* curr); + void validateMemBytes(uint8_t bytes, WasmType ty, Expression* curr); void validateBinaryenIR(Module& wasm); }; diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp index f6018a16ab3..f6275727931 100644 --- a/src/wasm/wasm-validator.cpp +++ b/src/wasm/wasm-validator.cpp @@ -214,28 +214,31 @@ void WasmValidator::visitSetLocal(SetLocal *curr) { } } void WasmValidator::visitLoad(Load *curr) { + validateMemBytes(curr->bytes, curr->type, curr); validateAlignment(curr->align, curr->type, curr->bytes, curr->isAtomic, curr); shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "load pointer type must be i32"); } void WasmValidator::visitStore(Store *curr) { + validateMemBytes(curr->bytes, curr->valueType, curr); validateAlignment(curr->align, curr->type, curr->bytes, curr->isAtomic, curr); shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "store pointer type must be i32"); shouldBeUnequal(curr->value->type, none, curr, "store value type must not be none"); shouldBeEqualOrFirstIsUnreachable(curr->value->type, curr->valueType, curr, "store value type must match"); } void WasmValidator::visitAtomicRMW(AtomicRMW* curr) { - //validateAlignment(curr-> + validateMemBytes(curr->bytes, curr->type, curr); } void WasmValidator::visitAtomicCmpxchg(AtomicCmpxchg* curr) { + validateMemBytes(curr->bytes, curr->type, curr); } -void WasmValidator::validateMemBytes(uint8_t bytes, Expression* curr) { +void WasmValidator::validateMemBytes(uint8_t bytes, WasmType ty, Expression* curr) { switch (bytes) { case 1: case 2: case 4: break; case 8: - shouldBeEqual(getWasmTypeSize(curr->type), 8U, curr, "8-byte mem operations are only allowed with 8-byte wasm types"); + shouldBeEqual(getWasmTypeSize(ty), 8U, curr, "8-byte mem operations are only allowed with 8-byte wasm types"); break; default: fail("Memory operations must be 1,2,4, or 8 bytes", curr); } From 88bf9ca298dfde994506454201f478e8cb505413 Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Wed, 12 Jul 2017 16:42:56 -0700 Subject: [PATCH 5/5] fix style --- src/wasm/wasm-validator.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp index f6275727931..77d3c415d8c 100644 --- a/src/wasm/wasm-validator.cpp +++ b/src/wasm/wasm-validator.cpp @@ -237,9 +237,10 @@ void WasmValidator::validateMemBytes(uint8_t bytes, WasmType ty, Expression* cur case 2: case 4: break; - case 8: + case 8: { shouldBeEqual(getWasmTypeSize(ty), 8U, curr, "8-byte mem operations are only allowed with 8-byte wasm types"); break; + } default: fail("Memory operations must be 1,2,4, or 8 bytes", curr); } }