1247855 - Support names in WAST text format.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Michael Bebenita [:mbx]]

Attachment: local_names.patch

Adds support for local names.

Comment 2

[Michael Bebenita [:mbx]]

Luke, my current approach is to resolve all name references to indices while parsing. This works for locals since we see all of their names upfront, before we parse get/set_locals. However, in case of function and type references, we may have not seen a function before we call it. What would you recommend? Should I just keep a list of AST nodes that need to be patched and do the name-to-index resolution after I've parsed the entire module?

Comment 3

[Luke Wagner [:luke]]

Great, thanks!

For the full wasm s-expr language, there are also other categories of names which I think can be defined/used out of order (types, signatures, tables), so since performance is zero concern here, I'd just add a second "resolve" pass over the whole AST (called by wasm::TextToBinary, after ParseModule).  Perhaps have the parsing pass build all the names into tables and the resolve pass does all the name lookups to set indices.  Any AST node that now holds an index could be given an additional name field and the name-resolution pass would test each node to see if it had a name and, if so, set the index.

Comment 4

[Luke Wagner [:luke]]

Btw, I was realizing that, even if we changed binaryen to emit name-less s-exprs, we'd still like to be able to run the .wast tests in spec/ml-proto/test unmodified, so glad to have this!

Comment 5

[Michael Bebenita [:mbx]]

Attachment: names.patch

Comment 6

[Michael Bebenita [:mbx]]

This is as far as I can get for now. Waiting on 1246116 to land before I can implement labels.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8720119 [details] [diff] [review]
names.patch

Review of attachment 8720119 [details] [diff] [review]:
-----------------------------------------------------------------

Great patch, very clean and simple!  Mostly just SM/asmjs style nits below:

