Closed Bug 1279539 Opened 4 years ago Closed 4 years ago

[wasm] Assertion failure: dest.isDouble(), at js/src/jit/arm/Assembler-arm.cpp:2139

Categories

(Core :: JavaScript Engine, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: gkw, Assigned: bbouvier)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update])

Attachments

(2 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision 3ccccf8e5036 (build with --32 --enable-debug --enable-more-deterministic --enable-simulator=arm, run with --fuzzing-safe --no-threads --ion-eager):

// Adapted from randomly chosen testcase: js/src/jit-test/tests/wasm/basic.js
Wasm.instantiateModule(wasmTextToBinary('\
    (module (import "a" "" (param f32)) (func (param f32) (call_import 0 (get_local 0))) (export "" 0))\
'), {}).exports;


Backtrace:

0   js-dbg-32-dm-clang-armSim-darwin-3ccccf8e5036	0x00555a24 js::jit::Assembler::as_FImm64Pool(js::jit::VFPRegister, double, js::jit::Assembler::Condition) + 324 (Assembler-arm.cpp:2139)
1   js-dbg-32-dm-clang-armSim-darwin-3ccccf8e5036	0x00586fa1 js::jit::MacroAssemblerARMCompat::loadConstantDouble(double, js::jit::VFPRegister) + 49 (MacroAssembler-arm.cpp:3014)
2   js-dbg-32-dm-clang-armSim-darwin-3ccccf8e5036	0x0060e4d4 void js::jit::MacroAssembler::storeDouble<js::jit::Address>(js::jit::VFPRegister, js::jit::Address const&) + 116 (IonAssemblerBuffer.h:26)
3   js-dbg-32-dm-clang-armSim-darwin-3ccccf8e5036	0x00505d6c FillArgumentArray(js::jit::MacroAssembler&, mozilla::Vector<js::wasm::ValType, 8ul, js::SystemAllocPolicy> const&, unsigned int, unsigned int, js::jit::Register, bool) + 1516 (WasmStubs.cpp:356)
4   js-dbg-32-dm-clang-armSim-darwin-3ccccf8e5036	0x00504db8 js::wasm::GenerateInterpExit(js::jit::MacroAssembler&, js::wasm::Import const&, unsigned int) + 360 (MacroAssembler.h:1949)
5   js-dbg-32-dm-clang-armSim-darwin-3ccccf8e5036	0x0018f634 js::wasm::ModuleGenerator::finishCodegen(js::wasm::StaticLinkData*) + 820 (WasmGenerator.cpp:381)
6   js-dbg-32-dm-clang-armSim-darwin-3ccccf8e5036	0x00194aee js::wasm::ModuleGenerator::finish(mozilla::Vector<js::wasm::CacheableChars, 0ul, js::SystemAllocPolicy>&&, mozilla::UniquePtr<js::wasm::CodeSegment, JS::DeletePolicy<js::wasm::CodeSegment> >*, RefPtr<js::wasm::Metadata const>*, RefPtr<js::wasm::StaticLinkData const>*, RefPtr<js::wasm::ExportMap const>*, mozilla::Vector<js::wasm::SlowFunction, 0ul, js::TempAllocPolicy>*) + 366 (WasmGenerator.cpp:897)
/snip

For detailed crash information, see attachment.
Summary: Assertion failure: dest.isDouble(), at js/src/jit/arm/Assembler-arm.cpp:2139 → [wasm] Assertion failure: dest.isDouble(), at js/src/jit/arm/Assembler-arm.cpp:2139
Flags: needinfo?(bbouvier)
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2c86039b1c68
user:        Jeff Walden
date:        Mon May 23 22:49:56 2016 +0200
summary:     Bug 1245627: Canonicalize before storing a floating point value in deterministic mode; r=nbp

Not sure what is at fault here (WebAssembly or this changeset). I was about to needinfo :bbouvier but seems like he just got it seconds ahead of me.
Blocks: 1245627
Flags: needinfo?(bbouvier)
Attached patch storedouble.patch (obsolete) — Splinter Review
For some reason, hg won't let me push this patch to mozreview:  cannot submit reviews referencing multiple bugs

Anyways, see commit message for explanation.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8762460 - Flags: review?(luke)
Attachment #8762460 - Flags: review?(luke) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcd9e1d4254a
Mark register as float when storing a float arg for a call; r=luke
Backed out for compile error on arm64:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6483d4dfb3

Push with failure: https://treeherder.mozilla.org/logviewer.html#?job_id=30076672&repo=mozilla-inbound
/home/worker/workspace/build/src/js/src/asmjs/WasmStubs.cpp:352:33: error: 'struct js::jit::FloatRegister' has no member named 'asDouble'
Flags: needinfo?(bbouvier)
Duh, asSingle()/asDouble() are implementation specific.

We were actually using storeDouble for storing only a Float32. Let's make the code more explicit here (and by the way remove a source of confusion for the custom NaN patch), which has the nice side-effect of also avoiding this fuzzbug.
Attachment #8762460 - Attachment is obsolete: true
Flags: needinfo?(bbouvier)
Attachment #8762916 - Flags: review?(luke)
Comment on attachment 8762916 [details] [diff] [review]
canonicalization.patch

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

::: js/src/asmjs/WasmStubs.cpp
@@ +361,1 @@
>              if (toValue) {

How about putting the Float32 path in the 'else' branch of the above 'if (type == Double)'.  Then you don't need the 'break's.
Attachment #8762916 - Flags: review?(luke) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e518fcf4301
Simplify passing float32 arguments to a wasm FFI; r=luke
https://hg.mozilla.org/mozilla-central/rev/3e518fcf4301
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.