From f47b672db24766910f476ff601c6d3df5175089b Mon Sep 17 00:00:00 2001 From: Sven SAULEAU Date: Mon, 28 May 2018 10:17:50 +0200 Subject: [PATCH 1/9] feat: add failing test --- .../validation/test/fixtures/global-initilizer/module.wast | 4 ++++ .../validation/test/fixtures/global-initilizer/output.txt | 1 + 2 files changed, 5 insertions(+) create mode 100644 packages/validation/test/fixtures/global-initilizer/module.wast create mode 100644 packages/validation/test/fixtures/global-initilizer/output.txt diff --git a/packages/validation/test/fixtures/global-initilizer/module.wast b/packages/validation/test/fixtures/global-initilizer/module.wast new file mode 100644 index 000000000..c36cbbd94 --- /dev/null +++ b/packages/validation/test/fixtures/global-initilizer/module.wast @@ -0,0 +1,4 @@ +(module + (global (mut i32) (i32.const 1)) + (global i32 (get_global 0)) +) diff --git a/packages/validation/test/fixtures/global-initilizer/output.txt b/packages/validation/test/fixtures/global-initilizer/output.txt new file mode 100644 index 000000000..dfb1841f6 --- /dev/null +++ b/packages/validation/test/fixtures/global-initilizer/output.txt @@ -0,0 +1 @@ +initializer expression can only reference an imported global From 7593a90ec8618cb6d7ab7472d768ec4607ab0b53 Mon Sep 17 00:00:00 2001 From: Mauro Bringolf Date: Fri, 8 Jun 2018 00:33:12 +0200 Subject: [PATCH 2/9] Move isConst into validation, check mutability of globals --- packages/helper-module-context/src/index.js | 2 +- packages/validation/src/index.js | 20 ++++++--- packages/validation/src/is-const.js | 39 +++++++++++------ packages/validation/src/type-checker.js | 6 +-- .../module.wast | 3 ++ .../fixtures/global-initializer/output.txt | 1 + .../fixtures/global-initilizer/output.txt | 1 - packages/validation/test/index.js | 4 +- packages/validation/test/is-const.js | 42 ------------------- .../src/interpreter/runtime/values/global.js | 6 +-- 10 files changed, 50 insertions(+), 74 deletions(-) rename packages/validation/test/fixtures/{global-initilizer => global-initializer}/module.wast (55%) create mode 100644 packages/validation/test/fixtures/global-initializer/output.txt delete mode 100644 packages/validation/test/fixtures/global-initilizer/output.txt delete mode 100644 packages/validation/test/is-const.js diff --git a/packages/helper-module-context/src/index.js b/packages/helper-module-context/src/index.js index a742eda53..9e357f5a4 100644 --- a/packages/helper-module-context/src/index.js +++ b/packages/helper-module-context/src/index.js @@ -207,7 +207,7 @@ export class ModuleContext { defineGlobal(global /*: Global*/) { const type = global.globalType.valtype; - const mutability = global.mutability; + const mutability = global.globalType.mutability; this.globals.push({ type, mutability }); diff --git a/packages/validation/src/index.js b/packages/validation/src/index.js index 5796b4b8c..c389ca7f0 100644 --- a/packages/validation/src/index.js +++ b/packages/validation/src/index.js @@ -1,13 +1,12 @@ // @flow import importOrderValidate from "./import-order"; +import isConst from "./is-const"; import typeChecker from "./type-checker"; +import { moduleContextFromModuleAST } from "@webassemblyjs/helper-module-context"; export default function validateAST(ast: Program) { - const errors = []; - - errors.push(...importOrderValidate(ast)); - errors.push(...typeChecker(ast)); + const errors = getValidationErrors(ast); if (errors.length !== 0) { const errorMessage = "Validation errors:\n" + errors.join("\n"); @@ -16,7 +15,18 @@ export default function validateAST(ast: Program) { } } -export { isConst } from "./is-const"; +export function getValidationErrors(ast: Program): Array { + const errors = []; + const moduleContext = moduleContextFromModuleAST(ast.body[0]); + + errors.push(...isConst(ast, moduleContext)); + errors.push(...importOrderValidate(ast)); + errors.push(...typeChecker(ast, moduleContext)); + + return errors; +} + export { getType, typeEq } from "./type-inference"; +export { isConst }; export const stack = typeChecker; diff --git a/packages/validation/src/is-const.js b/packages/validation/src/is-const.js index f9227fdca..efb91d7d7 100644 --- a/packages/validation/src/is-const.js +++ b/packages/validation/src/is-const.js @@ -1,5 +1,7 @@ // @flow +import { traverse } from "@webassemblyjs/ast"; + /** * Determine if a sequence of instructions form a constant expression * @@ -8,23 +10,18 @@ * TODO(sven): get_global x should check the mutability of x, but we don't have * access to the program at this point. */ -export function isConst(instrs: Array): boolean { - if (instrs.length === 0) { - return false; - } - - return instrs.reduce((acc, instr) => { - // Bailout - if (acc === false) { - return acc; - } - +export default function isConst( + ast: Program, + moduleContext: Object +): Array { + function isConstInstruction(instr): boolean { if (instr.id === "const") { return true; } if (instr.id === "get_global") { - return true; + const index = instr.args[0].value; + return !moduleContext.isMutableGlobal(index); } // FIXME(sven): this shoudln't be needed, we need to inject our end @@ -34,5 +31,21 @@ export function isConst(instrs: Array): boolean { } return false; - }, true); + } + + const errors = []; + + traverse(ast, { + Global(path) { + const isValid = path.node.init.reduce( + (acc, instr) => acc && isConstInstruction(instr), + true + ); + if (!isValid) { + errors.push("initializer expression cannot reference mutable global"); + } + } + }); + + return errors; } diff --git a/packages/validation/src/type-checker.js b/packages/validation/src/type-checker.js index ba4fda666..fc09c852d 100644 --- a/packages/validation/src/type-checker.js +++ b/packages/validation/src/type-checker.js @@ -1,6 +1,5 @@ import { traverse, isInstruction } from "@webassemblyjs/ast"; -import { moduleContextFromModuleAST } from "@webassemblyjs/helper-module-context"; import getType from "./type-checker/get-type.js"; import { ANY, POLYMORPHIC } from "./type-checker/types.js"; @@ -30,14 +29,11 @@ function checkTypes(a, b) { } } -export default function validate(ast) { +export default function validate(ast, moduleContext) { if (!ast.body || !ast.body[0] || !ast.body[0].fields) { return []; } - // Module context - const moduleContext = moduleContextFromModuleAST(ast.body[0]); - errors = []; // Simulate stack types throughout all function bodies diff --git a/packages/validation/test/fixtures/global-initilizer/module.wast b/packages/validation/test/fixtures/global-initializer/module.wast similarity index 55% rename from packages/validation/test/fixtures/global-initilizer/module.wast rename to packages/validation/test/fixtures/global-initializer/module.wast index c36cbbd94..7574ddbd9 100644 --- a/packages/validation/test/fixtures/global-initilizer/module.wast +++ b/packages/validation/test/fixtures/global-initializer/module.wast @@ -1,4 +1,7 @@ (module (global (mut i32) (i32.const 1)) (global i32 (get_global 0)) + + (global i32 (i32.const 0)) + (global i32 (get_global 1)) ) diff --git a/packages/validation/test/fixtures/global-initializer/output.txt b/packages/validation/test/fixtures/global-initializer/output.txt new file mode 100644 index 000000000..2cd11005d --- /dev/null +++ b/packages/validation/test/fixtures/global-initializer/output.txt @@ -0,0 +1 @@ +initializer expression cannot reference mutable global diff --git a/packages/validation/test/fixtures/global-initilizer/output.txt b/packages/validation/test/fixtures/global-initilizer/output.txt deleted file mode 100644 index dfb1841f6..000000000 --- a/packages/validation/test/fixtures/global-initilizer/output.txt +++ /dev/null @@ -1 +0,0 @@ -initializer expression can only reference an imported global diff --git a/packages/validation/test/index.js b/packages/validation/test/index.js index cb0961e94..c38b4c4c2 100644 --- a/packages/validation/test/index.js +++ b/packages/validation/test/index.js @@ -22,7 +22,7 @@ describe("validation", () => { describe("wast", () => { const pre = f => { - const errors = validations.stack(parse(f)); + const errors = validations.getValidationErrors(parse(f)); return errorsToString(errors); }; @@ -35,7 +35,7 @@ describe("validation", () => { const module = wabt.parseWat(suite, f); const { buffer } = module.toBinary({ write_debug_names: false }); - const errors = validations.stack(decode(buffer)); + const errors = validations.getValidationErrors(decode(buffer)); return errorsToString(errors); }; diff --git a/packages/validation/test/is-const.js b/packages/validation/test/is-const.js deleted file mode 100644 index ba541e0cf..000000000 --- a/packages/validation/test/is-const.js +++ /dev/null @@ -1,42 +0,0 @@ -// @flow - -const t = require("@webassemblyjs/ast"); -const { assert } = require("chai"); - -const { isConst } = require("../lib/is-const"); - -function i32ConstOf(v) { - return t.objectInstruction("const", "i32", [t.numberLiteralFromRaw(v)]); -} - -describe("validation", () => { - describe("is const", () => { - it("should return true for a const expression", () => { - const exprs = [t.objectInstruction("const", "i32")]; - - assert.isTrue(isConst(exprs)); - }); - - it("should return false if not constant", () => { - const exprs = [t.objectInstruction("neg", "i32", [i32ConstOf(1)])]; - - assert.isFalse(isConst(exprs)); - }); - - it("should return false if empty", () => { - assert.isFalse(isConst([])); - }); - - it("should return false with multiple non const expressions", () => { - const exprs = [i32ConstOf(1), t.instruction("nop")]; - - assert.isFalse(isConst(exprs)); - }); - - it("should return true with multiple const expressions", () => { - const exprs = [i32ConstOf(1), i32ConstOf(2)]; - - assert.isTrue(isConst(exprs)); - }); - }); -}); diff --git a/packages/webassemblyjs/src/interpreter/runtime/values/global.js b/packages/webassemblyjs/src/interpreter/runtime/values/global.js index 304b99a5b..2af3ad91d 100644 --- a/packages/webassemblyjs/src/interpreter/runtime/values/global.js +++ b/packages/webassemblyjs/src/interpreter/runtime/values/global.js @@ -1,6 +1,6 @@ // @flow -import { isConst, getType, typeEq } from "@webassemblyjs/validation"; +import { getType, typeEq } from "@webassemblyjs/validation"; const { evaluate } = require("../../partial-evaluation"); const { CompileError } = require("../../../errors"); @@ -12,10 +12,6 @@ export function createInstance( let value; const { valtype, mutability } = node.globalType; - if (node.init.length > 0 && isConst(node.init) === false) { - throw new CompileError("constant expression required"); - } - // None or multiple constant expressions in the initializer seems not possible // TODO(sven): find a specification reference for that // FIXME(sven): +1 because of the implicit end, change the order of validations From bcab50d06d1665a8622ee3210981081468a89936 Mon Sep 17 00:00:00 2001 From: Mauro Bringolf Date: Fri, 8 Jun 2018 11:41:59 +0200 Subject: [PATCH 3/9] Fix moduleContext instantiation --- packages/validation/src/index.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/validation/src/index.js b/packages/validation/src/index.js index c389ca7f0..250d6cf70 100644 --- a/packages/validation/src/index.js +++ b/packages/validation/src/index.js @@ -17,11 +17,14 @@ export default function validateAST(ast: Program) { export function getValidationErrors(ast: Program): Array { const errors = []; - const moduleContext = moduleContextFromModuleAST(ast.body[0]); - errors.push(...isConst(ast, moduleContext)); - errors.push(...importOrderValidate(ast)); - errors.push(...typeChecker(ast, moduleContext)); + ast.body.filter(({ type }) => type === "Module").forEach(m => { + const moduleContext = moduleContextFromModuleAST(m); + + errors.push(...isConst(ast, moduleContext)); + errors.push(...importOrderValidate(ast)); + errors.push(...typeChecker(ast, moduleContext)); + }); return errors; } From d436777b11fc877efca6ee08666a2a6c6854777b Mon Sep 17 00:00:00 2001 From: Mauro Bringolf Date: Mon, 11 Jun 2018 18:31:50 +0200 Subject: [PATCH 4/9] Make repl use the new validation API --- packages/repl/src/index.js | 4 ++-- packages/validation/src/index.js | 12 +++++++++++- packages/validation/src/is-const.js | 7 +++---- .../test/fixtures/global-initializer/output.txt | 2 +- 4 files changed, 17 insertions(+), 8 deletions(-) diff --git a/packages/repl/src/index.js b/packages/repl/src/index.js index 84fe72dcb..080501ba8 100644 --- a/packages/repl/src/index.js +++ b/packages/repl/src/index.js @@ -12,7 +12,7 @@ const { } = require("webassemblyjs/lib/interpreter/kernel/memory"); const { decode } = require("@webassemblyjs/wasm-parser"); const t = require("@webassemblyjs/ast"); -const typeCheck = require("@webassemblyjs/validation").stack; +const { getValidationErrors } = require("@webassemblyjs/validation"); const denormalizeTypeReferences = require("@webassemblyjs/ast/lib/transform/denormalize-type-references") .transform; @@ -308,7 +308,7 @@ export function createRepl({ isVerbose, onAssert, onLog, onOk }) { if (enableTypeChecking === true) { denormalizeTypeReferences(moduleNode); - const typeErrors = typeCheck(t.program([moduleNode])); + const typeErrors = getValidationErrors(t.program([moduleNode])); if (typeErrors.length > 0) { const containsImmutableGlobalViolation = typeErrors.some( diff --git a/packages/validation/src/index.js b/packages/validation/src/index.js index 250d6cf70..c08f2db9d 100644 --- a/packages/validation/src/index.js +++ b/packages/validation/src/index.js @@ -18,7 +18,17 @@ export default function validateAST(ast: Program) { export function getValidationErrors(ast: Program): Array { const errors = []; - ast.body.filter(({ type }) => type === "Module").forEach(m => { + let modules = []; + + if (ast.type === "Module") { + modules = [ast]; + } + + if (ast.type === "Program") { + modules = ast.body.filter(({ type }) => type === "Module"); + } + + modules.forEach(m => { const moduleContext = moduleContextFromModuleAST(m); errors.push(...isConst(ast, moduleContext)); diff --git a/packages/validation/src/is-const.js b/packages/validation/src/is-const.js index efb91d7d7..811fb8675 100644 --- a/packages/validation/src/is-const.js +++ b/packages/validation/src/is-const.js @@ -6,9 +6,6 @@ import { traverse } from "@webassemblyjs/ast"; * Determine if a sequence of instructions form a constant expression * * See https://webassembly.github.io/spec/core/multipage/valid/instructions.html#valid-constant - * - * TODO(sven): get_global x should check the mutability of x, but we don't have - * access to the program at this point. */ export default function isConst( ast: Program, @@ -42,7 +39,9 @@ export default function isConst( true ); if (!isValid) { - errors.push("initializer expression cannot reference mutable global"); + errors.push( + "constant expression required: initializer expression cannot reference mutable global" + ); } } }); diff --git a/packages/validation/test/fixtures/global-initializer/output.txt b/packages/validation/test/fixtures/global-initializer/output.txt index 2cd11005d..5d9660c10 100644 --- a/packages/validation/test/fixtures/global-initializer/output.txt +++ b/packages/validation/test/fixtures/global-initializer/output.txt @@ -1 +1 @@ -initializer expression cannot reference mutable global +constant expression required: initializer expression cannot reference mutable global From 84fd9a1499fd34a55e320228acb5f692be1e8c1c Mon Sep 17 00:00:00 2001 From: Sven Sauleau Date: Sat, 18 Aug 2018 14:20:41 +0200 Subject: [PATCH 5/9] Update type-checker.js --- packages/validation/src/type-checker.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/validation/src/type-checker.js b/packages/validation/src/type-checker.js index fd149c729..ab82ddcae 100644 --- a/packages/validation/src/type-checker.js +++ b/packages/validation/src/type-checker.js @@ -1,5 +1,6 @@ import { traverse, isInstruction } from "@webassemblyjs/ast"; +import { moduleContextFromModuleAST } from "@webassemblyjs/helper-module-context"; import getType from "./type-checker/get-type.js"; import { ANY, POLYMORPHIC } from "./type-checker/types.js"; @@ -22,13 +23,6 @@ function createTypeChecker() { errors.push(msg); } -export default function validate(ast, moduleContext) { - if (!ast.body || !ast.body[0] || !ast.body[0].fields) { - return []; - } - - errors = []; - function checkTypes(a, b, index) { if (a === ANY && b) { return; From c10941dcac99487c837b1fb12c3fbd67aa6ff00d Mon Sep 17 00:00:00 2001 From: Sven SAULEAU Date: Sat, 18 Aug 2018 14:35:33 +0200 Subject: [PATCH 6/9] refactor: global import mutability validation --- packages/validation/src/imports.js | 27 +++++++++++++++++++ packages/validation/src/index.js | 2 ++ .../src/interpreter/runtime/values/module.js | 5 ---- 3 files changed, 29 insertions(+), 5 deletions(-) create mode 100644 packages/validation/src/imports.js diff --git a/packages/validation/src/imports.js b/packages/validation/src/imports.js new file mode 100644 index 000000000..e8fa50547 --- /dev/null +++ b/packages/validation/src/imports.js @@ -0,0 +1,27 @@ +// @flow + +import { traverse } from "@webassemblyjs/ast"; + +/** + * Determine if a sequence of instructions form a constant expression + * + * See https://webassembly.github.io/spec/core/multipage/valid/instructions.html#valid-constant + */ +export default function ( + ast: Program, + moduleContext: Object +): Array { + const errors = []; + + traverse(ast, { + ModuleImport({ node }) { + const { mutability } = node.descr; + + if (mutability === "var") { + errors.push("mutable globals cannot be imported"); + } + } + }); + + return errors; +} diff --git a/packages/validation/src/index.js b/packages/validation/src/index.js index c08f2db9d..f2629c4ee 100644 --- a/packages/validation/src/index.js +++ b/packages/validation/src/index.js @@ -3,6 +3,7 @@ import importOrderValidate from "./import-order"; import isConst from "./is-const"; import typeChecker from "./type-checker"; +import imports from "./imports"; import { moduleContextFromModuleAST } from "@webassemblyjs/helper-module-context"; export default function validateAST(ast: Program) { @@ -31,6 +32,7 @@ export function getValidationErrors(ast: Program): Array { modules.forEach(m => { const moduleContext = moduleContextFromModuleAST(m); + errors.push(...imports(ast, moduleContext)); errors.push(...isConst(ast, moduleContext)); errors.push(...importOrderValidate(ast)); errors.push(...typeChecker(ast, moduleContext)); diff --git a/packages/webassemblyjs/src/interpreter/runtime/values/module.js b/packages/webassemblyjs/src/interpreter/runtime/values/module.js index e13961238..a4a205e2c 100644 --- a/packages/webassemblyjs/src/interpreter/runtime/values/module.js +++ b/packages/webassemblyjs/src/interpreter/runtime/values/module.js @@ -59,11 +59,6 @@ function instantiateImports( } function handleGlobalImport(node: ModuleImport, descr: GlobalType) { - // Validation: The mutability of globaltype must be const. - if (descr.mutability === "var") { - throw new CompileError("Mutable globals cannot be imported"); - } - const element = getExternalElementOrThrow(node.module, node.name); const externglobalinstance = externvalue.createGlobalInstance( From 8c888344d7ba214e7f058f85cee6c83b1d182169 Mon Sep 17 00:00:00 2001 From: Sven SAULEAU Date: Sat, 18 Aug 2018 14:43:16 +0200 Subject: [PATCH 7/9] fix: linting errors --- packages/validation/src/imports.js | 5 ++--- packages/validation/src/index.js | 3 +++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/validation/src/imports.js b/packages/validation/src/imports.js index e8fa50547..f92e0762c 100644 --- a/packages/validation/src/imports.js +++ b/packages/validation/src/imports.js @@ -7,9 +7,8 @@ import { traverse } from "@webassemblyjs/ast"; * * See https://webassembly.github.io/spec/core/multipage/valid/instructions.html#valid-constant */ -export default function ( - ast: Program, - moduleContext: Object +export default function( + ast: Program /*, moduleContext: Object */ ): Array { const errors = []; diff --git a/packages/validation/src/index.js b/packages/validation/src/index.js index f2629c4ee..b1d011126 100644 --- a/packages/validation/src/index.js +++ b/packages/validation/src/index.js @@ -21,10 +21,12 @@ export function getValidationErrors(ast: Program): Array { let modules = []; + // $FlowIgnore if (ast.type === "Module") { modules = [ast]; } + // $FlowIgnore if (ast.type === "Program") { modules = ast.body.filter(({ type }) => type === "Module"); } @@ -32,6 +34,7 @@ export function getValidationErrors(ast: Program): Array { modules.forEach(m => { const moduleContext = moduleContextFromModuleAST(m); + // $FlowIgnore errors.push(...imports(ast, moduleContext)); errors.push(...isConst(ast, moduleContext)); errors.push(...importOrderValidate(ast)); From 03baa2375397115fce76302a504b5040a016c572 Mon Sep 17 00:00:00 2001 From: Sven SAULEAU Date: Sat, 18 Aug 2018 14:49:51 +0200 Subject: [PATCH 8/9] fix: increase test timeout --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 8f9f26f5e..61e021779 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ NODE_OPTS = -TEST_TIMEOUT = 6000 +TEST_TIMEOUT = 7000 LERNA = ./node_modules/.bin/lerna FLOWTYPED = ./node_modules/.bin/flow-typed From cca0451c60448dc94c14f572471de789e1f82c6d Mon Sep 17 00:00:00 2001 From: Sven SAULEAU Date: Sat, 18 Aug 2018 14:52:54 +0200 Subject: [PATCH 9/9] fix: increase test timeout again --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 61e021779..f88fb1e3d 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ NODE_OPTS = -TEST_TIMEOUT = 7000 +TEST_TIMEOUT = 10000 LERNA = ./node_modules/.bin/lerna FLOWTYPED = ./node_modules/.bin/flow-typed