Fix validation of non-void if blocks with no else
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Dec 2016 02:38:27 +0000 (02:38 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Dec 2016 02:38:27 +0000 (02:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165938

Reviewed by Saam Barati.

JSTests:

Add a new failing test and a fix an existing one.

* wasm/function-tests/dead-call.js:
* wasm/function-tests/if-no-else-non-void.js: Added.

Source/JavaScriptCore:

We should not have been allowing non-void if-blocks that don't
have an else. Since this causes a value to be placed on the
stack that only appears under some control flow and not another.

* wasm/WasmValidate.cpp:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@209899 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/wasm/function-tests/dead-call.js
JSTests/wasm/function-tests/if-no-else-non-void.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/wasm/WasmValidate.cpp

index 7c8a1d6..0364a82 100644 (file)
@@ -1,5 +1,17 @@
 2016-12-15  Keith Miller  <keith_miller@apple.com>
 
+        Fix validation of non-void if blocks with no else
+        https://bugs.webkit.org/show_bug.cgi?id=165938
+
+        Reviewed by Saam Barati.
+
+        Add a new failing test and a fix an existing one.
+
+        * wasm/function-tests/dead-call.js:
+        * wasm/function-tests/if-no-else-non-void.js: Added.
+
+2016-12-15  Keith Miller  <keith_miller@apple.com>
+
         Wasm should not create empty unlinked callsites
         https://bugs.webkit.org/show_bug.cgi?id=165933
 
index 3fc2b44..d56e15b 100644 (file)
@@ -14,8 +14,12 @@ const builder = (new Builder())
 
           .Function("dead-call", { params: [], ret: "i32" })
               .I32Const(0)
-              .If("i32", b => b.Call(0).Return())
-              .I32Const(1)
+              .If("i32", b =>
+                  b.Call(0)
+                  .Return()
+                  .Else()
+                  .I32Const(1)
+                 )
           .End()
 
       .End();
diff --git a/JSTests/wasm/function-tests/if-no-else-non-void.js b/JSTests/wasm/function-tests/if-no-else-non-void.js
new file mode 100644 (file)
index 0000000..2338ff8
--- /dev/null
@@ -0,0 +1,16 @@
+import * as assert from '../assert.js';
+import Builder from '../Builder.js';
+
+const builder = (new Builder())
+      .Type().End()
+      .Function().End()
+      .Code()
+          .Function("bad-if", { params: [], ret: "i32" })
+              .I32Const(0)
+              .If("i32", b => b.I32Const(0))
+          .End()
+
+      .End();
+
+const bin = builder.WebAssembly().get();
+assert.throws(() => new WebAssembly.Module(bin), WebAssembly.CompileError, "WebAssembly.Module doesn't validate: If-block had a non-void result type: I32 but had no else-block (evaluating 'new WebAssembly.Module(bin)')");
index 537205e..744c9b5 100644 (file)
@@ -1,3 +1,16 @@
+2016-12-15  Keith Miller  <keith_miller@apple.com>
+
+        Fix validation of non-void if blocks with no else
+        https://bugs.webkit.org/show_bug.cgi?id=165938
+
+        Reviewed by Saam Barati.
+
+        We should not have been allowing non-void if-blocks that don't
+        have an else. Since this causes a value to be placed on the
+        stack that only appears under some control flow and not another.
+
+        * wasm/WasmValidate.cpp:
+
 2016-12-15  Filip Pizlo  <fpizlo@apple.com>
 
         Get rid of HeapRootVisitor and make SlotVisitor less painful to use
index d1b556e..0a129a9 100644 (file)
@@ -290,6 +290,7 @@ auto Validate::endBlock(ControlEntry& entry, ExpressionList& stack) -> Result
     if (block.signature() == Void)
         return { };
 
+    WASM_VALIDATOR_FAIL_IF(block.type() == BlockType::If, "If-block had a non-void result type: ", block.signature(), " but had no else-block");
     WASM_VALIDATOR_FAIL_IF(stack.isEmpty(), "typed block falls through on empty stack");
     WASM_VALIDATOR_FAIL_IF(block.signature() != stack.last(), "block fallthrough doesn't match its declared type");
 
@@ -299,8 +300,11 @@ auto Validate::endBlock(ControlEntry& entry, ExpressionList& stack) -> Result
 
 auto Validate::addEndToUnreachable(ControlEntry& entry) -> Result
 {
-    if (entry.controlData.signature() != Void)
-        entry.enclosedExpressionStack.append(entry.controlData.signature());
+    auto block = entry.controlData;
+    if (block.signature() != Void) {
+        WASM_VALIDATOR_FAIL_IF(block.type() == BlockType::If, "If-block had a non-void result type: ", block.signature(), " but had no else-block");
+        entry.enclosedExpressionStack.append(block.signature());
+    }
     return { };
 }