::: js/src/asmjs/WasmText.cpp
@@ +205,5 @@
>      uint32_t localIndex() const {
>          return localIndex_;
>      }
> +    void setLocalIndex(uint32_t localIndex) {
> +        localIndex_ = localIndex;

How about we create a (non-member) static const uint32_t WasmNoIndex = UINT32_MAX and then establish a pattern (here and in the 5 other AST nodes which have names/indices):
 - the ctor has MOZ_ASSERT(localName.isEmpty() ^ (localIndex == WasmNoIndex))
 - localIndex() asserts localIndex_ != WasmNoIndex
 - setLocalIndex() asserts localIndex_ == WasmNoIndex

@@ +1765,5 @@
> +    uint32_t index;
> +
> +    if (c.ts.getIf(WasmToken::Name, &token)) {
> +        name = token.name();
> +    } else if (c.ts.match(WasmToken::Index, &token, c.error)) {

How about moving the definition of ParseNameOrIndex up and reusing it here?

@@ +2092,5 @@
> +        *name = token.name();
> +    } else if (c.ts.match(WasmToken::Index, &token, c.error)) {
> +        *index = token.index();
> +    } else {
> +        return false;

Since we either get one of these two tokens or fail, how about:

  WasmToken token = c.ts.get();
  switch (token.kind()) {
    case WasmToken::Name:
      ...
    case WasmToken::Index:
      ...
    default:
      c.ts.generateError(token, c.error);
      return nullptr;
  }

which is what we do in a few other places when we have this situation.

@@ +2400,5 @@
> +    WasmName funcName;
> +    WasmToken nameToken;
> +    if (c.ts.getIf(WasmToken::Name, &nameToken)) {
> +        funcName = nameToken.name();
> +    }

SM style has no { } around single-line then branch.  Actually, since this pattern appears thrice below, how about adding a new `WasmName WasmTokenStream::getIfName()` which reduces this pattern to one line.

@@ +2534,5 @@
>        case WasmToken::Index:
> +        return new(c.lifo) WasmAstExport(name.text(), WasmName(), exportee.index());
> +      case WasmToken::Name: {
> +        return new(c.lifo) WasmAstExport(name.text(), exportee.name(), 0);
> +      }

no { }

@@ +2621,5 @@
> +    JSContext* cx_;
> +    WasmNameMap varMap_;
> +    WasmNameMap funcMap_;
> +    WasmNameMap importMap_;
> +    bool registerName(WasmNameMap& map, WasmName name, size_t index)

Can you put a \n before this function?

@@ +2638,5 @@
> +      : cx_(cx)
> +    {
> +        init();
> +    }
> +    bool init()

This function is fallible I think you need to take out the init() call from the ctor and instead have put it in ResolveModule (so you can propagate failure).

@@ +2641,5 @@
> +    }
> +    bool init()
> +    {
> +        return funcMap_.init() && importMap_.init() && varMap_.init();
> +    }

Except for the case of ctors with member initializers, the usual style is to put the { of a member function on the preceding line.  Can you do that for all these members?

@@ +2660,5 @@
> +        return registerName(varMap_, varName, index);
> +    }
> +    bool resolveFuncName(bool isCall, WasmName funcName, size_t* index)
> +    {
> +        WasmNameMap::Ptr p = (isCall ? funcMap_ : importMap_).lookup(funcName);

Both Expr::Call and Expr::CallImport are "calls", so how about, since you already have this nice pattern established, just having separate resolveFuncName/resolveImportName?  The code duplication is pretty minimal.

@@ +2743,5 @@
> +    if (sl.localName().isNull())
> +        return true;
> +
> +    size_t index;
> +    if (r.resolveVarName(sl.localName(), &index)) {

Here and above x2, can you invert branch so it's "if (!r.resolve...)"?  The general asmjs/ style is for the last line of a fallible function be the return true (which is nice if you later add more failure conditions).

@@ +2842,5 @@
> +
> +static bool
> +ResolveFunc(Resolver& r, WasmAstFunc& func)
> +{
> +    r.beginFunc();

Could you insert a \n after this line?

@@ +2850,5 @@
> +        if (name.isNull())
> +            continue;
> +        if (!r.registerVarName(name, i)) {
> +            return r.fail("duplicate var");
> +        }

no { } around single-line then-branch

@@ +2889,5 @@
> +            continue;
> +        WasmName funcName = export_->funcName();
> +        if (funcName.isNull()) {
> +            continue;
> +        }

ditto

@@ +2895,5 @@
> +        if (r.resolveFuncName(true, funcName, &index)) {
> +            export_->setFuncIndex(index);
> +        } else {
> +            return r.fail("function not found");
> +        }

ditto (even when an else-branch is present if both are single-line)

@@ +3148,2 @@
>  {
> +    UniqueChars utf8(JS::CharsToNewUTF8CharsZ(nullptr, TwoByteChars(wasmName.begin(), wasmName.length())).c_str());

This goes beyond 100 column limit.  Rather than adding an awkward-looking line break, how about:

    TwoByteChars range(wasmName.begin(), wasmName.length());
    UniqueChars utf8(JS::CharsToNewUTF8CharsZ(nullptr, range).c_str());

::: js/src/asmjs/WasmText.h
@@ -26,5 @@
>  namespace wasm {
>  
>  // Translate the textual representation of a wasm module (given by a
> -// null-terminated char16_t array) into a Bytecode object. If there is an error
> -// other than out-of-memory an error message string will be stored in 'error'.

I appreciate that this is a simpler interface, but the reason I put it this way is that I wanted to keep a hard boundary where TextToBinary was independent of JSContext* so that, in the future, we could use it in other contexts where an error isn't something you report but, rather, something you display as an error message (think wasm in WebIDE).  Can you change this interface/comment back?

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8720119 [details] [diff] [review]
names.patch

Review of attachment 8720119 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmText.cpp
@@ +2618,5 @@
> +
> +class Resolver
> +{
> +    JSContext* cx_;
> +    WasmNameMap varMap_;

GCC warns that we should put Resolver in an unnamed namespace since its members are in an unnamed namespace.

Comment 9

[Michael Bebenita [:mbx]]

(In reply to Luke Wagner [:luke] from comment #7)

> I appreciate that this is a simpler interface, but the reason I put it this
> way is that I wanted to keep a hard boundary where TextToBinary was
> independent of JSContext* so that, in the future, we could use it in other
> contexts where an error isn't something you report but, rather, something
> you display as an error message (think wasm in WebIDE).  Can you change this
> interface/comment back?

The reason I did this was because I wanted to sometimes report TypeErrors instead of SyntaxErrors.

Comment 10

[Luke Wagner [:luke]]

(In reply to Michael Bebenita [:mbx] from comment #9)
> The reason I did this was because I wanted to sometimes report TypeErrors
> instead of SyntaxErrors.

Ah, I'd report SyntaxErrors for everything.  This is useful for the testing harness b/c the tests can assertThrowsInstanceOf(..., TypeError | SyntaxError) to indicate whether the thrown error is meant to pop out of wasmTextToBinary (SyntaError) or wasmEval (TypeError).  This is all kinda cheating since normally these exceptions only pop out of JS parsing.  However, wasmTextToBinary/wasmEval won't ever be directly content-visible so we can do whatever. The ultimate content-visible JS builtin that gets defined, "WASM.compile" will likely return a Promise and so won't be throwing TypeErrors either.

Comment 11

[Michael Bebenita [:mbx]]

Attachment: names.patch

Rebased and addresses all review comments.

Comment 12

[Michael Bebenita [:mbx]]

Looks like master now also has support for sigs so I'll update this to handle named sigs.

Comment 13

[Michael Bebenita [:mbx]]

Attachment: names.patch

Resolve signatures and table references.

Comment 14

[Michael Bebenita [:mbx]]

Comment on attachment 8720713 [details] [diff] [review]
names.patch

Might want to give this another quick look, after rebasing I had to make some changes. Most notably, there's now a WasmRef class. It's just a name/index pair and makes the code a lot cleaner. I also added support for the new AST nodes that you've added.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 8720713 [details] [diff] [review]
names.patch

Very nice, WasmRef is a great idea.

Comment 16

[Luke Wagner [:luke]]

Pushed with a few minor nits fixed:

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/500bdcc53e54

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/500bdcc53e54