1287220 - wasm: explicit drop et al

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Dan Gohman [:sunfish]]

Attachment: wasm-explicit-drop.patch

This patch adds support for explicit drop and makes implicit drops invalid, adds tee_local, makes set_local, set_global, and store return nothing. See:

https://github.com/WebAssembly/design/pull/694
https://github.com/WebAssembly/design/pull/711

for details.

There are still some outstanding spec questions about the type-checking of br, br_table, return, and unreachable; this patch currently has an unreachable flag, however that's just one possible approach.

AsmJS.cpp is also updated to emit drops as needed, and all the asm.js tests pass in Ion mode, and all except two in Baseline mode. More work remains in other areas.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: wasm-explicit-drop.patch

This patch introduces an unreachable flag, which disables type checking in unreachable code.

The remaining asmjs failures on Baseline are also now all fixed. The main work remaining:
 - disable more validation errors in unreachable code (assuming the language design goes in this direction)
 - update more wasm tests for explicit drops etc.
 - support WasmBinaryToText.cpp and WasmBinaryToExperimentalText.cpp

Comment 2

[Dan Gohman [:sunfish]]

Attachment: wasm-explicit-drop.patch

This version:
 - Disables validation errors in unreachable code.
 - Updates more tests to use explicit drop.

All wasm and asm.js tests now pass, except for the text format tests.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: wasm-explicit-drop.patch

This version adds basic support to WasmBinaryToAST and the text printers. Now all the in-tree tests pass, except the spec tests, which are not yet updated for explicit drop upstream.

Before this could land, we'd need to synchronize with upcoming upstream spec test updates, possibly implement other features being discussed for 0xc, and ideally synchronize with updated 0xc demo bits.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8777211 [details] [diff] [review]
wasm-explicit-drop.patch

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

Overall this looks great, nice work and thanks!

We've discussed the possibility of replacing the Baseline/Ion compiler's existing notion of "in dead code" with the ExprIter's, but it makes sense to save that for a follow-up optimization patch.

