From ce652932b102e5a74d77c63b01ae74f1dcac9533 Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Thu, 13 Jul 2017 14:48:28 -0700 Subject: [PATCH 1/8] validate AtomicRMW and cmpxchg --- src/wasm/wasm-validator.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp index c1f44a68b41..77e5278a0f7 100644 --- a/src/wasm/wasm-validator.cpp +++ b/src/wasm/wasm-validator.cpp @@ -233,9 +233,31 @@ void WasmValidator::visitStore(Store *curr) { } void WasmValidator::visitAtomicRMW(AtomicRMW* curr) { validateMemBytes(curr->bytes, curr->type, curr); + shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "AtomicRMW pointer type must be i32"); + shouldBeEqualOrFirstIsUnreachable(curr->value->type, curr->type, curr, "AtomicRMW result type must match operand"); + switch (curr->type) { + case i32: + case i64: + case unreachable: + break; + default: fail("Atomic operations are only valid on int types", curr); + } } void WasmValidator::visitAtomicCmpxchg(AtomicCmpxchg* curr) { validateMemBytes(curr->bytes, curr->type, curr); + shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "cmpxchg pointer type must be i32"); + if (curr->expected->type != unreachable && curr->replacement->type != unreachable) { + shouldBeEqual(curr->expected->type, curr->replacement->type, curr, "cmpxchg operand types must match"); + } + shouldBeEqualOrFirstIsUnreachable(curr->expected->type, curr->type, curr, "Cmpxchg result type must match expected"); + shouldBeEqualOrFirstIsUnreachable(curr->replacement->type, curr->type, curr, "Cmpxchg result type must match replacement"); + switch (curr->expected->type) { + case i32: + case i64: + case unreachable: + break; + default: fail("Atomic operations are only valid on int types", curr); + } } void WasmValidator::validateMemBytes(uint8_t bytes, WasmType ty, Expression* curr) { switch (bytes) { From d2ddc43bf9b4a3f2f28a314faa3a095a37baf9ae Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Thu, 13 Jul 2017 16:32:13 -0700 Subject: [PATCH 2/8] lol templates --- src/wasm-validator.h | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/wasm-validator.h b/src/wasm-validator.h index 2ec530476e0..492b2c6bcce 100644 --- a/src/wasm-validator.h +++ b/src/wasm-validator.h @@ -46,13 +46,15 @@ namespace wasm { // Print anything that can be streamed to an ostream -template + template ::type>::value + >::type* = nullptr> inline std::ostream& printModuleComponent(T curr, std::ostream& stream) { stream << curr << std::endl; return stream; } -// Specialization for Expressions to print type info too -template <> +// Extra overload for Expressions, to print type info too inline std::ostream& printModuleComponent(Expression* curr, std::ostream& stream) { WasmPrinter::printExpression(curr, stream, false, true) << std::endl; return stream; @@ -162,13 +164,13 @@ struct WasmValidator : public PostWalker { // helpers private: template - std::ostream& fail(T curr, S text); + std::ostream& fail(S text, T curr); std::ostream& printFailureHeader(); template bool shouldBeTrue(bool result, T curr, const char* text) { if (!result) { - fail(curr, "unexpected false: " + std::string(text)); + fail("unexpected false: " + std::string(text), curr); return false; } return result; @@ -176,7 +178,7 @@ struct WasmValidator : public PostWalker { template bool shouldBeFalse(bool result, T curr, const char* text) { if (result) { - fail(curr, "unexpected true: " + std::string(text)); + fail("unexpected true: " + std::string(text), curr); return false; } return result; @@ -187,7 +189,7 @@ struct WasmValidator : public PostWalker { if (left != right) { std::ostringstream ss; ss << left << " != " << right << ": " << text; - fail(curr, ss.str()); + fail(ss.str(), curr); return false; } return true; @@ -198,7 +200,7 @@ struct WasmValidator : public PostWalker { if (left != unreachable && left != right) { std::ostringstream ss; ss << left << " != " << right << ": " << text; - fail(curr, ss.str()); + fail(ss.str(), curr); return false; } return true; @@ -209,7 +211,7 @@ struct WasmValidator : public PostWalker { if (left == right) { std::ostringstream ss; ss << left << " == " << right << ": " << text; - fail(curr, ss.str()); + fail(ss.str(), curr); return false; } return true; From 4f48e96b900a2ca8a1f7df2e7ad2849378c04394 Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Thu, 13 Jul 2017 16:42:50 -0700 Subject: [PATCH 3/8] forgot the cpp --- src/wasm/wasm-validator.cpp | 6 +++++- test/atomics.wast | 10 +++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp index 77e5278a0f7..df3ab0c6233 100644 --- a/src/wasm/wasm-validator.cpp +++ b/src/wasm/wasm-validator.cpp @@ -220,11 +220,13 @@ void WasmValidator::visitSetLocal(SetLocal *curr) { } } void WasmValidator::visitLoad(Load *curr) { + if (curr->isAtomic && !getModule()->memory.shared) fail("Atomic operation with non-shared memory", 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) { + if (curr->isAtomic && !getModule()->memory.shared) fail("Atomic operation with non-shared memory", 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"); @@ -232,6 +234,7 @@ void WasmValidator::visitStore(Store *curr) { shouldBeEqualOrFirstIsUnreachable(curr->value->type, curr->valueType, curr, "store value type must match"); } void WasmValidator::visitAtomicRMW(AtomicRMW* curr) { + if (!getModule()->memory.shared) fail("Atomic operation with non-shared memory", curr); validateMemBytes(curr->bytes, curr->type, curr); shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "AtomicRMW pointer type must be i32"); shouldBeEqualOrFirstIsUnreachable(curr->value->type, curr->type, curr, "AtomicRMW result type must match operand"); @@ -244,6 +247,7 @@ void WasmValidator::visitAtomicRMW(AtomicRMW* curr) { } } void WasmValidator::visitAtomicCmpxchg(AtomicCmpxchg* curr) { + if (!getModule()->memory.shared) fail("Atomic operation with non-shared memory", curr); validateMemBytes(curr->bytes, curr->type, curr); shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "cmpxchg pointer type must be i32"); if (curr->expected->type != unreachable && curr->replacement->type != unreachable) { @@ -677,7 +681,7 @@ void WasmValidator::validateBinaryenIR(Module& wasm) { } template -std::ostream& WasmValidator::fail(T curr, S text) { +std::ostream& WasmValidator::fail(S text, T curr) { valid = false; auto& ret = printFailureHeader() << text << ", on \n"; return printModuleComponent(curr, ret); diff --git a/test/atomics.wast b/test/atomics.wast index 6b85c80ef9f..006412025ae 100644 --- a/test/atomics.wast +++ b/test/atomics.wast @@ -104,7 +104,7 @@ ) (func $atomic-cmpxchg (type $0) (local $0 i32) - (local $1 i32) + (local $1 i64) (drop (i32.atomic.rmw.cmpxchg offset=4 (get_local $0) @@ -122,15 +122,15 @@ (drop (i64.atomic.rmw.cmpxchg offset=4 (get_local $0) - (get_local $0) - (get_local $0) + (get_local $1) + (get_local $1) ) ) (drop (i64.atomic.rmw32_u.cmpxchg align=4 (get_local $0) - (get_local $0) - (get_local $0) + (get_local $1) + (get_local $1) ) ) ) From e232af5e7c70615695c9d51993a58628b08b96aa Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Thu, 13 Jul 2017 17:00:27 -0700 Subject: [PATCH 4/8] update test --- test/atomics.wast.fromBinary | 10 +++++----- test/atomics.wast.fromBinary.noDebugInfo | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/test/atomics.wast.fromBinary b/test/atomics.wast.fromBinary index 63809f1eda4..09f97812254 100644 --- a/test/atomics.wast.fromBinary +++ b/test/atomics.wast.fromBinary @@ -108,7 +108,7 @@ ) (func $atomic-cmpxchg (type $0) (local $var$0 i32) - (local $var$1 i32) + (local $var$1 i64) (block $label$0 (drop (i32.atomic.rmw.cmpxchg offset=4 @@ -127,15 +127,15 @@ (drop (i64.atomic.rmw.cmpxchg offset=4 (get_local $var$0) - (get_local $var$0) - (get_local $var$0) + (get_local $var$1) + (get_local $var$1) ) ) (drop (i64.atomic.rmw32_u.cmpxchg (get_local $var$0) - (get_local $var$0) - (get_local $var$0) + (get_local $var$1) + (get_local $var$1) ) ) ) diff --git a/test/atomics.wast.fromBinary.noDebugInfo b/test/atomics.wast.fromBinary.noDebugInfo index 8dcca9d1577..3b3426b1d73 100644 --- a/test/atomics.wast.fromBinary.noDebugInfo +++ b/test/atomics.wast.fromBinary.noDebugInfo @@ -108,7 +108,7 @@ ) (func $2 (type $0) (local $var$0 i32) - (local $var$1 i32) + (local $var$1 i64) (block $label$0 (drop (i32.atomic.rmw.cmpxchg offset=4 @@ -127,15 +127,15 @@ (drop (i64.atomic.rmw.cmpxchg offset=4 (get_local $var$0) - (get_local $var$0) - (get_local $var$0) + (get_local $var$1) + (get_local $var$1) ) ) (drop (i64.atomic.rmw32_u.cmpxchg (get_local $var$0) - (get_local $var$0) - (get_local $var$0) + (get_local $var$1) + (get_local $var$1) ) ) ) From 63f17a6deff7d58f2a685d092941a91446cd14ef Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Thu, 13 Jul 2017 17:35:07 -0700 Subject: [PATCH 5/8] forgot another test file --- test/atomics.wast.from-wast | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/atomics.wast.from-wast b/test/atomics.wast.from-wast index 01735c1fc8c..f44e232c444 100644 --- a/test/atomics.wast.from-wast +++ b/test/atomics.wast.from-wast @@ -104,7 +104,7 @@ ) (func $atomic-cmpxchg (type $0) (local $0 i32) - (local $1 i32) + (local $1 i64) (drop (i32.atomic.rmw.cmpxchg offset=4 (get_local $0) @@ -122,15 +122,15 @@ (drop (i64.atomic.rmw.cmpxchg offset=4 (get_local $0) - (get_local $0) - (get_local $0) + (get_local $1) + (get_local $1) ) ) (drop (i64.atomic.rmw32_u.cmpxchg (get_local $0) - (get_local $0) - (get_local $0) + (get_local $1) + (get_local $1) ) ) ) From 5dbcd060a93b9783a47b92bae12835dfd99a4d87 Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Thu, 13 Jul 2017 17:39:48 -0700 Subject: [PATCH 6/8] format switch --- 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 df3ab0c6233..fe76180c14b 100644 --- a/src/wasm/wasm-validator.cpp +++ b/src/wasm/wasm-validator.cpp @@ -241,8 +241,9 @@ void WasmValidator::visitAtomicRMW(AtomicRMW* curr) { switch (curr->type) { case i32: case i64: - case unreachable: + case unreachable: { break; + } default: fail("Atomic operations are only valid on int types", curr); } } From bb91e6c81eca5a0e4aa1a3ca6796b8123a5557b7 Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Thu, 13 Jul 2017 17:57:35 -0700 Subject: [PATCH 7/8] remove spurious indent --- src/wasm-validator.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wasm-validator.h b/src/wasm-validator.h index 492b2c6bcce..c2d88e3020f 100644 --- a/src/wasm-validator.h +++ b/src/wasm-validator.h @@ -46,10 +46,10 @@ namespace wasm { // Print anything that can be streamed to an ostream - template ::type>::value - >::type* = nullptr> +template ::type>::value + >::type* = nullptr> inline std::ostream& printModuleComponent(T curr, std::ostream& stream) { stream << curr << std::endl; return stream; From 2002aa10992fe1ad8cdb83904c4adf55070bf508 Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Fri, 14 Jul 2017 18:45:51 -0700 Subject: [PATCH 8/8] use shouldBeIntOrUnreachable --- src/wasm-validator.h | 1 + src/wasm/wasm-validator.cpp | 25 +++++++++++-------------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/wasm-validator.h b/src/wasm-validator.h index c2d88e3020f..f778e6f2451 100644 --- a/src/wasm-validator.h +++ b/src/wasm-validator.h @@ -217,6 +217,7 @@ struct WasmValidator : public PostWalker { return true; } + void shouldBeIntOrUnreachable(WasmType ty, Expression* curr, const char* text); void validateAlignment(size_t align, WasmType type, Index bytes, bool isAtomic, Expression* curr); void validateMemBytes(uint8_t bytes, WasmType ty, Expression* curr); diff --git a/src/wasm/wasm-validator.cpp b/src/wasm/wasm-validator.cpp index fe76180c14b..1b328ec19d5 100644 --- a/src/wasm/wasm-validator.cpp +++ b/src/wasm/wasm-validator.cpp @@ -233,20 +233,23 @@ void WasmValidator::visitStore(Store *curr) { 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) { - if (!getModule()->memory.shared) fail("Atomic operation with non-shared memory", curr); - validateMemBytes(curr->bytes, curr->type, curr); - shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "AtomicRMW pointer type must be i32"); - shouldBeEqualOrFirstIsUnreachable(curr->value->type, curr->type, curr, "AtomicRMW result type must match operand"); - switch (curr->type) { +void WasmValidator::shouldBeIntOrUnreachable(WasmType ty, Expression* curr, const char* text) { + switch (ty) { case i32: case i64: case unreachable: { break; } - default: fail("Atomic operations are only valid on int types", curr); + default: fail(text, curr); } } +void WasmValidator::visitAtomicRMW(AtomicRMW* curr) { + if (!getModule()->memory.shared) fail("Atomic operation with non-shared memory", curr); + validateMemBytes(curr->bytes, curr->type, curr); + shouldBeEqualOrFirstIsUnreachable(curr->ptr->type, i32, curr, "AtomicRMW pointer type must be i32"); + shouldBeEqualOrFirstIsUnreachable(curr->value->type, curr->type, curr, "AtomicRMW result type must match operand"); + shouldBeIntOrUnreachable(curr->type, curr, "Atomic operations are only valid on int types"); +} void WasmValidator::visitAtomicCmpxchg(AtomicCmpxchg* curr) { if (!getModule()->memory.shared) fail("Atomic operation with non-shared memory", curr); validateMemBytes(curr->bytes, curr->type, curr); @@ -256,13 +259,7 @@ void WasmValidator::visitAtomicCmpxchg(AtomicCmpxchg* curr) { } shouldBeEqualOrFirstIsUnreachable(curr->expected->type, curr->type, curr, "Cmpxchg result type must match expected"); shouldBeEqualOrFirstIsUnreachable(curr->replacement->type, curr->type, curr, "Cmpxchg result type must match replacement"); - switch (curr->expected->type) { - case i32: - case i64: - case unreachable: - break; - default: fail("Atomic operations are only valid on int types", curr); - } + shouldBeIntOrUnreachable(curr->expected->type, curr, "Atomic operations are only valid on int types"); } void WasmValidator::validateMemBytes(uint8_t bytes, WasmType ty, Expression* curr) { switch (bytes) {