Dan Gohman [:sunfish] Assignee
	Description • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 1 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 2 • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 3 • 8 years ago

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.

	Luke Wagner [:luke]
	Comment 4 • 8 years ago

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/

	Dan Gohman [:sunfish] Assignee
	Comment 5 • 8 years ago

> 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.

	Dan Gohman [:sunfish] Assignee
	Comment 6 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 7 • 8 years ago

This patch tidies up reporting of invalid modules.

	Dan Gohman [:sunfish] Assignee
	Comment 8 • 8 years ago

This patch adds support for function `end` opcodes.

	Dan Gohman [:sunfish] Assignee
	Comment 9 • 8 years ago

This patch removes call and return arities.

	Dan Gohman [:sunfish] Assignee
	Comment 23 • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 24 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 39 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 40 • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 41 • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 42 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 43 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 44 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 45 • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 46 • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 47 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 48 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 49 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 50 • 8 years ago

This moves `call_indirect`'s callee operand last.

	Dan Gohman [:sunfish] Assignee
	Comment 51 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 52 • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 53 • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 54 • 8 years ago

This changes from section name strings to section opcodes.

	Dan Gohman [:sunfish] Assignee
	Comment 55 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 56 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 57 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 58 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 59 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 60 • 8 years ago

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

	Luke Wagner [:luke]
	Comment 61 • 8 years ago

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.

	Luke Wagner [:luke]
	Comment 62 • 8 years ago

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

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

Some nice cleanups

	Luke Wagner [:luke]
	Comment 63 • 8 years ago

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

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

ahh, much nicer

	Luke Wagner [:luke]
	Comment 64 • 8 years ago

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

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

Good riddance!

	Luke Wagner [:luke]
	Comment 65 • 8 years ago

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?

	Luke Wagner [:luke]
	Comment 66 • 8 years ago

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."?

	Luke Wagner [:luke]
	Comment 67 • 8 years ago

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.

	Luke Wagner [:luke]
	Comment 68 • 8 years ago

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?

	Luke Wagner [:luke]
	Comment 69 • 8 years ago

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

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

yay!

	Luke Wagner [:luke]
	Comment 70 • 8 years ago

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 :)

	Luke Wagner [:luke]
	Comment 71 • 8 years ago

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

Oops, missed this r+.

Overall great work!  Everything looked really clean.

	Dan Gohman [:sunfish] Assignee
	Comment 72 • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 73 • 8 years ago

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

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

	Luke Wagner [:luke]
	Comment 74 • 8 years ago

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).

	Luke Wagner [:luke]
	Comment 75 • 8 years ago

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)

	Dan Gohman [:sunfish] Assignee
	Comment 76 • 8 years ago

Fixed missing null check.

	Dan Gohman [:sunfish] Assignee
	Comment 77 • 8 years ago

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

	Christian Holler (:decoder)
	Comment 78 • 8 years ago

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
[...]

	Christian Holler (:decoder)
	Comment 80 • 8 years ago

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
[...]

	Christian Holler (:decoder)
	Comment 82 • 8 years ago

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
[...]

	Christian Holler (:decoder)
	Comment 84 • 8 years ago

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.

	Christian Holler (:decoder)
	Comment 86 • 8 years ago

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

	Christian Holler (:decoder)
	Comment 88 • 8 years ago

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.

	Christian Holler (:decoder)
	Comment 90 • 8 years ago

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.

	Christian Holler (:decoder)
	Comment 92 • 8 years ago

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

	Christian Holler (:decoder)
	Comment 94 • 8 years ago

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

	Christian Holler (:decoder)
	Comment 96 • 8 years ago

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.

	Luke Wagner [:luke]
	Comment 98 • 8 years ago

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

	Luke Wagner [:luke]
	Comment 99 • 8 years ago

(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.

	Luke Wagner [:luke]
	Comment 100 • 8 years ago

Comment on attachment 8792345 [details]
Testcase for comment 82

Fixed by the previous patch.

	Luke Wagner [:luke]
	Comment 101 • 8 years ago

Comment on attachment 8792347 [details]
Testcase for comment 84

Fixed by previous patch.

	Luke Wagner [:luke]
	Comment 102 • 8 years ago

Comment on attachment 8792348 [details]
Testcase for previous comment

Fixed by previous patch.

	Luke Wagner [:luke]
	Comment 103 • 8 years ago

Comment on attachment 8792349 [details]
Testcase for previous comment

Fixed by previous patch.

	Luke Wagner [:luke]
	Comment 104 • 8 years ago

Comment on attachment 8792350 [details]
Testcase for previous comment

Fixed by previous patch.

	Luke Wagner [:luke]
	Comment 105 • 8 years ago

Comment on attachment 8792351 [details]
Testcase for previous comment

Fixed by previous patch.

	Luke Wagner [:luke]
	Comment 106 • 8 years ago

Comment on attachment 8792352 [details]
Testcase for previous comment

Fixed by previous patch.

	Luke Wagner [:luke]
	Comment 107 • 8 years ago

Comment on attachment 8792353 [details]
Testcase for previous comment

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

	Luke Wagner [:luke]
	Comment 108 • 8 years ago

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.

	Dan Gohman [:sunfish] Assignee
	Comment 109 • 8 years ago

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

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

Nice!

	Luke Wagner [:luke]
	Comment 110 • 8 years ago

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

	Luke Wagner [:luke]
	Comment 111 • 8 years ago

Updated to make the next patch cleaner.

	Luke Wagner [:luke]
	Comment 112 • 8 years ago

This patch skips user-defined sections.

	Luke Wagner [:luke]
	Comment 113 • 8 years ago

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

	Dan Gohman [:sunfish] Assignee
	Comment 114 • 8 years ago

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?

	Dan Gohman [:sunfish] Assignee
	Comment 115 • 8 years ago

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".

	Christian Holler (:decoder)
	Comment 116 • 8 years ago

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
[...]

	Christian Holler (:decoder)
	Comment 118 • 8 years ago

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
[...]

	Christian Holler (:decoder)
	Comment 120 • 8 years ago

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
[...]

	Christian Holler (:decoder)
	Comment 122 • 8 years ago

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
[...]

	Luke Wagner [:luke]
	Comment 124 • 8 years ago

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

	Luke Wagner [:luke]
	Comment 125 • 8 years ago

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!

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 126 • 8 years ago

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

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

Stealing per request, looks good!

	Pulsebot
	Comment 127 • 8 years ago

Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/958074f3b830
Baldr: update to binary version 0xc (r=luke)

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 128 • 8 years ago

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

	Luke Wagner [:luke]
	Comment 129 • 8 years ago

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...

	Pulsebot
	Comment 130 • 8 years ago

Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4119fba22f7f
Baldr: update to binary version 0xc (r=luke)

	Luke Wagner [:luke]
	Comment 131 • 8 years ago

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

	Iris Hsiao [:ihsiao]
	Comment 132 • 8 years ago
bugherder

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