Closed Bug 1293313 Opened 8 years ago Closed 8 years ago

[wasm] Assertion failure: JSVAL_IS_DOUBLE_IMPL(l), at dist/include/js/Value.h:406

Categories

(Core :: JavaScript Engine, defect)

ARM
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: decoder, Assigned: sunfish)

References

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

The attached binary WebAssembly testcase crashes on mozilla-inbound revision aa4472559aa9+ (build with --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). To reproduce, you can run the following code in the JS shell (running with --wasm-always-baseline might be necessary):

var data = os.file.readFile(file, 'binary');
Wasm.instantiateModule(new Uint8Array(data.buffer));



Backtrace:

==8780==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x0916223c bp 0xffa1efc8 sp 0xffa1ef00 T0)
    #0 0x916223b in DOUBLE_TO_JSVAL_IMPL(double) dist/include/js/Value.h:406:5
    #1 0x916223b in JS::Value::setDouble(double) dist/include/js/Value.h:1071
    #2 0x916223b in JS::DoubleValue(double) dist/include/js/Value.h:1460
    #3 0x916223b in js::jit::MToDouble::foldsTo(js::jit::TempAllocator&) js/src/jit/MIR.cpp:4138
    #4 0x950f22d in js::jit::ValueNumberer::simplified(js::jit::MDefinition*) const js/src/jit/ValueNumbering.cpp:618:12
    #5 0x950f22d in js::jit::ValueNumberer::visitDefinition(js::jit::MDefinition*) js/src/jit/ValueNumbering.cpp:777
    #6 0x951362e in js::jit::ValueNumberer::visitBlock(js::jit::MBasicBlock*, js::jit::MBasicBlock const*) js/src/jit/ValueNumbering.cpp:987:14
    #7 0x9513d29 in js::jit::ValueNumberer::visitDominatorTree(js::jit::MBasicBlock*) js/src/jit/ValueNumbering.cpp:1032:18
    #8 0x95147ae in js::jit::ValueNumberer::visitGraph() js/src/jit/ValueNumbering.cpp:1070:18
    #9 0x9516ae0 in js::jit::ValueNumberer::run(js::jit::ValueNumberer::UpdateAliasAnalysisFlag) js/src/jit/ValueNumbering.cpp:1241:14
    #10 0x8c90a59 in js::jit::OptimizeMIR(js::jit::MIRGenerator*) js/src/jit/Ion.cpp:1688:14
    #11 0xb5b8465 in js::wasm::IonCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3592:14
    #12 0xb5e5a58 in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3617:16
    #13 0xb557d48 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:866:14
    #14 0xb4de7fb in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/WasmCompile.cpp:1299:12
    #15 0xb4de7fb in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/WasmCompile.cpp:1359
    #16 0xb4de7fb in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs&&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/asmjs/WasmCompile.cpp:1640
    #17 0x82f2918 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/WasmJS.cpp:238:27
    #18 0x8222f8c in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5271:14
    #19 0xa46d89d in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235:15
[...]
    #33 0x80aea21 in _start (/home/ubuntu/build/build/js+0x80aea21)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV dist/include/js/Value.h:406:5 in DOUBLE_TO_JSVAL_IMPL(double)
==8780==ABORTING
Attached file Testcase
This repros reliably on a vanilla x64 debug build if you pass --no-threads.  I can confirm this is introduced by https://hg.mozilla.org/mozilla-central/rev/e5f310c9b166.

I assume the fix is pretty easy if you're familiar with the invariants of GVN -- we're sticking a non-canonical double value into a js::Value which is a no no -- but I don't really know the invariants here.  Dan, do you suppose you could look at this?
Blocks: 1248555
Flags: needinfo?(sunfish)
Attached patch nan-bits.patchSplinter Review
This fixes the constant-folding code to create new MConstants with NewRawDouble and NewRawFloat32 when the result of constant folding can be a non-canonical NaN.
Assignee: nobody → sunfish
Status: NEW → ASSIGNED
Flags: needinfo?(sunfish)
Attachment #8779422 - Flags: review?(luke)
Comment on attachment 8779422 [details] [diff] [review]
nan-bits.patch

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

Thanks!

::: js/src/jit/MIR.cpp
@@ +172,5 @@
> +    // constant-folding observable.
> +    if (retVal.isDouble())
> +        return MConstant::NewRawDouble(alloc, ret);
> +
> +    return MConstant::New(alloc, retVal);

I know this is largely pre-existing, but I feel like the logic could be simplified:

  if (ins->type() == MIRType::Double)
      return MConstant::NewRawDouble(alloc, ret);

  Value retVal = NumberValue(JS::CanonicalizeNaN(ret));
  if (ins->type() != MIRTypeFromValue(retVal))
     ...

  return MConstant::New(alloc, retVal);
Attachment #8779422 - Flags: review?(luke) → review+
Pushed by dgohman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab58242977a
IonMonkey: Handle non-canonical NaNs in constant folding. r=luke
(In reply to Luke Wagner [:luke] from comment #4)
> ::: js/src/jit/MIR.cpp
> @@ +172,5 @@
> > +    // constant-folding observable.
> > +    if (retVal.isDouble())
> > +        return MConstant::NewRawDouble(alloc, ret);
> > +
> > +    return MConstant::New(alloc, retVal);
> 
> I know this is largely pre-existing, but I feel like the logic could be
> simplified:
> 
>   if (ins->type() == MIRType::Double)
>       return MConstant::NewRawDouble(alloc, ret);
> 
>   Value retVal = NumberValue(JS::CanonicalizeNaN(ret));
>   if (ins->type() != MIRTypeFromValue(retVal))
>      ...
> 
>   return MConstant::New(alloc, retVal);

Tidied up accordingly.
https://hg.mozilla.org/mozilla-central/rev/aab58242977a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.