[NFC] Make MemoryOrder parameters non-optional (#7171)

Update Builder and IRBuilder makeStructGet and makeStructSet functions to require the memory order to be explicitly supplied. This is slightly more verbose, but will reduce the chances that we forget to properly consider synchronization when implementing new features in the future.
WebAssembly · Dec 21, 2024 · 6ddacde · 6ddacde
1 parent 4d8a933
commit 6ddacde
Show file tree

Hide file tree

Showing 11 changed files with 36 additions and 31 deletions.
diff --git a/src/binaryen-c.cpp b/src/binaryen-c.cpp
@@ -1752,15 +1752,17 @@ BinaryenExpressionRef BinaryenStructGet(BinaryenModuleRef module,
                                         bool signed_) {
   return static_cast<Expression*>(
     Builder(*(Module*)module)
-      .makeStructGet(index, (Expression*)ref, Type(type), signed_));
+      .makeStructGet(
+        index, (Expression*)ref, MemoryOrder::Unordered, Type(type), signed_));
 }
 BinaryenExpressionRef BinaryenStructSet(BinaryenModuleRef module,
                                         BinaryenIndex index,
                                         BinaryenExpressionRef ref,
                                         BinaryenExpressionRef value) {
   return static_cast<Expression*>(
     Builder(*(Module*)module)
-      .makeStructSet(index, (Expression*)ref, (Expression*)value));
+      .makeStructSet(
+        index, (Expression*)ref, (Expression*)value, MemoryOrder::Unordered));
 }
 BinaryenExpressionRef BinaryenArrayNew(BinaryenModuleRef module,
                                        BinaryenHeapType type,

diff --git a/src/parser/contexts.h b/src/parser/contexts.h
@@ -740,15 +740,12 @@ struct NullInstrParserCtx {
                          HeapTypeT,
                          FieldIdxT,
                          bool,
-                         MemoryOrder = MemoryOrder::Unordered) {
+                         MemoryOrder) {
     return Ok{};
   }
   template<typename HeapTypeT>
-  Result<> makeStructSet(Index,
-                         const std::vector<Annotation>&,
-                         HeapTypeT,
-                         FieldIdxT,
-                         MemoryOrder = MemoryOrder::Unordered) {
+  Result<> makeStructSet(
+    Index, const std::vector<Annotation>&, HeapTypeT, FieldIdxT, MemoryOrder) {
     return Ok{};
   }
   template<typename HeapTypeT>

diff --git a/src/parser/parsers.h b/src/parser/parsers.h
@@ -2240,7 +2240,8 @@ Result<> makeStructGet(Ctx& ctx,
   CHECK_ERR(type);
   auto field = fieldidx(ctx, *type);
   CHECK_ERR(field);
-  return ctx.makeStructGet(pos, annotations, *type, *field, signed_);
+  return ctx.makeStructGet(
+    pos, annotations, *type, *field, signed_, MemoryOrder::Unordered);
 }
 
 template<typename Ctx>
@@ -2264,7 +2265,8 @@ makeStructSet(Ctx& ctx, Index pos, const std::vector<Annotation>& annotations) {
   CHECK_ERR(type);
   auto field = fieldidx(ctx, *type);
   CHECK_ERR(field);
-  return ctx.makeStructSet(pos, annotations, *type, *field);
+  return ctx.makeStructSet(
+    pos, annotations, *type, *field, MemoryOrder::Unordered);
 }
 
 template<typename Ctx>

diff --git a/src/passes/Heap2Local.cpp b/src/passes/Heap2Local.cpp
@@ -1066,7 +1066,9 @@ struct Array2Struct : PostWalker<Array2Struct> {
     }
 
     // Convert the ArraySet into a StructSet.
-    replaceCurrent(builder.makeStructSet(index, curr->ref, curr->value));
+    // TODO: Handle atomic array accesses.
+    replaceCurrent(builder.makeStructSet(
+      index, curr->ref, curr->value, MemoryOrder::Unordered));
   }
 
   void visitArrayGet(ArrayGet* curr) {
@@ -1085,8 +1087,9 @@ struct Array2Struct : PostWalker<Array2Struct> {
     }
 
     // Convert the ArrayGet into a StructGet.
-    replaceCurrent(
-      builder.makeStructGet(index, curr->ref, curr->type, curr->signed_));
+    // TODO: Handle atomic array accesses.
+    replaceCurrent(builder.makeStructGet(
+      index, curr->ref, MemoryOrder::Unordered, curr->type, curr->signed_));
   }
 
   // Some additional operations need special handling

diff --git a/src/passes/J2CLItableMerging.cpp b/src/passes/J2CLItableMerging.cpp
@@ -280,6 +280,7 @@ struct J2CLItableMerging : public Pass {
         replaceCurrent(builder.makeStructGet(
           0,
           curr->ref,
+          MemoryOrder::Unordered,
           parent.structInfoByITableType[curr->type.getHeapType()]
             ->javaClass.getStruct()
             .fields[0]
@@ -341,4 +342,4 @@ struct J2CLItableMerging : public Pass {
 } // anonymous namespace
 
 Pass* createJ2CLItableMergingPass() { return new J2CLItableMerging(); }
-} // namespace wasm
+} // namespace wasm
diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp
@@ -4493,7 +4493,8 @@ Expression* TranslateToFuzzReader::makeStructGet(Type type) {
   auto [structType, fieldIndex] = pick(structFields);
   auto* ref = makeTrappingRefUse(structType);
   auto signed_ = maybeSignedGet(structType.getStruct().fields[fieldIndex]);
-  return builder.makeStructGet(fieldIndex, ref, type, signed_);
+  return builder.makeStructGet(
+    fieldIndex, ref, MemoryOrder::Unordered, type, signed_);
 }
 
 Expression* TranslateToFuzzReader::makeStructSet(Type type) {
@@ -4505,7 +4506,7 @@ Expression* TranslateToFuzzReader::makeStructSet(Type type) {
   auto fieldType = structType.getStruct().fields[fieldIndex].type;
   auto* ref = makeTrappingRefUse(structType);
   auto* value = make(fieldType);
-  return builder.makeStructSet(fieldIndex, ref, value);
+  return builder.makeStructSet(fieldIndex, ref, value, MemoryOrder::Unordered);
 }
 
 // Make a bounds check for an array operation, given a ref + index. An optional

diff --git a/src/tools/wasm-ctor-eval.cpp b/src/tools/wasm-ctor-eval.cpp
@@ -957,7 +957,8 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
 
     Expression* set;
     if (global.type.isStruct()) {
-      set = builder.makeStructSet(index, getGlobal, value);
+      set =
+        builder.makeStructSet(index, getGlobal, value, MemoryOrder::Unordered);
     } else {
       set = builder.makeArraySet(
         getGlobal, builder.makeConst(int32_t(index)), value);

diff --git a/src/wasm-builder.h b/src/wasm-builder.h
@@ -938,9 +938,9 @@ class Builder {
   }
   StructGet* makeStructGet(Index index,
                            Expression* ref,
+                           MemoryOrder order,
                            Type type,
-                           bool signed_ = false,
-                           MemoryOrder order = MemoryOrder::Unordered) {
+                           bool signed_ = false) {
     auto* ret = wasm.allocator.alloc<StructGet>();
     ret->index = index;
     ret->ref = ref;
@@ -953,7 +953,7 @@ class Builder {
   StructSet* makeStructSet(Index index,
                            Expression* ref,
                            Expression* value,
-                           MemoryOrder order = MemoryOrder::Unordered) {
+                           MemoryOrder order) {
     auto* ret = wasm.allocator.alloc<StructSet>();
     ret->index = index;
     ret->ref = ref;

diff --git a/src/wasm-ir-builder.h b/src/wasm-ir-builder.h
@@ -204,13 +204,9 @@ class IRBuilder : public UnifiedExpressionVisitor<IRBuilder, Result<>> {
   makeBrOn(Index label, BrOnOp op, Type in = Type::none, Type out = Type::none);
   Result<> makeStructNew(HeapType type);
   Result<> makeStructNewDefault(HeapType type);
-  Result<> makeStructGet(HeapType type,
-                         Index field,
-                         bool signed_,
-                         MemoryOrder order = MemoryOrder::Unordered);
-  Result<> makeStructSet(HeapType type,
-                         Index field,
-                         MemoryOrder order = MemoryOrder::Unordered);
+  Result<>
+  makeStructGet(HeapType type, Index field, bool signed_, MemoryOrder order);
+  Result<> makeStructSet(HeapType type, Index field, MemoryOrder order);
   Result<> makeArrayNew(HeapType type);
   Result<> makeArrayNewDefault(HeapType type);
   Result<> makeArrayNewData(HeapType type, Name data);

diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp
@@ -4172,13 +4172,15 @@ Result<> WasmBinaryReader::readInst() {
         case BinaryConsts::StructGetU: {
           auto type = getIndexedHeapType();
           auto field = getU32LEB();
-          return builder.makeStructGet(
-            type, field, op == BinaryConsts::StructGetS);
+          return builder.makeStructGet(type,
+                                       field,
+                                       op == BinaryConsts::StructGetS,
+                                       MemoryOrder::Unordered);
         }
         case BinaryConsts::StructSet: {
           auto type = getIndexedHeapType();
           auto field = getU32LEB();
-          return builder.makeStructSet(type, field);
+          return builder.makeStructSet(type, field, MemoryOrder::Unordered);
         }
         case BinaryConsts::ArrayNew:
           return builder.makeArrayNew(getIndexedHeapType());

diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp
@@ -1801,7 +1801,7 @@ Result<> IRBuilder::makeStructGet(HeapType type,
   CHECK_ERR(ChildPopper{*this}.visitStructGet(&curr, type));
   CHECK_ERR(validateTypeAnnotation(type, curr.ref));
   push(
-    builder.makeStructGet(field, curr.ref, fields[field].type, signed_, order));
+    builder.makeStructGet(field, curr.ref, order, fields[field].type, signed_));
   return Ok{};
 }