[BinAST] Implement multipart binjs tokenizer

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

(Blocks 4 bugs)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(10 attachments, 5 obsolete attachments)

59 bytes, text/x-review-board-request
jorendorff
: review+
Details
59 bytes, text/x-review-board-request
arai
: review+
Details
59 bytes, text/x-review-board-request
arai
: review+
Details
59 bytes, text/x-review-board-request
arai
: review+
Details
59 bytes, text/x-review-board-request
arai
: review+
Details
59 bytes, text/x-review-board-request
arai
: review+
Details
59 bytes, text/x-review-board-request
arai
: review+
Details
59 bytes, text/x-review-board-request
arai
: review+
efaust
: review+
Details
59 bytes, text/x-review-board-request
arai
: review+
Details
59 bytes, text/x-review-board-request
arai
: review+
Details
We currently implement only the "simple" tokenizer, designed for debugging.

We should implement the "multipart" tokenizer, which is designed to be actually deployed, and which is bound to be more fragile to fuzzing (and attacks).
Priority: -- → P2
Assignee: nobody → dteller
Depends on: 1437004
Summary: Implement multipart binjs tokenizer → [BinAST] Implement multipart binjs tokenizer
Attachment #8960512 - Attachment is obsolete: true
Attachment #8960919 - Attachment is obsolete: true
Attachment #8961200 - Attachment is obsolete: true
Attachment #8961392 - Attachment is obsolete: true
Comment on attachment 8965574 [details]
Bug 1439855 - Extending BinAST parser generator with support for multipart tokenizer;

https://reviewboard.mozilla.org/r/234382/#review240310

::: js/src/frontend/binsource/src/main.rs:369
(Diff revision 1)
>          option_parsers_to_generate.sort_by(|a, b| str::cmp(a.name.to_str(), b.name.to_str()));
>  
> +        // Prepare variant_by_symbol, which will let us lookup the BinVariant name of
> +        // a symbol. Since some symbols can appear in several enums (e.g. "+"
> +        // is both a unary and a binary operator), we need to collect all the
> +        // string enums that contain each symbol and come up with a unique name.

These aren't really guaranteed unique, right? We could someday have `enum Foo` with variant `BazBar` and `enum FooBaz` with variant `Bar` (or heaven forbid interfaces with `Or` in the name).

I'm not actually worried about it, no change needed. Just trying to make sure I haven't missed something, since the comment says "unique".

::: js/src/frontend/binsource/src/main.rs:370
(Diff revision 1)
>  
> +        // Prepare variant_by_symbol, which will let us lookup the BinVariant name of
> +        // a symbol. Since some symbols can appear in several enums (e.g. "+"
> +        // is both a unary and a binary operator), we need to collect all the
> +        // string enums that contain each symbol and come up with a unique name.
> +        let mut enum_by_string : HashMap<String, Vec<std::rc::Rc<String>>> = HashMap::new();

The code below seems like it'd read nicer (and do fewer string copies) if the HashMap values were `Vec<NodeName>`.

