WebAssembly: implement init_expr for Element
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 Mar 2017 21:44:20 +0000 (21:44 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 Mar 2017 21:44:20 +0000 (21:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165888
<rdar://problem/29760199>

Reviewed by Keith Miller.

JSTests:

* wasm/Builder_WebAssemblyBinary.js:
(const.emitters.Element):
* wasm/assert.js:
* wasm/js-api/element.js:
(assert.throws):
(badInstantiation.makeModule):
(badInstantiation.test):
(badInstantiation):
* wasm/js-api/global-error.js:

Source/JavaScriptCore:

This patch fixes a few bugs. The main change is allowing init_expr
for the Element's offset. To do this, I had to fix a couple of
other bugs:

- I removed our invalid early module-parse-time invalidation
of out of bound Element sections. This is not in the spec because
it can't be validated in the general case when the offset is a
get_global.

- Our get_global validation inside our init_expr parsing code was simply wrong.
It thought that the index operand to get_global went into the pool of imports,
but it does not. It indexes into the pool of globals. I changed the code to
refer to the global pool instead.

* wasm/WasmFormat.h:
(JSC::Wasm::Element::Element):
* wasm/WasmModuleParser.cpp:
* wasm/js/WebAssemblyModuleRecord.cpp:
(JSC::WebAssemblyModuleRecord::evaluate):

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

JSTests/ChangeLog
JSTests/wasm/Builder_WebAssemblyBinary.js
JSTests/wasm/assert.js
JSTests/wasm/js-api/element.js
JSTests/wasm/js-api/global-error.js
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/wasm/WasmFormat.h
Source/JavaScriptCore/wasm/WasmModuleParser.cpp
Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp

index 636eb69..f556469 100644 (file)
@@ -1,3 +1,21 @@
+2017-03-06  Saam Barati  <sbarati@apple.com>
+
+        WebAssembly: implement init_expr for Element
+        https://bugs.webkit.org/show_bug.cgi?id=165888
+        <rdar://problem/29760199>
+
+        Reviewed by Keith Miller.
+
+        * wasm/Builder_WebAssemblyBinary.js:
+        (const.emitters.Element):
+        * wasm/assert.js:
+        * wasm/js-api/element.js:
+        (assert.throws):
+        (badInstantiation.makeModule):
+        (badInstantiation.test):
+        (badInstantiation):
+        * wasm/js-api/global-error.js:
+
 2017-03-06  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [JSC] [Linux] Test stress/spread-forward-call-varargs-stack-overflow.js fails
index 7e05a13..081de99 100644 (file)
@@ -195,11 +195,12 @@ const emitters = {
         for (const {tableIndex, offset, functionIndices} of data) {
             put(bin, "varuint32", tableIndex);
 
-            // FIXME allow complex init_expr here. https://bugs.webkit.org/show_bug.cgi?id=165700
-            // For now we only handle i32.const as offset.
-            put(bin, "uint8", WASM.description.opcode["i32.const"].value);
-            put(bin, WASM.description.opcode["i32.const"].immediate[0].type, offset);
-            put(bin, "uint8", WASM.description.opcode["end"].value);
+            let initExpr;
+            if (typeof offset === "number")
+                initExpr = {op: "i32.const", initValue: offset};
+            else
+                initExpr = offset;
+            putInitExpr(bin, initExpr);
 
             put(bin, "varuint32", functionIndices.length);
             for (const functionIndex of functionIndices)
index cd92986..d1aa087 100644 (file)
@@ -112,7 +112,7 @@ export const le = (lhs, rhs, msg) => {
 };
 
 // Ignore source information at the end of the error message if the expected message didn't specify that information. Sometimes it changes, or it's tricky to get just right.
-const _sourceRe = new RegExp(/ \(evaluating '.*'\)/);
+const _sourceRe = new RegExp(/( \(evaluating '.*'\))|( \(In .*\))/);
 
 const _throws = (func, type, message, ...args) => {
     try {
index b269292..a9ab26d 100644 (file)
@@ -53,7 +53,7 @@ import * as assert from '../assert.js';
             .Table({element: "anyfunc", initial: 20, maximum: 20})
         .End()
         .Element()
-            .Element({offset: 19, functionIndices: [0, 1]})
+            .Element({offset: 19, functionIndices: [0, 0]})
         .End()
         .Code()
             .Function("foo", {params: ["i32"], ret: "i32"})
@@ -64,7 +64,8 @@ import * as assert from '../assert.js';
             .End()
         .End();
 
-    assert.throws(() => new WebAssembly.Module(builder.WebAssembly().get()), WebAssembly.CompileError, "WebAssembly.Module doesn't parse at byte 35 / 49: Element section's 0th element writes to index 20 which exceeds the maximum 20 (evaluating 'new WebAssembly.Module(builder.WebAssembly().get())')");
+    const module = new WebAssembly.Module(builder.WebAssembly().get());
+    assert.throws(() => new WebAssembly.Instance(module), WebAssembly.LinkError, "Element is trying to set an out of bounds table index");
 }
 
 {
@@ -87,7 +88,8 @@ import * as assert from '../assert.js';
             .End()
         .End();
 
-    assert.throws(() => new WebAssembly.Module(builder.WebAssembly().get()), WebAssembly.CompileError, "WebAssembly.Module doesn't parse at byte 35 / 48: Element section's 0th element writes to index 20 which exceeds the maximum 20 (evaluating 'new WebAssembly.Module(builder.WebAssembly().get())')");
+    const module = new WebAssembly.Module(builder.WebAssembly().get());
+    assert.throws(() => new WebAssembly.Instance(module), WebAssembly.LinkError, "Element is trying to set an out of bounds table index");
 }
 
 {
@@ -144,3 +146,74 @@ import * as assert from '../assert.js';
         badInstantiation(table, WebAssembly.LinkError, "Element is trying to set an out of bounds table index (evaluating 'new WebAssembly.Instance(module, {imp: {table: actualTable}})')");
     }
 }
+
+{
+    function makeModule() {
+        const builder = new Builder()
+            .Type().End()
+            .Import()
+                .Table("imp", "table", {element: "anyfunc", initial: 19}) // unspecified maximum.
+                .Global().I32("imp", "global", "immutable").End()
+            .End()
+            .Function().End()
+            .Element()
+                .Element({offset: {op: "get_global", initValue: 0}, functionIndices: [0]})
+            .End()
+            .Code()
+                .Function("foo", {params: ["i32"], ret: "i32"})
+                    .GetLocal(0)
+                    .I32Const(42)
+                    .I32Add()
+                    .Return()
+                .End()
+            .End();
+
+        const bin = builder.WebAssembly().get();
+        return new WebAssembly.Module(bin);
+    }
+
+    function test(i) {
+        const table = new WebAssembly.Table({element: "anyfunc", initial: 19});
+        const global = i;
+        const module = makeModule();
+        const instance = new WebAssembly.Instance(module, {imp: {table, global}});
+        for (let j = 0; j < 19; j++) {
+            if (j === i)
+                assert.eq(table.get(j)(i*2), i*2 + 42);
+            else
+                assert.throws(() => table.get(j)(i*2), TypeError, "table.get(j) is not a function.");
+        }
+    }
+    for (let i = 0; i < 19; i++)
+        test(i);
+
+    assert.throws(() => test(19), Error, "Element is trying to set an out of bounds table index");
+}
+
+{
+    function badModule() {
+        const builder = new Builder()
+            .Type().End()
+            .Import()
+                .Table("imp", "table", {element: "anyfunc", initial: 19}) // unspecified maximum.
+                .Global().F32("imp", "global", "immutable").End()
+            .End()
+            .Function().End()
+            .Element()
+                .Element({offset: {op: "get_global", initValue: 0}, functionIndices: [0]})
+            .End()
+            .Code()
+                .Function("foo", {params: ["i32"], ret: "i32"})
+                    .GetLocal(0)
+                    .I32Const(42)
+                    .I32Add()
+                    .Return()
+                .End()
+            .End();
+
+        const bin = builder.WebAssembly().get();
+        return new WebAssembly.Module(bin);
+    }
+
+    assert.throws(() => badModule(), WebAssembly.CompileError, "WebAssembly.Module doesn't parse at byte 58 / 72: 0th Element init_expr must produce an i32");
+}
index 3fba3eb..aee7535 100644 (file)
@@ -23,7 +23,7 @@ import Builder from '../Builder.js';
     const bin = builder.WebAssembly();
     bin.trim();
 
-    assert.throws(() => new WebAssembly.Module(bin.get()), WebAssembly.CompileError, "WebAssembly.Module doesn't parse at byte 26 / 59: get_global's index 0 exceeds the number of imports 0 (evaluating 'new WebAssembly.Module(bin.get())')");
+    assert.throws(() => new WebAssembly.Module(bin.get()), WebAssembly.CompileError, "WebAssembly.Module doesn't parse at byte 26 / 59: get_global's index 0 exceeds the number of globals 0 (evaluating 'new WebAssembly.Module(bin.get())')");
 }
 
 
index ab6b1f3..ebbb108 100644 (file)
@@ -1,3 +1,31 @@
+2017-03-06  Saam Barati  <sbarati@apple.com>
+
+        WebAssembly: implement init_expr for Element
+        https://bugs.webkit.org/show_bug.cgi?id=165888
+        <rdar://problem/29760199>
+
+        Reviewed by Keith Miller.
+
+        This patch fixes a few bugs. The main change is allowing init_expr
+        for the Element's offset. To do this, I had to fix a couple of
+        other bugs:
+        
+        - I removed our invalid early module-parse-time invalidation
+        of out of bound Element sections. This is not in the spec because
+        it can't be validated in the general case when the offset is a
+        get_global.
+        
+        - Our get_global validation inside our init_expr parsing code was simply wrong.
+        It thought that the index operand to get_global went into the pool of imports,
+        but it does not. It indexes into the pool of globals. I changed the code to
+        refer to the global pool instead.
+
+        * wasm/WasmFormat.h:
+        (JSC::Wasm::Element::Element):
+        * wasm/WasmModuleParser.cpp:
+        * wasm/js/WebAssemblyModuleRecord.cpp:
+        (JSC::WebAssemblyModuleRecord::evaluate):
+
 2017-03-06  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Allow indexed module namespace object fields
index 1fc574e..9a7a672 100644 (file)
@@ -189,7 +189,11 @@ struct Segment {
 };
 
 struct Element {
-    uint32_t offset;
+    Element(I32InitExpr offset)
+        : offset(offset)
+    { }
+
+    I32InitExpr offset;
     Vector<uint32_t> functionIndices;
 };
 
index 8d45f95..21d1d0c 100644 (file)
@@ -431,36 +431,23 @@ auto ModuleParser::parseElement() -> PartialResult
     WASM_PARSER_FAIL_IF(!m_result.module->elements.tryReserveCapacity(elementCount), "can't allocate memory for ", elementCount, " Elements");
     for (unsigned elementNum = 0; elementNum < elementCount; ++elementNum) {
         uint32_t tableIndex;
-        uint64_t offset;
+        uint64_t initExprBits;
         uint8_t initOpcode;
         uint32_t indexCount;
 
         WASM_PARSER_FAIL_IF(!parseVarUInt32(tableIndex), "can't get ", elementNum, "th Element table index");
         WASM_PARSER_FAIL_IF(tableIndex, "Element section can only have one Table for now");
         Type initExprType;
-        WASM_FAIL_IF_HELPER_FAILS(parseInitExpr(initOpcode, offset, initExprType));
-        WASM_PARSER_FAIL_IF(initOpcode != OpType::I32Const, "Element section doesn't support non-i32 init_expr opcode for now, got ", initOpcode);
+        WASM_FAIL_IF_HELPER_FAILS(parseInitExpr(initOpcode, initExprBits, initExprType));
+        WASM_PARSER_FAIL_IF(initExprType != I32, "Element init_expr must produce an i32");
         WASM_PARSER_FAIL_IF(!parseVarUInt32(indexCount), "can't get ", elementNum, "th index count for Element section");
         WASM_PARSER_FAIL_IF(indexCount == std::numeric_limits<uint32_t>::max(), "Element section's ", elementNum, "th index count is too big ", indexCount);
 
         ASSERT(!!m_result.module->tableInformation);
-        if (std::optional<uint32_t> maximum = m_result.module->tableInformation.maximum()) {
-            // FIXME: should indexCount being zero be a validation error?
-            // https://bugs.webkit.org/show_bug.cgi?id=165826
-            if (indexCount) {
-                // FIXME: right now, provably out of bounds writes are validation errors.
-                // Should they be though?
-                // https://bugs.webkit.org/show_bug.cgi?id=165827
-                uint64_t lastWrittenIndex = static_cast<uint64_t>(indexCount) + static_cast<uint64_t>(offset) - 1;
-                WASM_PARSER_FAIL_IF(lastWrittenIndex >= static_cast<uint64_t>(*maximum), "Element section's ", elementNum, "th element writes to index ", lastWrittenIndex, " which exceeds the maximum ", *maximum);
-            }
-        }
 
-        Element element;
+        Element element(makeI32InitExpr(initOpcode, initExprBits));
         WASM_PARSER_FAIL_IF(!element.functionIndices.tryReserveCapacity(indexCount), "can't allocate memory for ", indexCount, " Element indices");
 
-        element.offset = offset;
-
         for (unsigned index = 0; index < indexCount; ++index) {
             uint32_t functionIndex;
             WASM_PARSER_FAIL_IF(!parseVarUInt32(functionIndex), "can't get Element section's ", elementNum, "th element's ", index, "th index");
@@ -536,12 +523,11 @@ auto ModuleParser::parseInitExpr(uint8_t& opcode, uint64_t& bitsOrImportNumber,
     case GetGlobal: {
         uint32_t index;
         WASM_PARSER_FAIL_IF(!parseVarUInt32(index), "can't get get_global's index");
-        WASM_PARSER_FAIL_IF(index >= m_result.module->imports.size(), "get_global's index ", index, " exceeds the number of imports ", m_result.module->imports.size());
-        const Import& import = m_result.module->imports[index];
-        WASM_PARSER_FAIL_IF(m_result.module->imports[index].kind != ExternalKind::Global, "get_global's import kind is ", m_result.module->imports[index].kind, " should be global");
-        WASM_PARSER_FAIL_IF(import.kindIndex >= m_result.module->firstInternalGlobal, "get_global import kind index ", import.kindIndex, " exceeds the first internal global ", m_result.module->firstInternalGlobal);
 
-        ASSERT(m_result.module->globals[import.kindIndex].mutability == Global::Immutable);
+        WASM_PARSER_FAIL_IF(index >= m_result.module->globals.size(), "get_global's index ", index, " exceeds the number of globals ", m_result.module->globals.size());
+        WASM_PARSER_FAIL_IF(index >= m_result.module->firstInternalGlobal, "get_global import kind index ", index, " exceeds the first internal global ", m_result.module->firstInternalGlobal);
+
+        ASSERT(m_result.module->globals[index].mutability == Global::Immutable);
         resultType = m_result.module->globals[index].type;
         bitsOrImportNumber = index;
         break;
index 0668b1e..9e98674 100644 (file)
@@ -224,7 +224,13 @@ JSValue WebAssemblyModuleRecord::evaluate(ExecState* state)
             if (!element.functionIndices.size())
                 continue;
 
-            uint32_t tableIndex = element.offset;
+            uint32_t tableIndex;
+
+            if (element.offset.isGlobalImport())
+                tableIndex = static_cast<uint32_t>(m_instance->loadI32Global(element.offset.globalImportIndex()));
+            else
+                tableIndex = element.offset.constValue();
+
             uint64_t lastWrittenIndex = static_cast<uint64_t>(tableIndex) + static_cast<uint64_t>(element.functionIndices.size()) - 1;
             if (lastWrittenIndex >= table->size())
                 return throwException(state, scope, createJSWebAssemblyLinkError(state, vm, ASCIILiteral("Element is trying to set an out of bounds table index")));