::: js/src/asmjs/WasmBinaryIterator.h
@@ +335,5 @@
>      MOZ_MUST_USE bool readAtomicBinOpOp(jit::AtomicOp* op) {
>          uint8_t x;
>          if (!readFixedU8(&x))
>              return false;
> +        if (unreachable_) {

I think this should be `if (!unreachable_)`.  From a quick scan, it seems like in most cases we test (!unreachable_).  Perhaps we could avoid the double-negative and invert to reachable_?

::: js/src/asmjs/WasmTypes.h
@@ -118,5 @@
> -// expression and can thus be used in any context.
> -const ExprType AnyType = ExprType::Limit;
> -
> -inline ExprType
> -Unify(ExprType a, ExprType b)

\o/

Comment 5

[Dan Gohman [:sunfish]]

Attachment: wasm-explicit-drop.patch

> I think this should be `if (!unreachable_)`.  From a quick scan, it seems like > in most cases we test (!unreachable_).  Perhaps we could avoid the
> double-negative and invert to reachable_?

Makes sense; done.

This version also has a few additional cleanups and is rebased on trunk.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: wasm-simplify-loop.patch

This patch implements the change to make loop has only one label, and several other fixes related to loops.

Comment 7

[Dan Gohman [:sunfish]]

Attachment: wasm-error-reporting.patch

This patch tidies up reporting of invalid modules.

Comment 8

[Dan Gohman [:sunfish]]

Attachment: wasm-function-end.patch

This patch adds support for function `end` opcodes.

Comment 9

[Dan Gohman [:sunfish]]

Attachment: wasm-call-return-aritites.patch

This patch removes call and return arities.

Comment 10

[Dan Gohman [:sunfish]]

Attachment: wasm-explicit-drop.patch

Comment 11

[Dan Gohman [:sunfish]]

Attachment: wasm-simplify-loop.patch

Comment 12

[Dan Gohman [:sunfish]]

Attachment: wasm-error-reporting.patch

Comment 13

[Dan Gohman [:sunfish]]

Attachment: wasm-function-end.patch

Comment 14

[Dan Gohman [:sunfish]]

Attachment: wasm-call-return-aritites.patch

Comment 15

[Dan Gohman [:sunfish]]

Attachment: wasm-sexpr-export-sugar.patch

Comment 16

[Dan Gohman [:sunfish]]

Attachment: wasm-sexpr-stack-syntax.patch

Comment 17

[Dan Gohman [:sunfish]]

Attachment: wasm-ast-first.patch

Comment 18

[Dan Gohman [:sunfish]]

Attachment: wasm-flat-format.patch

Comment 19

[Dan Gohman [:sunfish]]

Attachment: wasm-br-table-variable-length.patch

Comment 20

[Dan Gohman [:sunfish]]

Attachment: wasm-call-indirect-callee.patch

Comment 21

[Dan Gohman [:sunfish]]

Attachment: wasm-spectest.patch

Comment 22

[Dan Gohman [:sunfish]]

Attachment: wasm-tests.patch

Comment 23

[Dan Gohman [:sunfish]]

This series of patches now implements almost all of the 0xc changes, including:
 - Stack-machine support in binary decoding, Ion compilation, Baseline compilation, and the experimental text format.
 - The new "flat" text syntax (parsing and printing).
 - The latest spec testsuite from the "stack" branch of the spec repo.

Comment 24

[Dan Gohman [:sunfish]]

Attachment: wasm-explicit-drop.patch

This implements explicit-drop, no-push-void, and associated changes.

Comment 25

[Dan Gohman [:sunfish]]

Attachment: wasm-simplify-loop.patch

Comment 26

[Dan Gohman [:sunfish]]

Attachment: wasm-error-reporting.patch

Comment 27

[Dan Gohman [:sunfish]]

Attachment: wasm-function-end.patch

Comment 28

[Dan Gohman [:sunfish]]

Attachment: wasm-call-return-aritites.patch

Comment 29

[Dan Gohman [:sunfish]]

Attachment: wasm-sexpr-export-sugar.patch

Comment 30

[Dan Gohman [:sunfish]]

Attachment: wasm-sexpr-stack-syntax.patch

Comment 31

[Dan Gohman [:sunfish]]

Attachment: wasm-ast-first.patch

Comment 32

[Dan Gohman [:sunfish]]

Attachment: wasm-flat-format.patch

Comment 33

[Dan Gohman [:sunfish]]

Attachment: wasm-br-table-variable-length.patch

Comment 34

[Dan Gohman [:sunfish]]

Attachment: wasm-call-indirect-callee.patch

Comment 35

[Dan Gohman [:sunfish]]

Attachment: wasm-memory-syntax-sugar.patch

Comment 36

[Dan Gohman [:sunfish]]

Attachment: wasm-spectest.patch

Comment 37

[Dan Gohman [:sunfish]]

Attachment: wasm-tests.patch

Comment 38

[Dan Gohman [:sunfish]]

Attachment: wasm-new-format.patch

Comment 39

[Dan Gohman [:sunfish]]

This patch refresh:
 - implements the new memory syntax and syntax sugar
 - removes the "old format" code, implements the remaining corresponding "new format" features, and updates the tests to use "new format"
 - updates to the latest stack-branch spec tests
 - adds more tests
 - refactors how decoding errors are reported
 - refactors some more binary decoding routines to be shared between WasmCompile.cpp and WasmBinaryToAST.cpp
 - rebased on trunk

Not yet in this patch series:
 - block signatures
 - section opcodes

Comment 40

[Dan Gohman [:sunfish]]

Attachment: wasm-explicit-drop.patch

This patch implements the initial explicit drop rules, and eliminates pushing of void values. It adds `tee_local` and removes `store`'s return value.

Some of the validation changes in this patch are significantly revised in later patches.

Comment 41

[Dan Gohman [:sunfish]]

Attachment: wasm-simplify-loop.patch

This patch removes loop's bottom label, fixes some related validation issues. As with the previous patch, the validation code introduced here is significantly revised later.

Comment 42

[Dan Gohman [:sunfish]]

Attachment: wasm-function-end.patch

This adds an `end` to the ends of function bodies.

Comment 43

[Dan Gohman [:sunfish]]

Attachment: wasm-call-return-aritites.patch

This remove the arity immediates from `call` and `return` instructions.

Comment 44

[Dan Gohman [:sunfish]]

Attachment: wasm-sexpr-export-sugar.patch

This implements some of the new text syntax for defining exported functions.

Comment 45

[Dan Gohman [:sunfish]]

Attachment: wasm-sexpr-stack-syntax.patch

This adds parsing support for the new "flat" text format.

Note that this follows an earlier version of the rules, in which arity checking of s-expr-style instructions was still performed, so things like `(i32.add (i32.const 0))` are not yet accepted.

Comment 46

[Dan Gohman [:sunfish]]

Attachment: wasm-if-name.patch

This updates for a recent syntax change; instead of the then and else arms of an `if` each having their own name, now the `if` just has one name.

Comment 47

[Dan Gohman [:sunfish]]

Attachment: wasm-ast-first.patch

This adds the new "First" node to the wasm AST data structure and support for stack-machine constructs that need it.

Comment 48

[Dan Gohman [:sunfish]]

Attachment: wasm-flat-format.patch

This changes the primary text output to use the new flat format exclusively.

Comment 49

[Dan Gohman [:sunfish]]

Attachment: wasm-br-table-variable-length.patch

This makes `br_table`'s table use leb128 entries.

Comment 50

[Dan Gohman [:sunfish]]

Attachment: wasm-call-indirect-callee.patch

This moves `call_indirect`'s callee operand last.

Comment 51

[Dan Gohman [:sunfish]]

Attachment: wasm-memory-syntax-sugar.patch

This implements some of the new syntax for memory and data declarations.

Comment 52

[Dan Gohman [:sunfish]]

Attachment: wasm-new-format.patch

This removes the "old format" code and makes "new format" the default. It also contains a fair amount of refactoring to move binary decoding code out of WasmBinaryToAST.cpp/WasmCompile.cpp and into WasmBinary.cpp where it can be shared. It also unifies the error reporting mechanisms somewhat, so that Decoder::fail is used for all decoding errors, and errors are consistently reported with offsets.

Comment 53

[Dan Gohman [:sunfish]]

Attachment: wasm-block-signatures.patch

This adds block signatures and removes arities from branches, both encoding/decoding binary and printing/parsing text. It also significantly works how validation of blocks and branches work, to account for the fact that there is no longer a temporary "unknown" state.

It also adjusts validation to implement the remaining details of the new unreachable-code rule, where types are not checked in unreachable code, but immediates are.

The "experimental" text format is not yet updated to print block signatures.

Comment 54

[Dan Gohman [:sunfish]]

Attachment: wasm-section-opcodes.patch

This changes from section name strings to section opcodes.

Comment 55

[Dan Gohman [:sunfish]]

Attachment: wasm-tests.patch

This adds several tests for various features added in this patch series.

Comment 56

[Dan Gohman [:sunfish]]

Attachment: wasm-spectest.patch

This updates the wasm/spec tests to the latest block-sigs branch of the spec repo, and updates the known failures accordingly.

Comment 57

[Dan Gohman [:sunfish]]

Attachment: wasm-valtype.patch

This converts WasmBinaryIterator.h's value stack from ExprType to ValType, since void is no longer pushed on the stack.

Comment 58

[Dan Gohman [:sunfish]]

Attachment: wasm-num-memories-tables.patch

This adds counts to the table and memory sections, as has recently been proposed.

Comment 59

[Dan Gohman [:sunfish]]

Attachment: combined.patch

This patch is a combined patch with all the other patches here rolled up into one, for reference.

The following is a summary of the changes:

 - Instruction changes:
   - New instructions: `drop`, `tee_local`
   - Remove `store`'s return value
   - Remove arity immediates from `call` and `return`
   - Use variable-length encoding in `br_table`'s table
   - Move `call_indirect`'s callee operand last
   - Allow regular branches to target the function-body end
   - Add an explicit function `end`
   - Block signatures, remove branch arities
   - Remove `loop`'s bottom label
 - Stack machine changes:
   - Add a "first" operation to the WasmAST to represent stack-like code
   - Parsing and printing support for new "flat" syntax
 - Module encoding changes:
   - Section opcodes
   - Switch to "new format" by default and remove the "old format"
   - Add a count to the Table and Memory sections
 - Validation changes:
   - Require explicit drops in some contexts
   - Disable type checking, but not other validation errors, in unreachable code
   - Disallow values when branching to a `loop`
 - Misc text-format changes:
   - New syntax for memory and data declarations
   - New syntax for function exports

Comment 60

[Dan Gohman [:sunfish]]

Attachment: wasm-explicit-drop.patch

Attach correct patch for wasm-explicit-drop.patch.

Comment 61

[Luke Wagner [:luke]]

Comment on attachment 8790466 [details] [diff] [review]
wasm-explicit-drop.patch

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

Off to a good start!

::: js/src/asmjs/WasmBinaryIterator.h
@@ +1569,5 @@
> +    if (!readFixedF32(Output ? f32 : &unused))
> +        return false;
> +
> +    if (MOZ_LIKELY(reachable_)) {
> +        if (!push(ExprType::F32))

Could you maybe factor all these `if (MOZ_LIKELY(reachable_))`s into push() (and similarly for the popWithType)?  I know it might mean more branching when more than one push/pop is used, but I expect this wouldn't measurably hurt perf.

Comment 62

[Luke Wagner [:luke]]

Comment on attachment 8790309 [details] [diff] [review]
wasm-function-end.patch

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

Some nice cleanups

Comment 63

[Luke Wagner [:luke]]

Comment on attachment 8790310 [details] [diff] [review]
wasm-call-return-aritites.patch

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

ahh, much nicer

Comment 64

[Luke Wagner [:luke]]

Comment on attachment 8790378 [details] [diff] [review]
wasm-br-table-variable-length.patch

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

Good riddance!

Comment 65

[Luke Wagner [:luke]]

Comment on attachment 8790380 [details] [diff] [review]
wasm-call-indirect-callee.patch

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

I suppose even before asm.js is removed we could add `pick` and kill OldCallIndirect.  For now, could you factor out the commonality between CallIndirect and OldCallIndirect in WasmIonCompile and WasmBaselineCompile to avoid so much code duplication?

::: js/src/asmjs/WasmBinary.h
@@ +282,5 @@
>      // i64.eqz.
>      I64Eqz                               = 0xba,
>  
> +    // asm.js-style call_indirect with the callee evaluated first.
> +    OldCallIndirect                      = 0xbb,

Can you move this down to below the "The rest of these operators are currently only emitted internally when compiling asm.js" comment and put AsmJS in the name?

Comment 66

[Luke Wagner [:luke]]

Comment on attachment 8790382 [details] [diff] [review]
wasm-new-format.patch

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

::: js/src/asmjs/WasmBinary.cpp
@@ +18,5 @@
>  
>  #include "asmjs/WasmBinary.h"
>  
> +#include <stdarg.h>
> +#include "jsprf.h"

Can you put \n between and after these two?

::: js/src/asmjs/WasmBinary.h
@@ +968,5 @@
>  
> +MOZ_MUST_USE bool
> +DecodeGlobalType(Decoder& d, ValType* type, uint32_t* flags);
> +
> +struct Resizable

Coincidentally, I added ResizableLimits to WasmTypes.h in a recent patch; could you remove this and use ResizableLimits everywhere?

::: js/src/asmjs/WasmCompile.h
@@ +62,5 @@
>  //  - *error is null and the caller should report out-of-memory.
>  
>  SharedModule
> +Compile(const ShareableBytes& bytecode, const CompileArgs& args, UniqueChars* error,
> +        size_t* errorOffset);

Instead of doing this, which seems to lead to duplication in the clients of printing the offset, can the offset be put into the 'error' string by wasm::Compile and the error messages (in js.msg) changed to not take an offset?

::: js/src/asmjs/WasmJS.cpp
@@ +20,5 @@
>  
>  #include "mozilla/CheckedInt.h"
>  #include "mozilla/Maybe.h"
>  
> +#include "jsprf.h"

nit: can you add \n after this #include?

@@ +330,5 @@
>          return nullptr;
>  
>      global->as<GlobalObject>().setConstructor(JSProto_Wasm, ObjectValue(*Wasm));
> +
> +    // todo: hack

Can you change to "// Module instantiation assumes the WebAssembly constructor has been resolved because this API is temporary."?

Comment 67

[Luke Wagner [:luke]]

Comment on attachment 8790383 [details] [diff] [review]
wasm-block-signatures.patch

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

::: js/src/asmjs/WasmBinary.h
@@ +582,5 @@
> +    MOZ_MUST_USE bool writePatchableFixedU8(size_t* offset) {
> +        *offset = bytes_.length();
> +        return writeFixedU8(UINT8_MAX);
> +    }
> +    void patchFixedU8(size_t offset, uint8_t patchBits) {

Could you rename all these to U7 to indicate that we've not using that highest bit?  Also perhaps assert patchBits <= INT8_MAX.

Comment 68

[Luke Wagner [:luke]]

Comment on attachment 8790384 [details] [diff] [review]
wasm-section-opcodes.patch

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

::: js/src/asmjs/WasmBinary.h
@@ +39,5 @@
> +    Start,
> +    Elem,
> +    Code,
> +    Data,
> +    Name

Can you explicitly number these for easy xref with spec later on?

@@ +53,5 @@
>  static const char StartSectionId[]       = "start";
>  static const char CodeSectionId[]        = "code";
>  static const char ElemSectionId[]        = "elem";
>  static const char DataSectionId[]        = "data";
>  static const char NameSectionId[]        = "name";

Since these are only used for error messages, could you remove them and just inline the string literal into the two callers?

Comment 69

[Luke Wagner [:luke]]

Comment on attachment 8790389 [details] [diff] [review]
wasm-valtype.patch

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

yay!

Comment 70

[Luke Wagner [:luke]]

Comment on attachment 8790390 [details] [diff] [review]
wasm-num-memories-tables.patch

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

::: js/src/asmjs/WasmCompile.cpp
@@ +749,5 @@
> +    if (!d.readVarU32(&numTables))
> +        return d.fail("failed to read number of tables");
> +
> +    if (numTables > 1)
> +        return d.fail("the number of tables must be exactly one");

I think you need to handle the numTables/numMemories==0 case :)

Comment 71

[Luke Wagner [:luke]]

Comment on attachment 8790382 [details] [diff] [review]
wasm-new-format.patch

Oops, missed this r+.

Overall great work!  Everything looked really clean.

Comment 72

[Dan Gohman [:sunfish]]

Attachment: combined.patch

Attached is a new combined patch, which includes changes to address the review comments here and updates to use the new ResizableLimits struct, and is rebased on top of tree.

Comment 73

[Dan Gohman [:sunfish]]

Comment on attachment 8791349 [details] [diff] [review]
combined.patch

Requesting fuzzing for a major patch to SpiderMonkey's wasm decoder.

Comment 74

[Luke Wagner [:luke]]

Gary/Christian, just FYI: this is basically the last patch of substance to land before FF51 branches next Monday so a thorough round of fuzzing would be much appreciated to avoid branch fixes later (since the goal is to enable wasm by default before 51 is released).

Comment 75

[Luke Wagner [:luke]]

Comment on attachment 8791349 [details] [diff] [review]
combined.patch

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

::: js/src/asmjs/WasmJS.cpp
@@ +1328,5 @@
>  
> +    // Ideally we'd report a JSMSG_WASM_DECODE_FAIL here, but there's no easy
> +    // way to create an ErrorObject for an arbitrary error code with multiple
> +    // replacements.
> +    UniqueChars str(JS_smprintf("wasm validation error: %s", error.get()));

(nit: need null-check)

Comment 76

[Dan Gohman [:sunfish]]

Attachment: combined.patch

Fixed missing null check.

Comment 77

[Dan Gohman [:sunfish]]

I believe the patch applies cleanly at revision http://hg.mozilla.org/integration/mozilla-inbound/rev/6e355b663626.

Comment 78

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: isObject(), at dist/include/js/Value.h:1274
Build version: mozilla-inbound revision 6e355b663626+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu --enable-simulator=arm
Runtime options: 

Testcase:

See attachment.

Backtrace:

==10728==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x08396def bp 0xff86c2a8 sp 0xff86c140 T0)
    #0 0x8396dee in js::NativeObject::getSlot(unsigned int) const js/src/vm/NativeObject.h:785:9
    #1 0x8396dee in js::GlobalObject::getPrototype(JSProtoKey) const js/src/vm/GlobalObject.h:175
    #2 0x8396dee in js::wasm::Module::instantiateMemory(JSContext*, JS::MutableHandle<js::WasmMemoryObject*>) const js/src/asmjs/WasmModule.cpp:605
    #3 0x8399ae7 in js::wasm::Module::instantiate(JSContext*, JS::Handle<JS::GCVector<JSFunction*, 0u, js::TempAllocPolicy> >, JS::Handle<js::WasmTableObject*>, JS::Handle<js::WasmMemoryObject*>, mozilla::Vector<js::wasm::Val, 0u, js::SystemAllocPolicy> const&, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) const js/src/asmjs/WasmModule.cpp:798:10
    #4 0x830b4d9 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:250:12
    #5 0x82323cf in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14
[...]

Comment 79

[Christian Holler (:decoder)]

Attachment: Testcase for comment 79

Comment 80

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: !empty(), at dist/include/mozilla/Vector.h:473
Build version: mozilla-inbound revision 6e355b663626+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu
Runtime options: 

Testcase:

See attachment.

Backtrace:

==1270==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x0b840d29 bp 0xff977148 sp 0xff977120 T0)
    #0 0xb840d28 in mozilla::Vector<js::wasm::ControlStackEntry<mozilla::Nothing>, 0u, js::SystemAllocPolicy>::back() /srv/jenkins/jobs/mozilla-inbound-build-wasm/workspace/arch/32/type/debug/dist/include/mozilla/Vector.h:472:5
    #1 0xb81e5a9 in js::wasm::ExprIter<(anonymous namespace)::ValidatingPolicy>::enterUnreachableCode() js/src/asmjs/WasmBinaryIterator.h:495:30
    #2 0xb81e5a9 in js::wasm::ExprIter<(anonymous namespace)::ValidatingPolicy>::readUnreachable() js/src/asmjs/WasmBinaryIterator.h:1163
    #3 0xb81e5a9 in DecodeExpr((anonymous namespace)::FunctionDecoder&) js/src/asmjs/WasmCompile.cpp:471
    #4 0xb80f5e9 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:14
    #5 0xb80f5e9 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1149
    #6 0xb80f5e9 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1399
    #7 0x82fd38d in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:232:27
    #8 0x822e60f in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14
[...]

Comment 81

[Christian Holler (:decoder)]

Attachment: Testcase for comment 80

Comment 82

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: !controlStack_.empty(), at js/src/asmjs/WasmBinaryIterator.h:735
Build version: mozilla-inbound revision 6e355b663626+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu
Runtime options: 

Testcase:

See attachment.

Backtrace:

==1242==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x0b830622 bp 0xffa452a8 sp 0xffa45270 T0)
    #0 0xb830621 in MOZ_ReportAssertionFailure(char const*, char const*, int) /srv/jenkins/jobs/mozilla-inbound-build-wasm/workspace/arch/32/type/debug/dist/include/mozilla/Assertions.h:165:10
    #1 0xb830621 in js::wasm::ExprIter<(anonymous namespace)::ValidatingPolicy>::mergeControl(js::wasm::LabelKind*, js::wasm::ExprType*, mozilla::Nothing*) js/src/asmjs/WasmBinaryIterator.h:745
    #2 0xb81f1c2 in js::wasm::ExprIter<(anonymous namespace)::ValidatingPolicy>::readElse(js::wasm::ExprType*, mozilla::Nothing*) js/src/asmjs/WasmBinaryIterator.h:940:10
    #3 0xb81f1c2 in DecodeExpr((anonymous namespace)::FunctionDecoder&) js/src/asmjs/WasmCompile.cpp:246
    #4 0xb80f5e9 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:14
    #5 0xb80f5e9 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1149
    #6 0xb80f5e9 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1399
    #7 0x82fd38d in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:232:27
    #8 0x822e60f in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14