::: js/src/frontend/binsource/src/main.rs:415
(Diff revision 1)
>          let type_ok = self.get_type_ok(name, default_type_ok);
>          let kind = name.to_class_cases();
>          format!("    JS::Result<{type_ok}> parse{prefix}{kind}({args});\n",
>              prefix = prefix,
>              type_ok = type_ok,
> -            kind = kind,
> +            kind = kind.to_class_cases(),

This string was already converted to class-case a few lines earlier, I think.

::: js/src/frontend/binsource/src/main.rs:426
(Diff revision 1)
>          let type_ok = self.get_type_ok(name, default_type_ok);
>          let kind = name.to_class_cases();
> -        format!("JS::Result<{type_ok}>\nBinASTParser::parse{prefix}{kind}({args})",
> +        format!("template<typename Tok> JS::Result<{type_ok}>\nBinASTParser<Tok>::parse{prefix}{kind}({args})",
>              prefix = prefix,
>              type_ok = type_ok,
> -            kind = kind,
> +            kind = kind.to_class_cases(),

Same here.

::: js/src/frontend/binsource/src/main.rs:493
(Diff revision 1)
> +                Ord::cmp(name_1, name_2)
> +                    .then_with(|| Ord::cmp(symbol_1, symbol_2))
> +            });
> +
> +        buffer.push_str(&format!("\n#define FOR_EACH_BIN_VARIANT(F) \\\n{nodes}\n",
> +            nodes = enum_variants.drain(..)

Use `.into_iter()` here instead of `.drain(..)`, then remove the `mut` from `enum_variants`.

::: js/src/frontend/binsource/src/main.rs:496
(Diff revision 1)
> +
> +        buffer.push_str(&format!("\n#define FOR_EACH_BIN_VARIANT(F) \\\n{nodes}\n",
> +            nodes = enum_variants.drain(..)
> +                .map(|(symbol, name)| format!("    F({variant_name}, \"{spec_name}\")",
> +                    spec_name = symbol,
> +                    variant_name = name))                .format(" \\\n")));

Looks like you meant to put that `.format()` on a separate line?
Attachment #8965574 - Flags: review?(jorendorff) → review+
Comment on attachment 8965575 [details]
Bug 1439855 - Splitting the BinTokenReaderTester in two;

https://reviewboard.mozilla.org/r/234384/#review240444

Partial review here.
I'll read the multipart documentation first, and then review the multipart tokenizer.

r+ with the comments addressed, and the reason for `BinTokenReaderBase::offset()` change is clarified.

::: js/src/frontend/BinTokenReaderBase.cpp:1
(Diff revision 1)
> +#include "frontend/BinSource-macros.h"
> +#include "frontend/BinTokenReaderBase.h"

I think placing `#include "frontend/BinTokenReaderBase.h"` (and empty line after that) before `#include "frontend/BinSource-macros.h"` is the convension.

Also, not a big deal tho, this file doesn't have modelines and license block, compared to other files.

also, BinSource-macros.h is the file that is introduced in the later patch, right?
(it's better adding the file first)

::: js/src/frontend/BinTokenReaderBase.cpp:58
(Diff revision 1)
> +
> +
> +size_t
> +BinTokenReaderBase::offset() const
> +{
> +    return current_ - start_;

this is different than current `BinTokenReaderTester::offset()`,
what's the reason behind the change?

::: js/src/frontend/BinTokenReaderTester.h:124
(Diff revision 1)
> -     *
> -     * WARNING: At this stage, the `string` encoding has NOT been validated.
> +    MOZ_MUST_USE JS::Result<JSAtom*> readMaybeAtom();
> +    MOZ_MUST_USE JS::Result<JSAtom*> readAtom();

Please describe the difference between readMaybeAtom and readAtom here (since the return type doesn't have `Maybe<>`)

::: js/src/frontend/BinTokenReaderTester.h:131
(Diff revision 1)
> +
> +
> +    /**
> +     * Read a single `string | null` value.
>       *
> -     * @return false If a string could not be read. In this case, an error
> +     * MAY check if that string is not valid UTF-8.

what does "MAY" actually mean?
what's the difference between `readAtom`'s "Fails if that string is not valid UTF-8" ?

::: js/src/frontend/BinTokenReaderTester.cpp:14
(Diff revision 1)
> +
> +

please remove these empty lines.

::: js/src/frontend/BinTokenReaderTester.cpp:31
(Diff revision 1)
> -}
> -
> -bool
> -BinTokenReaderTester::readBuf(uint8_t* bytes, uint32_t len)
>  {
> -    MOZ_ASSERT(!cx_->isExceptionPending());
> +    return Ok();

might be nice to add some comment that clarifies this filetype doesn't have header?
(instead of that this is not-yet-implemented)

::: js/src/frontend/BinTokenReaderTester.cpp:150
(Diff revision 1)
>      if (current_ + byteLen > stop_)
>          return raiseError("Not enough bytes to read chars");
>  
>      if (byteLen == 2 && *current_ == 255 && *(current_ + 1) == 0) {
>          // 3. Special case: null string.
> -        out = Nothing();
> +        result = nullptr;

this is not necessary, because it's already null.
but I'm fine with explicitly assigining for clarifying the special case.

::: js/src/frontend/BinTokenReaderTester.cpp:196
(Diff revision 1)
> +
> +    // 4. Other strings (bytes are copied)
> +    if (!out.resize(byteLen))
> +        return raiseOOM();
> +
> +    PodCopy(out.begin(), current_, byteLen);

`current_ += byteLen` should be performed here.

::: js/src/frontend/BinTokenReaderTester.cpp:216
(Diff revision 1)
> +}
>  
> -    // Perform lookup, without side-effects.
> -    if (!std::equal(current_, current_ + N - 1 /*implicit NUL*/, value))
> -        return false;
> +JS::Result<BinVariant>
> +BinTokenReaderTester::readVariant()
> +{
> +    RootedAtom atom(cx_);

this is not used.

::: js/src/frontend/BinTokenReaderTester.cpp:234
(Diff revision 1)
> -}
> +    }
>  
> +    BinaryASTSupport::CharSlice slice((const char*)current_, byteLen);
> +    current_ += byteLen;
> +
> +    MOZ_TRY_DECL(variant, cx_->runtime()->binast().binVariant(cx_, slice));

`binVariant` is the one introduced in the next patch, right?

::: js/src/frontend/BinTokenReaderTester.cpp:242
(Diff revision 1)
>  // - "<tuple>";
>  // - contents (specified by the higher-level grammar);

pre-existing tho, just noticed that the existence of trailing semi-colon seems to be random, across the other comments.
Attachment #8965575 - Flags: review?(arai.unmht) → review+
Comment on attachment 8965576 [details]
Bug 1439855 - Fast lookup for BinAST string constants, shared among parsers;

https://reviewboard.mozilla.org/r/234386/#review240448

r+ with the comments addressed.

I prefer fixing the `NamedValue.name_` to `const char*` tho, if there's reason behind the current structure, I'm fine with keeping it,
but please add some comment in that case.

::: js/src/frontend/BinSourceRuntimeSupport.h:37
(Diff revision 1)
> +        CharSlice(const char* start, const uint32_t byteLen) :
> +            start_(start),

":" should be placed before the first member.
like
```
        CharSlice(const char* start, const uint32_t byteLen)
          : start_(start),
```

here and above.

::: js/src/frontend/BinSourceRuntimeSupport.h:49
(Diff revision 1)
> +        const char* end() const {
> +            return start_ + byteLen_;
> +        }
> +#ifdef DEBUG
> +        void dump() const {
> +            for (auto c: *this) {

braces can be removed.

::: js/src/frontend/BinSourceRuntimeSupport.h:90
(Diff revision 1)
> +        }
> +        T value() const {
> +            return value_;
> +        }
> +      private:
> +        JS::UniqueChars name_; // The key is a slice into `name`.

looks like the `name_` can just be a `const char*` to static string (in `BINKIND_DESCRIPTIONS` etc) for each case.
that way you don't have to allocate+copy when initializing,
also I think you can just use a loop over them, instead of generating same code for each item.

::: js/src/frontend/BinToken.cpp:7
(Diff revision 1)
> +#include "frontend/BinSourceRuntimeSupport.h"
>  #include "frontend/BinToken.h"

BinToken.h should be the first item,
and others should be placed after mozilla/Maybe.h

::: js/src/frontend/BinToken.cpp:33
(Diff revision 1)
> +    #define WITH_VARIANT(_, SPEC_NAME) #SPEC_NAME,
> +        FOR_EACH_BIN_VARIANT(WITH_VARIANT)
> +    #undef WITH_VARIANT

you can remove leading 4 spaces for each line.
here and `BINFIELD_DESCRIPTION`

::: js/src/frontend/BinToken.cpp:46
(Diff revision 1)
>  
>  const char* describeBinField(const BinField& field) {
>      return BINFIELD_DESCRIPTIONS[static_cast<size_t>(field)];
>  }
>  
> +const char* describeBinVariant(const BinVariant& field) {

pre-existing tho, please place "{" into its own line.
here and other functions.

::: js/src/frontend/BinToken.cpp:63
(Diff revision 1)
> +            JS::UniqueChars name(static_cast<char*>(js_malloc(sizeof(#SPEC_NAME))));  \
> +            if (!name) {                                                              \
> +                ReportOutOfMemory(cx);                                                \
> +                return cx->alreadyReportedError();                                    \
> +            }                                                                         \
> +            memcpy(name.get(), #SPEC_NAME, sizeof(#SPEC_NAME));                   \
> +            NamedValue<BinKind> value(Move(name), BinKind::CPP_NAME);             \

Please see the comment for `NamedValue.name_` in BinSourceRuntimeSupport.h.
these code seems to be unnecessary, here and other cases.

::: js/src/frontend/BinToken.cpp:72
(Diff revision 1)
> +            auto ptr = binKindMap_.lookupForAdd(key);                             \
> +            MOZ_ASSERT(!ptr);                                                     \
> +            if (!binKindMap_.add(ptr, Move(key), Move(value)))                    \
> +                return cx->alreadyReportedError();                                \

these code can be shared between items.

::: js/src/vm/Runtime.h:938
(Diff revision 1)
>      // List of all the live wasm::Instances in the runtime. Equal to the union
>      // of all instances registered in all JSCompartments. Accessed from watchdog
>      // threads for purposes of wasm::InterruptRunningCode().
>      js::ExclusiveData<js::wasm::InstanceVector> wasmInstances;
>  
> -  public:
> +public:

please undo the dedent here.
Attachment #8965576 - Flags: review?(arai.unmht) → review+
Comment on attachment 8965577 [details]
Bug 1439855 - Bunch of macros shared among BinAST files;

https://reviewboard.mozilla.org/r/234388/#review240450

r+ with the `#undef` things somehow fixed.

::: js/src/frontend/BinSource-macros.h:7
(Diff revision 1)
> + * vim: set ts=8 sts=4 et sw=4 tw=99:
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef frontend_BinSourceMacros_h

`frontend_BinSource_macros_h` maybe?

::: js/src/frontend/BinSource-macros.h:69
(Diff revision 1)
> +#undef TRY
> +#undef TRY_VAR
> +#undef TRY_DECL
> +#undef TRY_EMPL
> +#undef MOZ_TRY_EMPLACE
> +#undef MOZ_TRY_DECL

it's better providing a file for it, if this is really necessary,
so that the consumer doesn't have to know the list of macros defined in this file.

I'm not sure if this is necessary tho...
Attachment #8965577 - Flags: review?(arai.unmht) → review+
Comment on attachment 8965578 [details]
Bug 1439855 - Extend JS shell binParse with ability to pick a tokenizer;

https://reviewboard.mozilla.org/r/234390/#review240452

r+ with the comments addressed.

::: js/src/shell/js.cpp:4335
(Diff revision 1)
> +        RootedValue optionFormat(cx);
> +        if (!JS_GetProperty(cx, objOptions, "format", &optionFormat))
> +            return false;
> +
> +        if (optionFormat.isUndefined()) {
> +            useMultipart = true;

`useMultiplart` is always `true`.
maybe you want to se it to `false` here?
or it should be set to `false` at the beginning?

actually I don't understand why you're checking `isUndefine` here.
can you add some comment about the `options` parameter details?

::: js/src/shell/js.cpp:4409
(Diff revision 1)
>      const char16_t* chars = stableChars.twoByteRange().begin().get();
>  
>      CompileOptions options(cx);
>      options.setIntroductionType("js shell parse")
> -           .setFileAndLine("<string>", 1);
> +           .setFileAndLine("<string>", 1)
> +           .setAllowSyntaxParser(allowSyntaxParser);

this change should be in the next patch.
Attachment #8965578 - Flags: review?(arai.unmht) → review+
Comment on attachment 8965579 [details]
Bug 1439855 - Extend JS shell parse command to allow forcing full parsing;

https://reviewboard.mozilla.org/r/234392/#review240454

::: js/src/shell/js.cpp:4409
(Diff revision 1)
> +        if (!optionAllowSyntaxParser.isUndefined()) {
> +            if (optionAllowSyntaxParser.isBoolean()) {

how about this?
```
if (optionAllowSyntaxParser.isBoolean()) {
...
} else if (!optionAllowSyntaxParser.isUndefined()) {
...
}
```


(might be nice to add some helper function that checks the property existence first, and return the value, and use it everywhere tho, it's for another bug)
Attachment #8965579 - Flags: review?(arai.unmht) → review+
Comment on attachment 8965580 [details]
Bug 1439855 - Introduce BinAST multipart tokenizer;

https://reviewboard.mozilla.org/r/234394/#review240456

Just to confirm, "part" in "multipart" means grammar table, string table, and the tree, right?
(at first I thought it's about a single file with multiple trees)

::: js/src/frontend/BinTokenReaderMultipart.h:36
(Diff revision 1)
> +    class AutoTuple;
> +    class AutoTaggedTuple;
> +
> +    using CharSlice = BinaryASTSupport::CharSlice;
> +
> +    // This implementation of `BinFields` is effectively `void`.

can you add some comment for *why* it's void?

::: js/src/frontend/BinTokenReaderMultipart.h:140
(Diff revision 1)
> +     * @param tag (OUT) The tag of the tuple.
> +     * @param guard (OUT) A guard, ensuring that we read the tagged tuple correctly.

`@param fields` is missing.

::: js/src/frontend/BinTokenReaderMultipart.h:175
(Diff revision 1)
> +    MOZ_MUST_USE JS::Result<uint32_t> readInternalUint32();
> +
> +  private:
> +    // A mapping grammar index => BinKind, as defined by the [grammar]
> +    // section of the file. Populated during readHeader().
> +    Vector<BinKind> grammarTable;

please append `_` to each member,
so that it's clear what's member and what's local.

::: js/src/frontend/BinTokenReaderMultipart.h:177
(Diff revision 1)
> +  private:
> +    // A mapping grammar index => BinKind, as defined by the [grammar]
> +    // section of the file. Populated during readHeader().
> +    Vector<BinKind> grammarTable;
> +
> +    // A mapping string index => BinVariant as extracted from the [strings]

"[GRAMMAR]" and "[STRINGS]" should be capital I think?

::: js/src/frontend/BinTokenReaderMultipart.h:236
(Diff revision 1)
> +      public:
> +        explicit AutoList(BinTokenReaderMultipart& reader);
> +
> +        // Check that we have properly read to the end of the list.
> +        MOZ_MUST_USE JS::Result<Ok> done();
> +      protected:

can you put an empty line before "protected:" and "private:" ?

::: js/src/frontend/BinTokenReaderMultipart.cpp:3
(Diff revision 1)
> +#include "frontend/BinSource-macros.h"
> +#include "frontend/BinSourceRuntimeSupport.h"
> +#include "frontend/BinTokenReaderMultipart.h"

BinTokenReaderMultipart.h should be the first item,
and others should be placed after "mozilla/..."

::: js/src/frontend/BinTokenReaderMultipart.cpp:38
(Diff revision 1)
> +    : BinTokenReaderBase(cx, start, length)
> +    , grammarTable(cx)
> +    , atomsTable(cx, AtomVector(cx))
> +    , slicesTable(cx)
> +    , posBeforeTree(nullptr)

please remove 2 spaces for each line

::: js/src/frontend/BinTokenReaderMultipart.cpp:54
(Diff revision 1)
> +    if (version != MAGIC_FORMAT_VERSION)
> +        return raiseError("Format version not implemented");

wouldn't it be better checking `version > MAGIC_FORMAT_VERSION` ?
or perhaps backward compatibility is not guaranteed at this point?

::: js/src/frontend/BinTokenReaderMultipart.cpp:88
(Diff revision 1)
> +        if (!grammarTable.append(*kind)) // We called `reserve` before the loop.
> +            MOZ_CRASH();

you can use `infallibleAppend` here, and in other similar code.

::: js/src/frontend/BinTokenReaderMultipart.cpp:135
(Diff revision 1)
> +        if (!atomsTable.append(atom))
> +            MOZ_CRASH(); // We have reserved before entering the loop.
> +
> +        // Populate `slicesTable`: i => slice
> +        Chars slice((const char*)current_, byteLen);
> +        if (!slicesTable.append(Move(slice)))

if we atomize the strings, what's the point of having slicesTable and CharSlice methods?
will they be merged with atom in the future?

::: js/src/frontend/BinTokenReaderMultipart.cpp:149
(Diff revision 1)
> +    // Start reading AST.
> +    MOZ_TRY(readConst(SECTION_HEADER_TREE));
> +    MOZ_TRY(readConst(COMPRESSION_IDENTITY)); // For the moment, we only support identity compression.
> +    posBeforeTree = current_;
> +
> +    MOZ_TRY_DECL(treeByteLen, readInternalUint32());

don't we check the position after reading tree?

::: js/src/frontend/BinTokenReaderMultipart.cpp:208
(Diff revision 1)
> +    MOZ_TRY(readBuf(reinterpret_cast<uint8_t*>(bytes), ArrayLength(bytes)));
> +
> +    // Decode little-endian.
> +    const uint64_t asInt = LittleEndian::readUint64(bytes);
> +
> +    if (asInt == 0x7FF0000000000001)

it's better using some constant, instead of magic number, given that it already appears in 2 places
(4 places including comment)

::: js/src/frontend/BinTokenReaderMultipart.cpp:237
(Diff revision 1)
> +    RootedAtom atom(cx_);
> +    atom = atomsTable[index];
> +    return atom.get();

you don't have to root here.
just `return atomsTable[index]` will work
Attachment #8965580 - Flags: review?(arai.unmht) → review+
Comment on attachment 8965581 [details]
Bug 1439855 - Make BinSource work with multipart tokenizer;

https://reviewboard.mozilla.org/r/234396/#review240674

r=me with readHeader comment explained. The rest of the changes look sensible and good.

::: js/src/frontend/BinSource-auto.h:86
(Diff revision 1)
>  
>  
>  // ----- Sums of interfaces (by lexicographical order)
>  // Implementations are autogenerated
>  // `ParseNode*` may never be nullptr
> -JS::Result<ParseNode*> parseAssignmentTarget();JS::Result<ParseNode*> parseAssignmentTargetOrAssignmentTargetWithInitializer();JS::Result<ParseNode*> parseAssignmentTargetPattern();JS::Result<ParseNode*> parseAssignmentTargetProperty();JS::Result<ParseNode*> parseBinding();JS::Result<ParseNode*> parseBindingOrBindingWithInitializer();JS::Result<ParseNode*> parseBindingPattern();JS::Result<ParseNode*> parseBindingProperty();JS::Result<ParseNode*> parseExportDeclaration();JS::Result<ParseNode*> parseExpression();JS::Result<ParseNode*> parseExpressionOrSuper();JS::Result<ParseNode*> parseExpressionOrTemplateElement();JS::Result<ParseNode*> parseForInOfBindingOrAssignmentTarget();JS::Result<ParseNode*> parseFunctionBodyOrExpression();JS::Result<ParseNode*> parseFunctionDeclarationOrClassDeclarationOrExpression();JS::Result<ParseNode*> parseFunctionDeclarationOrClassDeclarationOrVariableDeclaration();JS::Result<ParseNode*> parseImportDeclaration();JS::Result<ParseNode*> parseImportDeclarationOrExportDeclarationOrStatement();JS::Result<ParseNode*> parseIterationStatement();JS::Result<ParseNode*> parseLiteral();JS::Result<ParseNode*> parseMethodDefinition();JS::Result<ParseNode*> parseObjectProperty();JS::Result<ParseNode*> parseParameter();JS::Result<ParseNode*> parseProgram();JS::Result<ParseNode*> parsePropertyName();JS::Result<ParseNode*> parseSimpleAssignmentTarget();JS::Result<ParseNode*> parseSpreadElementOrExpression();JS::Result<ParseNode*> parseStatement();JS::Result<ParseNode*> parseVariableDeclarationOrExpression();JS::Result<ParseNode*> parseSumAssignmentTarget(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumAssignmentTargetOrAssignmentTargetWithInitializer(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumAssignmentTargetPattern(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumAssignmentTargetProperty(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumBinding(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumBindingOrBindingWithInitializer(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumBindingPattern(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumBindingProperty(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumExportDeclaration(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumExpression(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumExpressionOrSuper(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumExpressionOrTemplateElement(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumForInOfBindingOrAssignmentTarget(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumFunctionBodyOrExpression(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumFunctionDeclarationOrClassDeclarationOrExpression(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumFunctionDeclarationOrClassDeclarationOrVariableDeclaration(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumImportDeclaration(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumImportDeclarationOrExportDeclarationOrStatement(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumIterationStatement(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumLiteral(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumMethodDefinition(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumObjectProperty(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumParameter(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumProgram(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumPropertyName(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumSimpleAssignmentTarget(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumSpreadElementOrExpression(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumStatement(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumVariableDeclarationOrExpression(const size_t start, const BinKind kind, const BinFields& fields);
> +JS::Result<ParseNode*> parseArrowExpression();JS::Result<ParseNode*> parseAssignmentTarget();JS::Result<ParseNode*> parseAssignmentTargetOrAssignmentTargetWithInitializer();JS::Result<ParseNode*> parseAssignmentTargetPattern();JS::Result<ParseNode*> parseAssignmentTargetProperty();JS::Result<ParseNode*> parseBinding();JS::Result<ParseNode*> parseBindingOrBindingWithInitializer();JS::Result<ParseNode*> parseBindingPattern();JS::Result<ParseNode*> parseBindingProperty();JS::Result<ParseNode*> parseExportDeclaration();JS::Result<ParseNode*> parseExpression();JS::Result<ParseNode*> parseExpressionOrSuper();JS::Result<ParseNode*> parseExpressionOrTemplateElement();JS::Result<ParseNode*> parseForInOfBindingOrAssignmentTarget();JS::Result<ParseNode*> parseFunctionBodyOrExpression();JS::Result<ParseNode*> parseFunctionDeclaration();JS::Result<ParseNode*> parseFunctionDeclarationOrClassDeclarationOrExpression();JS::Result<ParseNode*> parseFunctionDeclarationOrClassDeclarationOrVariableDeclaration();JS::Result<ParseNode*> parseFunctionExpression();JS::Result<ParseNode*> parseGetter();JS::Result<ParseNode*> parseImportDeclaration();JS::Result<ParseNode*> parseImportDeclarationOrExportDeclarationOrStatement();JS::Result<ParseNode*> parseIterationStatement();JS::Result<ParseNode*> parseLiteral();JS::Result<ParseNode*> parseMethod();JS::Result<ParseNode*> parseMethodDefinition();JS::Result<ParseNode*> parseObjectProperty();JS::Result<ParseNode*> parseParameter();JS::Result<ParseNode*> parseProgram();JS::Result<ParseNode*> parsePropertyName();JS::Result<ParseNode*> parseSetter();JS::Result<ParseNode*> parseSimpleAssignmentTarget();JS::Result<ParseNode*> parseSpreadElementOrExpression();JS::Result<ParseNode*> parseStatement();JS::Result<ParseNode*> parseVariableDeclarationOrExpression();JS::Result<ParseNode*> parseSumArrowExpression(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumAssignmentTarget(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumAssignmentTargetOrAssignmentTargetWithInitializer(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumAssignmentTargetPattern(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumAssignmentTargetProperty(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumBinding(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumBindingOrBindingWithInitializer(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumBindingPattern(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumBindingProperty(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumExportDeclaration(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumExpression(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumExpressionOrSuper(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumExpressionOrTemplateElement(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumForInOfBindingOrAssignmentTarget(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumFunctionBodyOrExpression(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumFunctionDeclaration(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumFunctionDeclarationOrClassDeclarationOrExpression(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumFunctionDeclarationOrClassDeclarationOrVariableDeclaration(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumFunctionExpression(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumGetter(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumImportDeclaration(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumImportDeclarationOrExportDeclarationOrStatement(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumIterationStatement(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumLiteral(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumMethod(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumMethodDefinition(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumObjectProperty(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumParameter(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumProgram(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumPropertyName(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumSetter(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumSimpleAssignmentTarget(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumSpreadElementOrExpression(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumStatement(const size_t start, const BinKind kind, const BinFields& fields);JS::Result<ParseNode*> parseSumVariableDeclarationOrExpression(const size_t start, const BinKind kind, const BinFields& fields);

Obviously it doesn't really matter, but for human-readability it would be nice to put a newline in the header generation code after each method def.

I guess the point of putting them on one line is that humans never care, since they're just pulling the tuples apart?

::: js/src/frontend/BinSource.cpp:112
(Diff revision 1)
>  
>      ParseContext::VarScope varScope(cx_, &globalpc, usedNames_);
>      if (!varScope.init(&globalpc))
>          return cx_->alreadyReportedError();
>  
> +    MOZ_TRY(tokenizer_->readHeader());

Does this belong here? I don't see any other references to it in either the new code in this patch, or the new generated parser.
Attachment #8965581 - Flags: review?(efaustbmo) → review+
Comment on attachment 8965581 [details]
Bug 1439855 - Make BinSource work with multipart tokenizer;

https://reviewboard.mozilla.org/r/234396/#review240462

I cannot review Cargo.lock, I'd forward it to someone else (I'm not sure who can tho...)

::: commit-message-10840:3
(Diff revision 1)
> +Bug 1439855 - Make BinSource work with multipart tokenizer;r?arai,efaust
> +
> +Note that BinSource-auto.{h, cpp} and BinToken.h are auto-generated from BinSource.{yaml, webidl}.

pre-existing tho, it would be nice to add "-auto" suffix to all auto-generated files (of course in other bug).
given that currently some binast-related files have "-auto" suffix, it makes me feel that files without "-auto" suffix is *not* auto-generated.

(might be nice to apply this rule to other auto-generated files, like Unicode.cpp etc?)

::: js/src/frontend/BinSource-auto.cpp:6996
(Diff revision 1)
> -    // Unoptimized implementation.
> +    MOZ_TRY_DECL(variant, tokenizer_->readVariant());
> -    Chars chars(cx_);
> -    MOZ_TRY(readString(chars));
>  
> -    if (chars == ",")
> +    switch (variant) {
> +    case BinVariant::BinaryOperatorComma:

2 more spaces needed

::: js/src/frontend/BinSource-auto.cpp:7684
(Diff revision 1)
>  #undef TRY
>  #undef TRY_VAR
>  #undef TRY_DECL
> +#undef TRY_EMPL
> +#undef MOZ_TRY_EMPLACE
>  #undef MOZ_TRY_DECL

as mentioned in the previous review, this isn't good.
these lines should be replaced with single #include or something like that.

::: js/src/frontend/BinSource.h:113
(Diff revision 1)
> +    friend void TraceBinParser(JSTracer* trc, AutoGCRooter* parser);
> +
> +  protected:
> +    JSContext* cx_;
> +
> +// ---- Memory-related stuff

indentation is inconsistent.
please add 4 spaces at the beginning of this line

::: js/src/frontend/BinSource.h:125
(Diff revision 1)
> +    ParseNodeAllocator nodeAlloc_;
> +
> +    // Root atoms and objects allocated for the parse tree.
> +    AutoKeepAtoms keepAtoms_;
> +
> +// ---- Parsing-related stuff

and here

::: js/src/frontend/BinSource.h:260
(Diff revision 1)
> +        public:
> +        explicit AutoVariableDeclarationKind(BinASTParser<Tok>* parser
> +                                                MOZ_GUARD_OBJECT_NOTIFIER_PARAM) :

please remove 2 spaces before "public:",
and please align "M" of "MOZ_GUARD..." to "B" of "BinASTParser",
and please put ":" in the next line.

::: js/src/frontend/BinSource.cpp:8
(Diff revision 1)
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "frontend/BinSource.h"
> +#include "frontend/BinSource-macros.h"

this should be placed later, with other headers
Attachment #8965581 - Flags: review?(arai.unmht) → review+
Comment on attachment 8965574 [details]
Bug 1439855 - Extending BinAST parser generator with support for multipart tokenizer;

https://reviewboard.mozilla.org/r/234382/#review240310

> These aren't really guaranteed unique, right? We could someday have `enum Foo` with variant `BazBar` and `enum FooBaz` with variant `Bar` (or heaven forbid interfaces with `Or` in the name).
> 
> I'm not actually worried about it, no change needed. Just trying to make sure I haven't missed something, since the comment says "unique".

Indeed, it is possible that we may encounter the problem in the future. I tweaked the comment to take this into account.

> Use `.into_iter()` here instead of `.drain(..)`, then remove the `mut` from `enum_variants`.

Ah, nice, I learnt something today :)
Comment on attachment 8965575 [details]
Bug 1439855 - Splitting the BinTokenReaderTester in two;

https://reviewboard.mozilla.org/r/234384/#review240444

> this is different than current `BinTokenReaderTester::offset()`,
> what's the reason behind the change?

With the multipart tokenizer, I started breaking a number of offset-related assertions in `FullParseHandler`. At some point, I realized that this offset() algorithm worked better with them. Nothing deeper than that :/

> what does "MAY" actually mean?
> what's the difference between `readAtom`'s "Fails if that string is not valid UTF-8" ?

Actually, it's a difference between the two tokenizers. The multipart one guarantees that the string is UTF-8, the other doesn't.

> `current_ += byteLen` should be performed here.

Mmmh... Obviously, I failed to run the tests ok this tokenizer after the patch.

Good catch, thanks!

> `binVariant` is the one introduced in the next patch, right?

Right.
Comment on attachment 8965576 [details]
Bug 1439855 - Fast lookup for BinAST string constants, shared among parsers;

https://reviewboard.mozilla.org/r/234386/#review240448

> looks like the `name_` can just be a `const char*` to static string (in `BINKIND_DESCRIPTIONS` etc) for each case.
> that way you don't have to allocate+copy when initializing,
> also I think you can just use a loop over them, instead of generating same code for each item.

Good idea.
Comment on attachment 8965577 [details]
Bug 1439855 - Bunch of macros shared among BinAST files;

https://reviewboard.mozilla.org/r/234388/#review240450

I can't think of any good way to fix it.

> it's better providing a file for it, if this is really necessary,
> so that the consumer doesn't have to know the list of macros defined in this file.
> 
> I'm not sure if this is necessary tho...

`BinSource-macros-undef.h`? Yeah, that would work.

What else do you suggest? These macros will kill us if we ever attempt again to perform unified compilation.
Comment on attachment 8965577 [details]
Bug 1439855 - Bunch of macros shared among BinAST files;

https://reviewboard.mozilla.org/r/234388/#review240450

> `BinSource-macros-undef.h`? Yeah, that would work.
> 
> What else do you suggest? These macros will kill us if we ever attempt again to perform unified compilation.

What's the actual problem which happens if you don't undef them?

(also, the current one doesn't work with unified compilation, since the file is enclosed by ifdef+define.
you have to undef `frontend_BinSourceMacros_h` at the end of each file, in order to effectively include it again later)

then, if the problem is something critical and cannot be solved without undef-ing them, I think adding -undef.h file is the simple solution, so that you don't have to maintain the consistency between multiple files.

other way would be embedding undef into BinSource-macros.h, enclosed by some ifdef/ifndef, like:

```
// in BinSource-macros.h

// for each macro
#ifndef UNDEF_BINSOURCE_MACROS
#define TRY ...
#else
#undef TRY
#endif

...

// at the end of file
#ifdef UNDEF_BINSOURCE_MACROS
#undef UNDEF_BINSOURCE_MACROS
#endif

---

// in each file that uses macro

// at the end of file
#define UNDEF_BINSOURCE_MACROS
#include "BinSource-macros.h"
```

so that you can manage the list of macros in single place, but that would be a bit tricky...
Comment on attachment 8965578 [details]
Bug 1439855 - Extend JS shell binParse with ability to pick a tokenizer;

https://reviewboard.mozilla.org/r/234390/#review240452

> `useMultiplart` is always `true`.
> maybe you want to se it to `false` here?
> or it should be set to `false` at the beginning?
> 
> actually I don't understand why you're checking `isUndefine` here.
> can you add some comment about the `options` parameter details?

Actually, it's just that I forgot to implement the `MOZ_CRASH("Not implemented")` block. Let me fix that...
Comment on attachment 8965580 [details]
Bug 1439855 - Introduce BinAST multipart tokenizer;

https://reviewboard.mozilla.org/r/234394/#review240456

Yes, it's a one grammar table, one strings table, one ast. Future versions may have [several tables](https://github.com/binast/binjs-ref/issues/86), but we're not there yet.

> wouldn't it be better checking `version > MAGIC_FORMAT_VERSION` ?
> or perhaps backward compatibility is not guaranteed at this point?

At this point, `MAGIC_FORMAT_VERSION` is `0`, so it doesn't make much difference :) But yeah, I believe it's a bit too early to guarantee compatibility.

> if we atomize the strings, what's the point of having slicesTable and CharSlice methods?
> will they be merged with atom in the future?

Some API clients need the `JSAtom*`, some need the `const char*`. Since going from `JSAtom*` to `const char*` is non-trivial, fallible and may require memory allocations, we store both.

> don't we check the position after reading tree?

Yeah, we don't have a way to check that yet.
Comment on attachment 8965577 [details]
Bug 1439855 - Bunch of macros shared among BinAST files;

https://reviewboard.mozilla.org/r/234388/#review240450

> What's the actual problem which happens if you don't undef them?
> 
> (also, the current one doesn't work with unified compilation, since the file is enclosed by ifdef+define.
> you have to undef `frontend_BinSourceMacros_h` at the end of each file, in order to effectively include it again later)
> 
> then, if the problem is something critical and cannot be solved without undef-ing them, I think adding -undef.h file is the simple solution, so that you don't have to maintain the consistency between multiple files.
> 
> other way would be embedding undef into BinSource-macros.h, enclosed by some ifdef/ifndef, like:
> 
> ```
> // in BinSource-macros.h
> 
> // for each macro
> #ifndef UNDEF_BINSOURCE_MACROS
> #define TRY ...
> #else
> #undef TRY
> #endif
> 
> ...
> 
> // at the end of file
> #ifdef UNDEF_BINSOURCE_MACROS
> #undef UNDEF_BINSOURCE_MACROS
> #endif
> 
> ---
> 
> // in each file that uses macro
> 
> // at the end of file
> #define UNDEF_BINSOURCE_MACROS
> #include "BinSource-macros.h"
> ```
> 
> so that you can manage the list of macros in single place, but that would be a bit tricky...

> What's the actual problem which happens if you don't undef them?

If we ever remove `BinSource*.*` from the unified builds blacklist, we may end up with collisions on all these macros. I'm trying to save the next developer from a nervous breakdown :)

> so that you can manage the list of macros in single place, but that would be a bit tricky...

That's a tad footgunish, don't you think?
Attachment #8965582 - Attachment is obsolete: true
Comment on attachment 8965581 [details]
Bug 1439855 - Make BinSource work with multipart tokenizer;

https://reviewboard.mozilla.org/r/234396/#review241796

::: js/src/frontend/BinSource-auto.cpp:7678
(Diff revision 4)
> -#undef TRY
> -#undef TRY_VAR
> -#undef TRY_DECL
> -#undef TRY_EMPL
> +#undef BINJS_TRY
> +#undef BINJS_TRY_VAR
> +#undef BINJS_TRY_DECL
> +#undef BINJS_TRY_EMPL
>  #undef MOZ_TRY_EMPLACE
> -#undef MOZ_TRY_DECL
> +#undef BINJS_MOZ_TRY_DECL

now that they're defined in a header with include-guard,
and has specific prefix, you shouldn't undef them.

otherwise unified build will be broken, since if there are 2 files that includes the -macros.h, the 2nd one doesn't define them after undef, and the 2nd file cannot be compiled.
Comment on attachment 8967339 [details]
Bug 1439855 - Tests for multipart tokenizer;

https://reviewboard.mozilla.org/r/236032/#review242082
Attachment #8967339 - Flags: review?(arai.unmht) → review+
Comment on attachment 8967340 [details]
Bug 1439855 - Tests for BinAST multipart tokenizer (data);

https://reviewboard.mozilla.org/r/236034/#review242084
Attachment #8967340 - Flags: review?(arai.unmht) → review+
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41c442ae9e6c
Tests for multipart tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/f8dabfc553ff
Tests for BinAST multipart tokenizer (data);r=arai
https://hg.mozilla.org/integration/autoland/rev/2916fe19f035
Introduce BinAST multipart tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/59838090809b
Extending BinAST parser generator with support for multipart tokenizer;r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/00aa64418797
Splitting the BinTokenReaderTester in two;r=arai
https://hg.mozilla.org/integration/autoland/rev/16a32f1eddb2
Fast lookup for BinAST string constants, shared among parsers;r=arai
https://hg.mozilla.org/integration/autoland/rev/f5acf9dfb9ad
Bunch of macros shared among BinAST files;r=arai
https://hg.mozilla.org/integration/autoland/rev/e1ac1d0b4ea1
Extend JS shell binParse with ability to pick a tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/da50d0cc6c5b
Extend JS shell parse command to allow forcing full parsing;r=arai
https://hg.mozilla.org/integration/autoland/rev/4a36617f7f0d
Make BinSource work with multipart tokenizer;r=arai,efaust
Flags: needinfo?(dteller)
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0906a6c24238
Tests for multipart tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/a9b425f0d0f7
Tests for BinAST multipart tokenizer (data);r=arai
https://hg.mozilla.org/integration/autoland/rev/7112c12e331f
Introduce BinAST multipart tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/2e26c3859ed3
Extending BinAST parser generator with support for multipart tokenizer;r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/8e4d613963db
Splitting the BinTokenReaderTester in two;r=arai
https://hg.mozilla.org/integration/autoland/rev/e19f06ed8be9
Fast lookup for BinAST string constants, shared among parsers;r=arai
https://hg.mozilla.org/integration/autoland/rev/6e57f703e46d
Bunch of macros shared among BinAST files;r=arai
https://hg.mozilla.org/integration/autoland/rev/bb47995e6481
Extend JS shell binParse with ability to pick a tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/7eb7f7be58f0
Extend JS shell parse command to allow forcing full parsing;r=arai
https://hg.mozilla.org/integration/autoland/rev/8bdeab80ff08
Make BinSource work with multipart tokenizer;r=arai,efaust
I'm on it, thanks.
Flags: needinfo?(dteller)
Duplicate of this bug: 1454615
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0acfc9ae925d
Tests for multipart tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/6809950e6c1b
Tests for BinAST multipart tokenizer (data);r=arai
https://hg.mozilla.org/integration/autoland/rev/cf13314fb3bf
Introduce BinAST multipart tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/1878e7f3c0e7
Extending BinAST parser generator with support for multipart tokenizer;r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/5ab969c94c26
Splitting the BinTokenReaderTester in two;r=arai
https://hg.mozilla.org/integration/autoland/rev/c5362bed3eb3
Fast lookup for BinAST string constants, shared among parsers;r=arai
https://hg.mozilla.org/integration/autoland/rev/1025426ce7b9
Bunch of macros shared among BinAST files;r=arai
https://hg.mozilla.org/integration/autoland/rev/8f73e5e6ae40
Extend JS shell binParse with ability to pick a tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/879b279a5321
Extend JS shell parse command to allow forcing full parsing;r=arai
https://hg.mozilla.org/integration/autoland/rev/d364fb740439
Make BinSource work with multipart tokenizer;r=arai,efaust
Backed out 10 changesets (bug 1439855) for spidermonkey bustage at builds/worker/workspace/build/src/js/src/jsapi-tests/testBinTokenReaderTester.cpp

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d364fb74043959d131aeef565731501a9160f3af

Backout: https://hg.mozilla.org/integration/autoland/rev/868d65e224827a9ec7a065662a91854397845e2c.

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174282458&repo=autoland&lineNumber=90004

[task 2018-04-18T09:31:02.291Z] TEST-PASS | testDeepFreeze_bug535703 | ok
[task 2018-04-18T09:31:02.291Z] testDebugger_newScriptHook
[task 2018-04-18T09:31:02.291Z] TEST-PASS | testDebugger_newScriptHook | ok
[task 2018-04-18T09:31:02.291Z] testDateToLocaleString
[task 2018-04-18T09:31:02.291Z] TEST-PASS | testDateToLocaleString | ok
[task 2018-04-18T09:31:02.291Z] testCompileScript
[task 2018-04-18T09:31:02.291Z] TEST-PASS | testCompileScript | ok
[task 2018-04-18T09:31:02.291Z] test_cloneScriptWithPrincipals
[task 2018-04-18T09:31:02.291Z] TEST-PASS | test_cloneScriptWithPrincipals | ok
[task 2018-04-18T09:31:02.291Z] test_cloneScript
[task 2018-04-18T09:31:02.291Z] TEST-PASS | test_cloneScript | ok
[task 2018-04-18T09:31:02.291Z] testChromeBuffer
[task 2018-04-18T09:31:02.291Z] TEST-PASS | testChromeBuffer | ok
[task 2018-04-18T09:31:02.291Z] test_CallNonGenericMethodOnProxy
[task 2018-04-18T09:31:02.291Z] TEST-PASS | test_CallNonGenericMethodOnProxy | ok
[task 2018-04-18T09:31:02.291Z] testCallArgs_isConstructing_constructor
[task 2018-04-18T09:31:02.291Z] TEST-PASS | testCallArgs_isConstructing_constructor | ok
[task 2018-04-18T09:31:02.291Z] testCallArgs_isConstructing_native
[task 2018-04-18T09:31:02.291Z] TEST-PASS | testCallArgs_isConstructing_native | ok
[task 2018-04-18T09:31:02.291Z] testBug604087
[task 2018-04-18T09:31:02.291Z] TEST-PASS | testBug604087 | ok
[task 2018-04-18T09:31:02.291Z] testBoundFunction
[task 2018-04-18T09:31:02.291Z] TEST-PASS | testBoundFunction | ok
[task 2018-04-18T09:31:02.292Z] testBinTokenReaderTesterNestedList
[task 2018-04-18T09:31:02.292Z] TEST-UNEXPECTED-FAIL | testBinTokenReaderTesterNestedList | /builds/worker/workspace/build/src/js/src/jsapi-tests/testBinTokenReaderTester.cpp:296:CHECK_EQUAL failed: expected ((uint32_t)1) = 1, got (outerLength) = 73
[task 2018-04-18T09:31:02.292Z] testBinTokenReaderTesterSimpleList
[task 2018-04-18T09:31:02.292Z] TEST-UNEXPECTED-FAIL | testBinTokenReaderTesterSimpleList | /builds/worker/workspace/build/src/js/src/jsapi-tests/testBinTokenReaderTester.cpp:267:CHECK failed: length == 2
[task 2018-04-18T09:31:02.292Z] testBinTokenReaderTesterEmptyList
[task 2018-04-18T09:31:02.292Z] TEST-UNEXPECTED-FAIL | testBinTokenReaderTesterEmptyList | /builds/worker/workspace/build/src/js/src/jsapi-tests/testBinTokenReaderTester.cpp:247:CHECK failed: length == 0
[task 2018-04-18T09:31:02.292Z] testBinTokenReaderTesterSimpleTaggedTuple
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testBinTokenReaderTesterSimpleTaggedTuple | ok
[task 2018-04-18T09:31:02.292Z] testBinTokenReaderTesterTwoStringsInTuple
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testBinTokenReaderTesterTwoStringsInTuple | ok
[task 2018-04-18T09:31:02.292Z] testBinTokenReaderTesterEmptyUntaggedTuple
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testBinTokenReaderTesterEmptyUntaggedTuple | ok
[task 2018-04-18T09:31:02.292Z] testBinTokenReaderTesterStringWithEscapes
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testBinTokenReaderTesterStringWithEscapes | ok
[task 2018-04-18T09:31:02.292Z] testBinTokenReaderTesterSimpleString
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testBinTokenReaderTesterSimpleString | ok
[task 2018-04-18T09:31:02.292Z] testBinASTReaderMultipartECMAScript2
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testBinASTReaderMultipartECMAScript2 | ok
[task 2018-04-18T09:31:02.292Z] testBinASTReaderSimpleECMAScript2
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testBinASTReaderSimpleECMAScript2 | ok
[task 2018-04-18T09:31:02.292Z] testAtomicOperationsU8Clamped
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testAtomicOperationsU8Clamped | ok
[task 2018-04-18T09:31:02.292Z] testAtomicOperationsF64
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testAtomicOperationsF64 | ok
[task 2018-04-18T09:31:02.292Z] testAtomicOperationsF32
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testAtomicOperationsF32 | ok
[task 2018-04-18T09:31:02.292Z] testAtomicOperationsI64
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testAtomicOperationsI64 | ok
[task 2018-04-18T09:31:02.292Z] testAtomicOperationsU64
[task 2018-04-18T09:31:02.292Z] TEST-PASS | testAtomicOperationsU64 | ok
[task 2018-04-18T09:31:02.292Z] testAtomicOperationsI32
[task 2018-04-18T09:31:02.293Z] TEST-PASS | testAtomicOperationsI32 | ok
[task 2018-04-18T09:31:02.293Z] testAtomicOperationsU32
[task 2018-04-18T09:31:02.293Z] TEST-PASS | testAtomicOperationsU32 | ok
[task 2018-04-18T09:31:02.293Z] testAtomicOperationsI16
[task 2018-04-18T09:31:02.293Z] TEST-PASS | testAtomicOperationsI16 | ok
[task 2018-04-18T09:31:02.293Z] testAtomicOperationsU16
[task 2018-04-18T09:31:02.293Z] TEST-PASS | testAtomicOperationsU16 | ok
[task 2018-04-18T09:31:02.293Z] testAtomicOperationsI8
[task 2018-04-18T09:31:02.293Z] TEST-PASS | testAtomicOperationsI8 | ok
[task 2018-04-18T09:31:02.293Z] testAtomicOperationsU8
[task 2018-04-18T09:31:02.293Z] TEST-PASS | testAtomicOperationsU8 | ok
Flags: needinfo?(dteller)
Merge error, my bad.
Flags: needinfo?(dteller)
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da0dccbfc133
Tests for multipart tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/39efa7c54b52
Tests for BinAST multipart tokenizer (data);r=arai
https://hg.mozilla.org/integration/autoland/rev/f556625d7124
Introduce BinAST multipart tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/a0e12b5ffb98
Extending BinAST parser generator with support for multipart tokenizer;r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/aabb983736c1
Splitting the BinTokenReaderTester in two;r=arai
https://hg.mozilla.org/integration/autoland/rev/0355b10958a0
Fast lookup for BinAST string constants, shared among parsers;r=arai
https://hg.mozilla.org/integration/autoland/rev/6e7a79a8060e
Bunch of macros shared among BinAST files;r=arai
https://hg.mozilla.org/integration/autoland/rev/09611fba9d62
Extend JS shell binParse with ability to pick a tokenizer;r=arai
https://hg.mozilla.org/integration/autoland/rev/cf3964dd7ca4
Extend JS shell parse command to allow forcing full parsing;r=arai
https://hg.mozilla.org/integration/autoland/rev/714828115cc1
Make BinSource work with multipart tokenizer;r=arai,efaust
You need to log in before you can comment on or make changes to this bug.