Repository: bytecodealliance/wasmtime
Pull request #5858 (2023-02-23 00:08:57)

wasi-threads: fix use of `wait` in test
As @yamt points out [here], the `wait`/`notify` pairing used in this manual WAT test was not effective. The `wait` always immediately returned, meaning that the main thread essentially spins until a counter is atomically incremented. This is fine for test correctness, but was not the original intent, which was lost in a refactoring. This change uses the `$i` local to keep track of the counter value we expect to see for the `wait`, so that the `wait`/`notify` pair actually waits as expected.

[here]: https://github.com/bytecodealliance/wasmtime/pull/5484#discussion_r1101200012

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

- [ ] This has been discussed in issue #..., or if not, please tell us why
  here.
- [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
- [ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the [code of
conduct](https://github.com/bytecodealliance/wasmtime/blob/master/CODE_OF_CONDUCT.md).
-->



Patch:
diff --git a/cranelift/interpreter/src/step.rs b/cranelift/interpreter/src/step.rs
index d21372297..c77a5fa43 100644
--- a/cranelift/interpreter/src/step.rs
+++ b/cranelift/interpreter/src/step.rs
@@ -7,8 +7,8 @@ use crate::value::{Value, ValueConversionKind, ValueError, ValueResult};
 use cranelift_codegen::data_value::DataValue;
 use cranelift_codegen::ir::condcodes::{FloatCC, IntCC};
 use cranelift_codegen::ir::{
-    types, AbiParam, Block, BlockCall, ExternalName, FuncRef, Function, InstructionData, Opcode,
-    TrapCode, Type, Value as ValueRef,
+    types, AbiParam, AtomicRmwOp, Block, BlockCall, ExternalName, FuncRef, Function,
+    InstructionData, Opcode, TrapCode, Type, Value as ValueRef,
 };
 use log::trace;
 use smallvec::{smallvec, SmallVec};
@@ -1187,7 +1187,41 @@ where
             Value::convert(arg(0)?, ValueConversionKind::ExtractUpper(types::I64))?,
         ]),
         Opcode::Iconcat => assign(Value::concat(arg(0)?, arg(1)?)?),
-        Opcode::AtomicRmw => unimplemented!("AtomicRmw"),
+        Opcode::AtomicRmw => {
+            let op = inst.atomic_rmw_op().unwrap();
+            let val = arg(1)?;
+            let addr = arg(0)?.into_int()? as u64;
+            let loaded = Address::try_from(addr).and_then(|addr| state.checked_load(addr, ctrl_ty));
+            let prev_val = match loaded {
+                Ok(v) => v,
+                Err(e) => return Ok(ControlFlow::Trap(CraneliftTrap::User(memerror_to_trap(e)))),
+            };
+            let prev_val_to_assign = prev_val.clone();
+            let replace = match op {
+                AtomicRmwOp::Xchg => Ok(val),
+                AtomicRmwOp::Add => Value::add(prev_val, val),
+                AtomicRmwOp::Sub => Value::sub(prev_val, val),
+                AtomicRmwOp::And => Value::and(prev_val, val),
+                AtomicRmwOp::Or => Value::or(prev_val, val),
+                AtomicRmwOp::Xor => Value::xor(prev_val, val),
+                AtomicRmwOp::Nand => Value::and(prev_val, val).and_then(V::not),
+                AtomicRmwOp::Smax => Value::max(prev_val, val),
+                AtomicRmwOp::Smin => Value::min(prev_val, val),
+                AtomicRmwOp::Umax => Value::max(
+                    Value::convert(val, ValueConversionKind::ToUnsigned)?,
+                    Value::convert(prev_val, ValueConversionKind::ToUnsigned)?,
+                )
+                .and_then(|v| Value::convert(v, ValueConversionKind::ToSigned)),
+                AtomicRmwOp::Umin => Value::min(
+                    Value::convert(val, ValueConversionKind::ToUnsigned)?,
+                    Value::convert(prev_val, ValueConversionKind::ToUnsigned)?,
+                )
+                .and_then(|v| Value::convert(v, ValueConversionKind::ToSigned)),
+            }?;
+            let stored =
+                Address::try_from(addr).and_then(|addr| state.checked_store(addr, replace));
+            assign_or_memtrap(stored.map(|_| prev_val_to_assign))
+        }
         Opcode::AtomicCas => unimplemented!("AtomicCas"),
         Opcode::AtomicLoad => {
             let load_ty = inst_context.controlling_type().unwrap();


Test patch:
diff --git a/cranelift/filetests/filetests/runtests/atomic-rmw-little.clif b/cranelift/filetests/filetests/runtests/atomic-rmw-little.clif
index cfb56f791..4c09c3f1c 100644
--- a/cranelift/filetests/filetests/runtests/atomic-rmw-little.clif
+++ b/cranelift/filetests/filetests/runtests/atomic-rmw-little.clif
@@ -1,3 +1,4 @@
+test interpret
 test run
 target s390x
 target s390x has_mie2
@@ -116,11 +117,11 @@ block0(v0: i32, v1: i32):
     return v4
 }
 
-; run: %atomic_rmw_and_i64(0, 0) == 0
-; run: %atomic_rmw_and_i64(1, 0) == 0
-; run: %atomic_rmw_and_i64(0, 1) == 0
-; run: %atomic_rmw_and_i64(1, 1) == 1
-; run: %atomic_rmw_and_i64(0xF1FFFEFE, 0xCEFFEFEF) == 0xC0FFEEEE
+; run: %atomic_rmw_and_i32(0, 0) == 0
+; run: %atomic_rmw_and_i32(1, 0) == 0
+; run: %atomic_rmw_and_i32(0, 1) == 0
+; run: %atomic_rmw_and_i32(1, 1) == 1
+; run: %atomic_rmw_and_i32(0xF1FFFEFE, 0xCEFFEFEF) == 0xC0FFEEEE
 
 
 
diff --git a/tests/all/cli_tests/threads.wat b/tests/all/cli_tests/threads.wat
index 3e5b4397b..def5011f8 100644
--- a/tests/all/cli_tests/threads.wat
+++ b/tests/all/cli_tests/threads.wat
@@ -21,12 +21,15 @@
     (drop (call $__wasi_thread_spawn (i32.const 0)))
 
     ;; Wait for all the threads to notify us that they are done.
+    (local.set $i (i32.const 0))
     (loop $again
-      ;; Retrieve the i32 at address 128, compare it to -1 (it should always
-      ;; fail) and load it atomically to check if all three threads are
-      ;; complete. This wait is for 1ms or until notified, whichever is first.
-      (drop (memory.atomic.wait32 (i32.const 128) (i32.const -1) (i64.const 1000000)))
-      (br_if $again (i32.lt_s (i32.atomic.load (i32.const 128)) (i32.const 3)))
+      ;; Wait for the i32 at address 128 to be incremented by each thread. We
+      ;; maintain a local $i with the atomically loaded value as the expected
+      ;; wait value and to check if all three threads are complete. This wait is
+      ;; for 1ms or until notified, whichever is first.
+      (drop (memory.atomic.wait32 (i32.const 128) (local.get $i) (i64.const 1000000)))
+      (local.set $i (i32.atomic.load (i32.const 128)))
+      (br_if $again (i32.lt_s (local.get $i) (i32.const 3)))
     )
 
     ;; Print "Done".