[...]

Comment 83

[Christian Holler (:decoder)]

Attachment: Testcase for comment 82

Comment 84

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: isObject(), at dist/include/js/Value.h:1274
Build version: mozilla-inbound revision 6e355b663626+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu
Runtime options: 

Testcase:

See attachment.

Backtrace:

==3746==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x0831e2f2 bp 0xfff77bc8 sp 0xfff779e0 T0)
    #0 0x831e2f1 in js::NativeObject::getSlot(unsigned int) const js/src/vm/NativeObject.h:785:9
    #1 0x831e2f1 in js::GlobalObject::getPrototype(JSProtoKey) const js/src/vm/GlobalObject.h:175
    #2 0x831e2f1 in js::WasmTableObject::create(JSContext*, js::wasm::ResizableLimits) js/src/asmjs/WasmJS.cpp:1059
    #3 0x838a62f in js::wasm::Module::instantiateTable(JSContext*, JS::MutableHandle<js::WasmTableObject*>, mozilla::Vector<RefPtr<js::wasm::Table>, 0u, js::SystemAllocPolicy>*) const js/src/asmjs/WasmModule.cpp:644:30
    #4 0x838c28c in js::wasm::Module::instantiate(JSContext*, JS::Handle<JS::GCVector<JSFunction*, 0u, js::TempAllocPolicy> >, JS::Handle<js::WasmTableObject*>, JS::Handle<js::WasmMemoryObject*>, mozilla::Vector<js::wasm::Val, 0u, js::SystemAllocPolicy> const&, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) const js/src/asmjs/WasmModule.cpp:803:10
    #5 0x82fdb79 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:250:12
    #6 0x822e60f in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14
