Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

wasm-edit -- Valid edits can throw with default decode options. #405

Open
TimHambourger opened this issue Jul 3, 2018 · 5 comments · May be fixed by #409
Open

wasm-edit -- Valid edits can throw with default decode options. #405

TimHambourger opened this issue Jul 3, 2018 · 5 comments · May be fixed by #409
Labels
bug pkg: wasm-gen

Comments

@TimHambourger
Copy link
Contributor

Description

Because of the assertNotIdentifier calls in wasm-gen/lib/encoder/index.js, editing, say, a module export name can throw with the default decode options.

Steps to repro

Start with input WASM corresponding to the WAT

(module
  (func $add (param $lhs i32) (param $rhs i32) (result i32)
    get_local $lhs
    get_local $rhs
    i32.add)
  (export "add" (func $add))
)

(from MDN's examples).

Then edit the export name like

import { edit } from '@webassemblyjs/wasm-edit';

const bin; // ArrayBuffer containing .wasm contents

edit(bin, {
    ModuleExport(path) {
        path.node.name += '!!';
    }
});

This throws with "Unsupported node Identifier" due to the assertion at

function encodeModuleExport(n) {
  var out = [];
  assertNotIdentifierNode(n.descr.id);
  // etc.
}

If you instead decode with ignoreCustomNameSection: true then the edit succeeds:

import { decode } from '@webassemblyjs/wasm-parser';
import { editWithAST } from '@webassemblyjs/wasm-edit';

const bin; // ArrayBuffer containing .wasm contents

const ast = decode(bin, {
    ignoreCustomNameSection: true
});

editWithAST(ast, bin, {
    ModuleExport(path) {
        path.node.name += '!!';
    }
});

At the relevant assertion, n.node.descr now has type 'NumberLiteral' instead of 'Identifier'.

Expected behavior

That valid edits succeed with default decode options. The current behavior feels restrictive.

Other Cases?

This behavior with export names seems to repro reliably across input .wasm files. I also found a possible repro with import names, but that one only affected some of the files I tested and might be due to a bug with decode. I'll file that separately.
@TimHambourger TimHambourger mentioned this issue Jul 3, 2018
Closed
@ColinEberhardt
Copy link
Collaborator

Thanks for the detailed report. As you've no doubt worked out this issue is due to the presence of a custom name section within your wasm binary.

The binary encoded form of wasm doesn't retain function / local names, instead these are all referenced by index. When generating a wasm binary you can optionally supply this information in the form of a custom section. With wat2wasm this is done via the debug-names option:

wat2wasm test.wat -o test.wasm --debug-names

The default behaviour of @webassemblyjs/wasm-parser is to read this custom name section (if present) and restore the names in within the AST.

Unfortunately the @webassemblyjs/wasm-gen doesn't support the 'inverse' of this.

The wasm-gen module should probably be updated to do the following:

  1. If functions / locals within the AST are 'named', convert these to indices and write the wasm binary - this would solve your immediate problem.
  2. Optionally (although perhaps by default to mirror wasm-parse) write these to a custom name section

I might give this a go.

@xtuc
Copy link
Owner

xtuc commented Jul 4, 2018 •
edited
Loading

Hi @TimHambourger, thanks for your bug reports (and the great test suite, do you mind if we use it?).

This issue is due to a design bug. Here's the definition of a ModuleExport: https://github.com/xtuc/webassemblyjs/blob/master/packages/ast/src/types/nodes.js#L289-L301, where name is the exported identifier and descr.id is the reference to the exported element.

  • wasm-gen asserts that descr.id is an index because that's mapping the wasm format.
  • wasm-parse can resolve names and will replace descr.id by the internal name.
  • optionally, for wast-printer, it's more readable if descr.id is an identifier.

Here are the fixes I have in mind:

@ColinEberhardt
Copy link
Collaborator

wasm-parser: as we saw before, both (index and identifier) are important informations. I would suggest to simply add both to your AST. (i'm 👍 on this).

Just to clarify, it sounds like you are proposing that the transformation that uses the custom name section to update index-based references to named references should not be destructive?

i.e. remove Identifier from the union type here https://github.com/xtuc/webassemblyjs/blob/master/packages/ast/src/types/basic.js#L37 and instead make it an optional property on relevant nodes so that the index is retained?

That gets a 👍 from me!

@xtuc
Copy link
Owner

xtuc commented Jul 4, 2018

Yes, that's right, for example a ModuleExport would have an index and optionnally a name. We can then use the information we need depending on what we do.

@TimHambourger
Copy link
Contributor Author

Just to clarify, it sounds like you are proposing that the transformation that uses the custom name section to update index-based references to named references should not be destructive?
...
That gets a 👍 from me!

Agreed, this sounds like a great approach! @xtuc, yes, feel free to use any of my tests as you see fit.

@xtuc xtuc linked a pull request Jul 7, 2018 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug pkg: wasm-gen
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor index xtuc/webassemblyjs
3 participants
@ColinEberhardt @xtuc @TimHambourger