Validation for AtomicRMW and cmpxchg (#1092)

Also fix cases where fail() had the arguments backwards. Wasn't an error because lol templates.
Also fix printModuleComponent template to SFINAE on Expression* so we properly get the specialized version.

Author: dschuff

src/wasm-validator.h

@@ -46,13 +46,15 @@
 namespace wasm {
 
 // Print anything that can be streamed to an ostream
-template <typename T>
+template <typename T,
+  typename std::enable_if<
+    !std::is_base_of<Expression, typename std::remove_pointer<T>::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,21 +164,21 @@ struct WasmValidator : public PostWalker<WasmValidator> {
   // helpers
  private:
   template <typename T, typename S>
-  std::ostream& fail(T curr, S text);
+  std::ostream& fail(S text, T curr);
   std::ostream& printFailureHeader();
 
   template<typename T>
   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;
   }
   template<typename T>
   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<WasmValidator> {
     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<WasmValidator> {
     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,12 +211,13 @@ struct WasmValidator : public PostWalker<WasmValidator> {
     if (left == right) {
       std::ostringstream ss;
       ss << left << " == " << right << ": " << text;
-      fail(curr, ss.str());
+      fail(ss.str(), curr);
       return false;
     }
     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);

src/wasm/wasm-validator.cpp

@@ -220,22 +220,46 @@ 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");
   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::shouldBeIntOrUnreachable(WasmType ty, Expression* curr, const char* text) {
+  switch (ty) {
+    case i32:
+    case i64:
+    case unreachable: {
+      break;
+    }
+    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);
+  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");
+  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) {
@@ -655,7 +679,7 @@ void WasmValidator::validateBinaryenIR(Module& wasm) {
 }
 
 template <typename T, typename S>
-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);

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)
    )
   )
  )

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)
    )
   )
  )

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)
     )
    )
   )

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)
     )
    )
   )