[...]

This one has a different stack compared to the other isObject assert already posted.

Comment 85

[Christian Holler (:decoder)]

Attachment: Testcase for comment 84

Comment 86

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: !hasPushed(curBlock_), at js/src/asmjs/WasmIonCompile.cpp:1162
Build version: mozilla-inbound revision 6e355b663626+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug
Runtime options: 

Testcase:

See attachment.

Backtrace:

==14205==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000002bcd3e0 bp 0x7ffeaff60c70 sp 0x7ffeaff5f900 T0)
    #0 0x2bcd3df in (anonymous namespace)::FunctionCompiler::pushDef(js::jit::MDefinition*) js/src/asmjs/WasmIonCompile.cpp:1162:9
    #1 0x2bcd3df in (anonymous namespace)::FunctionCompiler::brTable(js::jit::MDefinition*, unsigned int, mozilla::Vector<unsigned int, 0ul, js::SystemAllocPolicy> const&, js::jit::MDefinition*) js/src/asmjs/WasmIonCompile.cpp:1538
    #2 0x2bcd3df in EmitBrTable((anonymous namespace)::FunctionCompiler&) js/src/asmjs/WasmIonCompile.cpp:1825
    #3 0x2bcd3df in EmitExpr((anonymous namespace)::FunctionCompiler&) js/src/asmjs/WasmIonCompile.cpp:3134
    #4 0x2b91a29 in js::wasm::IonCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3749:18
    #5 0x2bce0a3 in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3794:16
    #6 0x2b45370 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:946:14
    #7 0x2aed29e in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1089:12
    #8 0x2aed29e in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1149
    #9 0x2aed29e in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1399
    #10 0x657522 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:232:27
    #11 0x5a9fb0 in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14

