Expressions should not appear twice in the ast (#1191)

Author: kripken

README.md

@@ -46,12 +46,14 @@ There are a few differences between Binaryen IR and the WebAssembly language:
      * Blocks without names may not be branch targets.
      * Names are required to be unique. (Reading wast files with duplicate names is supported; the names are modified when the IR is constructed).
    * As an optimization, a block that is the child of a loop (or if arm, or function toplevel) and which has no branches targeting it will not be emitted when generating wasm. Instead its list of operands will be directly used in the containing node. Such a block is sometimes called an "implicit block".
-   
- 
- 
 
 As a result, you might notice that round-trip conversions (wasm => Binaryen IR => wasm) change code a little in some corner cases.
 
+Notes when working with Binaryen IR:
+
+ * As mentioned above, Binaryen IR has a tree structure. As a result, each expression should have exactly one parent - you should not "reuse" a node by having it appear more than once in the tree. The motivation for this limitation is that when we optimize we modify nodes, so if they appear more than once in the tree, a change in one place can appear in another incorrectly.
+ * For similar reasons, nodes should not appear in more than one functions.
+
 ## Tools
 
 This repository contains code that builds the following tools in `bin/`:

src/wasm-emscripten.cpp

@@ -129,7 +129,8 @@ void generateStackAllocFunction(LinkerObject& linker) {
   Block* block = builder.makeBlock();
   block->list.push_back(setStackLocal);
   block->list.push_back(storeStack);
-  block->list.push_back(getStackLocal);
+  GetLocal* getStackLocal2 = builder.makeGetLocal(1, i32);
+  block->list.push_back(getStackLocal2);
   block->type = i32;
   function->body = block;
 

src/wasm-validator.h

@@ -39,6 +39,7 @@
 
 #include <set>
 #include <sstream>
+#include <unordered_set>
 
 #include "wasm.h"
 #include "wasm-printing.h"
@@ -83,6 +84,8 @@ struct WasmValidator : public PostWalker<WasmValidator> {
 
   std::set<Name> labelNames; // Binaryen IR requires that label names must be unique - IR generators must ensure that
 
+  std::unordered_set<Expression*> seenExpressions; // expressions must not appear twice
+
   void noteLabelName(Name name);
 
 public:

src/wasm/wasm-validator.cpp

@@ -559,6 +559,24 @@ void WasmValidator::visitFunction(Function *curr) {
   shouldBeTrue(breakTargets.empty(), curr->body, "all named break targets must exist");
   returnType = unreachable;
   labelNames.clear();
+  // expressions must not be seen more than once
+  struct Walker : public PostWalker<Walker, UnifiedExpressionVisitor<Walker>> {
+    std::unordered_set<Expression*>& seen;
+    std::vector<Expression*> dupes;
+
+    Walker(std::unordered_set<Expression*>& seen) : seen(seen) {}
+
+    void visitExpression(Expression* curr) {
+      bool inserted;
+      std::tie(std::ignore, inserted) = seen.insert(curr);
+      if (!inserted) dupes.push_back(curr);
+    }
+  };
+  Walker walker(seenExpressions);
+  walker.walk(curr->body);
+  for (auto* bad : walker.dupes) {
+    fail("expression seen more than once in the tree", bad);
+  }
 }
 
 static bool checkOffset(Expression* curr, Address add, Address max) {

test/example/c-api-kitchen-sink.c

@@ -98,6 +98,7 @@ void test_core() {
 
   BinaryenExpressionRef callOperands2[] = { makeInt32(module, 13), makeFloat64(module, 3.7) };
   BinaryenExpressionRef callOperands4[] = { makeInt32(module, 13), makeInt64(module, 37), makeFloat32(module, 1.3f), makeFloat64(module, 3.7) };
+  BinaryenExpressionRef callOperands4b[] = { makeInt32(module, 13), makeInt64(module, 37), makeFloat32(module, 1.3f), makeFloat64(module, 3.7) };
 
   BinaryenType params[4] = { BinaryenInt32(), BinaryenInt64(), BinaryenFloat32(), BinaryenFloat64() };
   BinaryenFunctionTypeRef iiIfF = BinaryenAddFunctionType(module, "iiIfF", BinaryenInt32(), params, 4);
@@ -203,7 +204,7 @@ void test_core() {
       )
     ),
     BinaryenUnary(module, BinaryenEqZInt32(), // check the output type of the call node
-      BinaryenCallIndirect(module, makeInt32(module, 2449), callOperands4, 4, "iiIfF")
+      BinaryenCallIndirect(module, makeInt32(module, 2449), callOperands4b, 4, "iiIfF")
     ),
     BinaryenDrop(module, BinaryenGetLocal(module, 0, BinaryenInt32())),
     BinaryenSetLocal(module, 0, makeInt32(module, 101)),
@@ -545,18 +546,36 @@ void test_interpret() {
 
 void test_nonvalid() {
   // create a module that fails to validate
-  BinaryenModuleRef module = BinaryenModuleCreate();
+  {
+    BinaryenModuleRef module = BinaryenModuleCreate();
 
-  BinaryenFunctionTypeRef v = BinaryenAddFunctionType(module, "v", BinaryenNone(), NULL, 0);
-  BinaryenType localTypes[] = { BinaryenInt32() };
-  BinaryenFunctionRef func = BinaryenAddFunction(module, "func", v, localTypes, 1,
-    BinaryenSetLocal(module, 0, makeInt64(module, 1234)) // wrong type!
-  );
+    BinaryenFunctionTypeRef v = BinaryenAddFunctionType(module, "v", BinaryenNone(), NULL, 0);
+    BinaryenType localTypes[] = { BinaryenInt32() };
+    BinaryenFunctionRef func = BinaryenAddFunction(module, "func", v, localTypes, 1,
+      BinaryenSetLocal(module, 0, makeInt64(module, 1234)) // wrong type!
+    );
 
-  BinaryenModulePrint(module);
-  printf("validation: %d\n", BinaryenModuleValidate(module));
+    BinaryenModulePrint(module);
+    printf("validation: %d\n", BinaryenModuleValidate(module));
 
-  BinaryenModuleDispose(module);
+    BinaryenModuleDispose(module);
+  }
+  // validation failure due to duplicate nodes
+  {
+    BinaryenModuleRef module = BinaryenModuleCreate();
+
+    BinaryenFunctionTypeRef v = BinaryenAddFunctionType(module, "i", BinaryenInt32(), NULL, 0);
+    BinaryenType localTypes[] = { };
+    BinaryenExpressionRef num = makeInt32(module, 1234);
+    BinaryenFunctionRef func = BinaryenAddFunction(module, "func", v, NULL, 0,
+      BinaryenBinary(module, BinaryenInt32(), num, num) // incorrectly use num twice
+    );
+
+    BinaryenModulePrint(module);
+    printf("validation: %d\n", BinaryenModuleValidate(module));
+
+    BinaryenModuleDispose(module);
+  }
 }
 
 void test_tracing() {