Comment 87

[Christian Holler (:decoder)]

Attachment: Testcase for previous comment

Comment 88

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: !empty(), at /srv/jenkins/jobs/mozilla-inbound-build-wasm/workspace/arch/32/type/debug/dist/include/mozilla/Vector.h:473
Build version: mozilla-inbound revision 6e355b663626+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu
Runtime options: 

Testcase:

See attachment.

Backtrace:

==14876==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x0b840d29 bp 0xffdf7658 sp 0xffdf7630 T0)
    #0 0xb840d28 in mozilla::Vector<js::wasm::ControlStackEntry<mozilla::Nothing>, 0u, js::SystemAllocPolicy>::back() /srv/jenkins/jobs/mozilla-inbound-build-wasm/workspace/arch/32/type/debug/dist/include/mozilla/Vector.h:472:5
    #1 0xb82f0b7 in js::wasm::ExprIter<(anonymous namespace)::ValidatingPolicy>::checkTop() js/src/asmjs/WasmBinaryIterator.h:410:49
    #2 0xb821b89 in js::wasm::ExprIter<(anonymous namespace)::ValidatingPolicy>::popWithType(js::wasm::ValType, mozilla::Nothing*) js/src/asmjs/WasmBinaryIterator.h:432:14
    #3 0xb821b89 in js::wasm::ExprIter<(anonymous namespace)::ValidatingPolicy>::readIf(mozilla::Nothing*) js/src/asmjs/WasmBinaryIterator.h:922
    #4 0xb821b89 in DecodeIf((anonymous namespace)::FunctionDecoder&) js/src/asmjs/WasmCompile.cpp:176
    #5 0xb821b89 in DecodeExpr((anonymous namespace)::FunctionDecoder&) js/src/asmjs/WasmCompile.cpp:244
    #6 0xb80f5e9 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:14
    #7 0xb80f5e9 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1149
    #8 0xb80f5e9 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1399
    #9 0x82fd38d in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:232:27
    #10 0x822e60f in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14

Again different stack compared to the other, identical assert previously posted.

Comment 89

[Christian Holler (:decoder)]

Attachment: Testcase for previous comment

Comment 90

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: masm.framePushed() == framePushed, at js/src/asmjs/WasmFrameIterator.cpp:419
Build version: mozilla-inbound revision 6e355b663626+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug
Runtime options: 

Testcase:

See attachment.

Backtrace:

==12497==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000002b24c7a bp 0x7ffe931135f0 sp 0x7ffe93113520 T0)
    #0 0x2b24c79 in js::jit::X86Encoding::JmpDst::JmpDst(int) js/src/jit/x86-shared/Patching-x86-shared.h:104:9
    #1 0x2b24c79 in js::jit::X86Encoding::BaseAssembler::label() js/src/jit/x86-shared/BaseAssembler-x86-shared.h:3689
    #2 0x2b24c79 in js::jit::AssemblerX86Shared::currentOffset() js/src/jit/x86-shared/Assembler-x86-shared.h:985
    #3 0x2b24c79 in js::wasm::GenerateFunctionEpilogue(js::jit::MacroAssembler&, unsigned int, js::wasm::FuncOffsets*) js/src/asmjs/WasmFrameIterator.cpp:438
    #4 0x2da0ec1 in js::wasm::BaseCompiler::endFunction() js/src/asmjs/WasmBaselineCompile.cpp:1906:9
    #5 0x2d82447 in js::wasm::BaseCompiler::emitFunction() js/src/asmjs/WasmBaselineCompile.cpp:6702:10
    #6 0x2d85863 in js::wasm::BaselineCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmBaselineCompile.cpp:6936:10
    #7 0x2bce0da in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3796:16
    #8 0x2b45370 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:946:14
    #9 0x2aed29e in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1089:12
    #10 0x2aed29e in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1149
    #11 0x2aed29e in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1399
    #12 0x657522 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:232:27
    #13 0x5a9fb0 in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14


Not sure if this is caused by your patch but I haven't seen it in the follow-up fuzzing without your patch.

Comment 91

[Christian Holler (:decoder)]

Attachment: Testcase for previous comment

Comment 92

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: aIndex < mLength, at dist/include/mozilla/Vector.h:459
Build version: mozilla-inbound revision 6e355b663626+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug
Runtime options: 

Testcase:

See attachment.

Backtrace:

==15382==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000002b1956d bp 0x7ffec92faeb0 sp 0x7ffec92faea0 T0)
    #0 0x2b1956c in mozilla::Vector<js::wasm::ControlStackEntry<mozilla::Nothing>, 0ul, js::SystemAllocPolicy>::operator[](unsigned long) /srv/jenkins/jobs/mozilla-inbound-build-wasm/workspace/arch/64/type/debug/dist/include/mozilla/Vector.h:458:5
    #1 0x2afa1b4 in js::wasm::ExprIter<(anonymous namespace)::ValidatingPolicy>::readReturn(mozilla::Nothing*) js/src/asmjs/WasmBinaryIterator.h:870:55
    #2 0x2afa1b4 in DecodeExpr((anonymous namespace)::FunctionDecoder&) js/src/asmjs/WasmCompile.cpp:469
    #3 0x2aed0b9 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:14
    #4 0x2aed0b9 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1149
    #5 0x2aed0b9 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1399
    #6 0x657522 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:232:27
    #7 0x5a9fb0 in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14

Comment 93

[Christian Holler (:decoder)]

Attachment: Testcase for previous comment

Comment 94

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: controlItem.kind() == LabelKind::Block, at js/src/asmjs/WasmBinaryIterator.h:871
Build version: mozilla-inbound revision 6e355b663626+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug
Runtime options: 

Testcase:

See attachment.

Backtrace:

==32467==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000002b04ced bp 0x7ffecc8e49b0 sp 0x7ffecc8e47a0 T0)
    #0 0x2b04cec in DecodeExpr((anonymous namespace)::FunctionDecoder&) js/src/asmjs/WasmCompile.cpp:218:16
    #1 0x2aed0b9 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:14
    #2 0x2aed0b9 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1149
    #3 0x2aed0b9 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1399
    #4 0x657522 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:232:27
    #5 0x5a9fb0 in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14

Comment 95

[Christian Holler (:decoder)]

Attachment: Testcase for previous comment

Comment 96

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: !empty(), at /srv/jenkins/jobs/mozilla-inbound-build-wasm/workspace/arch/64/type/debug/dist/include/mozilla/Vector.h:473
Build version: mozilla-inbound revision 6e355b663626+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug
Runtime options: 

Testcase:

See attachment.

Backtrace:

==4211==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000002b149e2 bp 0x7ffe0ca20cc0 sp 0x7ffe0ca20cb0 T0)
    #0 0x2b149e1 in mozilla::Vector<js::wasm::ControlStackEntry<mozilla::Nothing>, 0ul, js::SystemAllocPolicy>::back() /srv/jenkins/jobs/mozilla-inbound-build-wasm/workspace/arch/64/type/debug/dist/include/mozilla/Vector.h:472:5
    #1 0x2b05e42 in js::wasm::ExprIter<(anonymous namespace)::ValidatingPolicy>::peek(unsigned int, js::wasm::TypeAndValue<mozilla::Nothing>*) js/src/asmjs/WasmBinaryIterator.h:480:48
    #2 0x2b05e42 in js::wasm::ExprIter<(anonymous namespace)::ValidatingPolicy>::readCallArg(js::wasm::ValType, unsigned int, unsigned int, mozilla::Nothing*) js/src/asmjs/WasmBinaryIterator.h:1743
    #3 0x2b05e42 in DecodeCallArgs((anonymous namespace)::FunctionDecoder&, js::wasm::Sig const&) js/src/asmjs/WasmCompile.cpp:94
    #4 0x2af8624 in DecodeCall((anonymous namespace)::FunctionDecoder&) js/src/asmjs/WasmCompile.cpp:122:12
    #5 0x2af8624 in DecodeExpr((anonymous namespace)::FunctionDecoder&) js/src/asmjs/WasmCompile.cpp:216
    #6 0x2aed0b9 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:14
    #7 0x2aed0b9 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1149
    #8 0x2aed0b9 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1399
    #9 0x657522 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:232:27
    #10 0x5a9fb0 in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14

This might be a dup to one of the previous asserts of the same kind. But the stack here looked different than the others so I'm including it here.

Comment 97

[Christian Holler (:decoder)]

Attachment: Testcase for previous comment

Comment 98

[Luke Wagner [:luke]]

Attachment: ensure-webassembly-in-eval

(Fixes comment 78)  AFL-only bug: AFL fuzzing calls wasm::Eval directly w/o resolving 'WebAssembly' so we have to do that explicitly.

Comment 99

[Luke Wagner [:luke]]

Attachment: fix-early-end

(Fixes comment 79)  The bug is that we kept decoding after seeing the outer functions' end.  This patch restructures the loop in a way which both makes it a branch-or-two faster and uses empty-stack as the only non-error loop termination condition.

While trying to write a testcase, I noticed a few ops were missing from WasmToken:isOpcode(), hence the unrelated changes in WasmTextToBinary.cpp.

Comment 100

[Luke Wagner [:luke]]

Comment on attachment 8792345 [details]
Testcase for comment 82

Fixed by the previous patch.

Comment 101

[Luke Wagner [:luke]]

Comment on attachment 8792347 [details]
Testcase for comment 84

Fixed by previous patch.

Comment 102

[Luke Wagner [:luke]]

Comment on attachment 8792348 [details]
Testcase for previous comment

Fixed by previous patch.

Comment 103

[Luke Wagner [:luke]]

Comment on attachment 8792349 [details]
Testcase for previous comment

Fixed by previous patch.

Comment 104

[Luke Wagner [:luke]]

Comment on attachment 8792350 [details]
Testcase for previous comment

Fixed by previous patch.

Comment 105

[Luke Wagner [:luke]]

Comment on attachment 8792351 [details]
Testcase for previous comment

Fixed by previous patch.

Comment 106

[Luke Wagner [:luke]]

Comment on attachment 8792352 [details]
Testcase for previous comment

Fixed by previous patch.

Comment 107

[Luke Wagner [:luke]]

Comment on attachment 8792353 [details]
Testcase for previous comment

Cool, looks like these were all one of those two bugs, primarily fix-early-end!

Comment 108

[Luke Wagner [:luke]]

Attachment: combined-v2.patch

Thanks for the first round!  This patch fixes all know issues and is rebased to mozilla-inbound tip (db14395dc391).  Could we get another round?  It's possible that one bug was obscuring others.

Comment 109

[Dan Gohman [:sunfish]]

Comment on attachment 8793482 [details] [diff] [review]
fix-early-end

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

Nice!

Comment 110

[Luke Wagner [:luke]]

Attachment: update-names-section

This patch updates the names sections and tests to match 0xc.  (Note: next patch will add in skipping of user-defined sections.)

Comment 111

[Luke Wagner [:luke]]

Attachment: update-names-section

Updated to make the next patch cleaner.

Comment 112

[Luke Wagner [:luke]]

Attachment: skip-unknown

This patch skips user-defined sections.

Comment 113

[Luke Wagner [:luke]]

Attachment: combined-v3.patch

In case you haven't started, here is an updated combined patch that includes the last two patches.

Comment 114

[Dan Gohman [:sunfish]]

Comment on attachment 8793777 [details] [diff] [review]
update-names-section

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

::: js/src/asmjs/WasmCompile.cpp
@@ +1270,4 @@
>  
>      NameInBytecodeVector funcNames;
>      if (!funcNames.resize(numFuncNames))
> +        return;

This is an OOM check, rather than an error in the input. It's surprising to me, at least, to see it being silently ignored. If that's really the intent, please note it in a comment.

@@ +1310,5 @@
>      if (sectionStart == Decoder::NotStarted)
>          return true;
>  
> +    // Per interface contract, unconditionally call finishUserDefinedSection()
> +    // after the section is started.

Should this comment be one line below?

Comment 115

[Dan Gohman [:sunfish]]

Comment on attachment 8793779 [details] [diff] [review]
skip-unknown

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

::: js/src/asmjs/WasmBinary.h
@@ +867,5 @@
>              goto backup;
> +        while (idValue != uint32_t(id)) {
> +            if (idValue != uint32_t(SectionId::UserDefined))
> +                goto backup;
> +            cur_ = beforeId;

This little dance with beforeId reads a little subtle to me. How about a comment like "skipUserDefinedSection expects to read the id itself, so rewind cur_ back to before the id".

Comment 116

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: producer_ != nullptr, at js/src/jit/MIR.h:273
Build version: mozilla-inbound revision db14395dc391+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu
Runtime options: 

Testcase:

See attachment.

Backtrace:

==19739==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x08d414fb bp 0xfff7d658 sp 0xfff7d630 T0)
    #0 0x8d414fa in js::jit::MDefinition::addUseUnchecked(js::jit::MUse*) js/src/jit/MIR.h:840:9
    #1 0x8d40f39 in js::jit::MUse::initUnchecked(js::jit::MDefinition*, js::jit::MNode*) js/src/jit/MIR.h:13870:5
    #2 0x8d40f39 in js::jit::MUse::init(js::jit::MDefinition*, js::jit::MNode*) js/src/jit/MIR.h:13862
    #3 0xb96652a in js::jit::MAryControlInstruction<1u, 2u>::initOperand(unsigned int, js::jit::MDefinition*) js/src/jit/MIR.h:2899:9
    #4 0xb96652a in js::jit::MTest::MTest(js::jit::MDefinition*, js::jit::MBasicBlock*, js::jit::MBasicBlock*) js/src/jit/MIR.h:2977
    #5 0xb96652a in js::jit::MTest* js::jit::MTest::New<js::jit::MDefinition*&, js::jit::MBasicBlock*&, js::jit::MBasicBlock*&>(js::jit::TempAllocator&, js::jit::MDefinition*&, js::jit::MBasicBlock*&, js::jit::MBasicBlock*&) js/src/jit/MIR.h:2984
    #6 0xb96652a in (anonymous namespace)::FunctionCompiler::branchAndStartThen(js::jit::MDefinition*, js::jit::MBasicBlock**) js/src/asmjs/WasmIonCompile.cpp:1229
    #7 0xb96652a in EmitIf((anonymous namespace)::FunctionCompiler&) js/src/asmjs/WasmIonCompile.cpp:1682
    #8 0xb96652a in EmitExpr((anonymous namespace)::FunctionCompiler&) js/src/asmjs/WasmIonCompile.cpp:3124
    #9 0xb958809 in js::wasm::IonCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3749:18
    #10 0xb9aae38 in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3794:16
    #11 0xb8f6c78 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:946:14
    #12 0xb885d19 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:12
    #13 0xb885d19 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1134
    #14 0xb885d19 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1384
    #15 0x830b045 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:235:27
    #16 0x823ab6f in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14
[...]

Comment 117

[Christian Holler (:decoder)]

Attachment: Testcase for comment 116

Comment 118

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Assertion failure: !hasPushed(curBlock_), at js/src/asmjs/WasmIonCompile.cpp:1162
Build version: mozilla-inbound revision db14395dc391+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu
Runtime options: 

Testcase:

See attachment.

Backtrace:

==26105==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x0b9d30ff bp 0xffb71188 sp 0xffb71140 T0)
    #0 0xb9d30fe in (anonymous namespace)::FunctionCompiler::hasPushed(js::jit::MBasicBlock*) js/src/asmjs/WasmIonCompile.cpp:1147:9
    #1 0xb9d30fe in (anonymous namespace)::FunctionCompiler::pushDef(js::jit::MDefinition*) js/src/asmjs/WasmIonCompile.cpp:1162
    #2 0xb9d30fe in (anonymous namespace)::FunctionCompiler::brIf(unsigned int, js::jit::MDefinition*, js::jit::MDefinition*) js/src/asmjs/WasmIonCompile.cpp:1486
    #3 0xb97d9e4 in EmitBrIf((anonymous namespace)::FunctionCompiler&) js/src/asmjs/WasmIonCompile.cpp:1785:14
    #4 0xb97d9e4 in EmitExpr((anonymous namespace)::FunctionCompiler&) js/src/asmjs/WasmIonCompile.cpp:3132
    #5 0xb958809 in js::wasm::IonCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3749:18
    #6 0xb9aae38 in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3794:16
    #7 0xb8f6c78 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:946:14
    #8 0xb885d19 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:12
    #9 0xb885d19 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1134
    #10 0xb885d19 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1384
    #11 0x830b045 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:235:27
    #12 0x823ab6f in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14
[...]

Comment 119

[Christian Holler (:decoder)]

Attachment: Testcase for comment 118

Comment 120

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Crash [@ js::jit::MDefinition::type]
Build version: mozilla-inbound revision db14395dc391+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu
Runtime options: 

Testcase:

See attachment.

Backtrace:

==30419==ERROR: AddressSanitizer: SEGV on unknown address 0x0000001c (pc 0x0b968651 bp 0xffd57d98 sp 0xffd566c0 T0)
    #0 0xb968650 in js::jit::MDefinition::type() const js/src/jit/MIR.h:743:16
    #1 0xb968650 in js::jit::MDefinition* (anonymous namespace)::FunctionCompiler::unary<js::jit::MToFloat32>(js::jit::MDefinition*) js/src/asmjs/WasmIonCompile.cpp:1644
    #2 0xb968650 in bool EmitConversion<js::jit::MToFloat32>((anonymous namespace)::FunctionCompiler&, js::wasm::ValType, js::wasm::ValType) js/src/asmjs/WasmIonCompile.cpp:2131
    #3 0xb968650 in EmitExpr((anonymous namespace)::FunctionCompiler&) js/src/asmjs/WasmIonCompile.cpp:3382
    #4 0xb958809 in js::wasm::IonCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3749:18
    #5 0xb9aae38 in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3794:16
    #6 0xb8f6c78 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:946:14
    #7 0xb885d19 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:12
    #8 0xb885d19 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1134
    #9 0xb885d19 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1384
    #10 0x830b045 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:235:27
    #11 0x823ab6f in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14
[...]

Comment 121

[Christian Holler (:decoder)]

Attachment: Testcase for comment 120

Comment 122

[Christian Holler (:decoder)]

This is an automated crash issue comment:

Summary: Crash [@ js::jit::MClz::NewAsmJS]
Build version: mozilla-inbound revision db14395dc391+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu
Runtime options: 

Testcase:

See attachment.

Backtrace:

==17648==ERROR: AddressSanitizer: SEGV on unknown address 0x0000001c (pc 0x0b965cf8 bp 0xffb3a458 sp 0xffb38d7c T0)
    #0 0xb965cf7 in js::jit::MClz::NewAsmJS(js::jit::TempAllocator&, js::jit::MDefinition*) js/src/jit/MIR.h:6226:27
    #1 0xb965cf7 in js::jit::MDefinition* (anonymous namespace)::FunctionCompiler::unary<js::jit::MClz>(js::jit::MDefinition*) js/src/asmjs/WasmIonCompile.cpp:357
    #2 0xb965cf7 in bool EmitUnary<js::jit::MClz>((anonymous namespace)::FunctionCompiler&, js::wasm::ValType) js/src/asmjs/WasmIonCompile.cpp:2119
    #3 0xb965cf7 in EmitExpr((anonymous namespace)::FunctionCompiler&) js/src/asmjs/WasmIonCompile.cpp:3208
    #4 0xb958809 in js::wasm::IonCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3749:18
    #5 0xb9aae38 in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3794:16
    #6 0xb8f6c78 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:946:14
    #7 0xb885d19 in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1074:12
    #8 0xb885d19 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1134
    #9 0xb885d19 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1384
    #10 0x830b045 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:235:27
    #11 0x823ab6f in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5392:14
[...]

Comment 123

[Christian Holler (:decoder)]

Attachment: Testcase for comment 122

Comment 124

[Luke Wagner [:luke]]

Attachment: fix-close-loop

Wow, another great find and one trivial root bug causing all four failures.

Comment 125

[Luke Wagner [:luke]]

Comment on attachment 8793782 [details] [diff] [review]
combined-v3.patch

Unlike the first bug, I doubt this bug is "hiding" others that a third round would find.  Thanks a lot for the help guys!

Comment 126

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8794298 [details] [diff] [review]
fix-close-loop

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

Stealing per request, looks good!

Comment 127

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/958074f3b830
Baldr: update to binary version 0xc (r=luke)

Comment 128

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Jit tests were frequently failing like https://treeherder.mozilla.org/logviewer.html#?job_id=36396696&repo=mozilla-inbound when this push landed.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3354a7c329fe

Comment 129

[Luke Wagner [:luke]]

Dan spotted the bug: WasmTextToBinary was using number-of-chars as number-of-pages and therefore allocating almost 4gb, hence the frequent OOMs on 32-bit.  I'm surprised they were more frequent...

Comment 130

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4119fba22f7f
Baldr: update to binary version 0xc (r=luke)

Comment 131

[Luke Wagner [:luke]]

(I should've said: I'm surprised they're *not* more frequent; didn't see this in several try pushes earlier.)

Comment 132

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/4119fba22f7f