Closed Bug 1317675 Opened 3 years ago Closed 3 years ago

Assertion failure: IsDiscardable(def) (Discarding non-discardable definition), at js/src/jit/ValueNumbering.cpp:345

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: decoder, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

The attached binary WebAssembly testcase crashes on mozilla-inbound revision c6e0621b8155+ (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). 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');
new WebAssembly.Instance(new WebAssembly.Module(data.buffer));



Backtrace:

==18142==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000020b5142 bp 0x7ffe4af89f40 sp 0x7ffe4af89e20 T0)
    #0 0x20b5141 in js::jit::ValueNumberer::discardDef(js::jit::MDefinition*) js/src/jit/ValueNumbering.cpp:377:13
    #1 0x20b3d4a in js::jit::ValueNumberer::discardDefsRecursively(js::jit::MDefinition*) js/src/jit/ValueNumbering.cpp:277:12
    #2 0x20b71ba in js::jit::ValueNumberer::removePredecessorAndDoDCE(js::jit::MBasicBlock*, js::jit::MBasicBlock*, unsigned long) js/src/jit/ValueNumbering.cpp:497:18
    #3 0x20b80f5 in js::jit::ValueNumberer::removePredecessorAndCleanUp(js::jit::MBasicBlock*, js::jit::MBasicBlock*) js/src/jit/ValueNumbering.cpp:546:10
    #4 0x20bf13c in js::jit::ValueNumberer::visitUnreachableBlock(js::jit::MBasicBlock*) js/src/jit/ValueNumbering.cpp:941:14
    #5 0x20c102b in js::jit::ValueNumberer::visitDominatorTree(js::jit::MBasicBlock*) js/src/jit/ValueNumbering.cpp:1036:18
    #6 0x20c20f1 in js::jit::ValueNumberer::visitGraph() js/src/jit/ValueNumbering.cpp:1079:18
    #7 0x20c55e2 in js::jit::ValueNumberer::run(js::jit::ValueNumberer::UpdateAliasAnalysisFlag) js/src/jit/ValueNumbering.cpp:1250:14
    #8 0x167666f in js::jit::OptimizeMIR(js::jit::MIRGenerator*) js/src/jit/Ion.cpp:1679:14
    #9 0xdd0de6 in js::wasm::IonCompileFunction(js::wasm::IonCompileTask*) js/src/wasm/WasmIonCompile.cpp:3766:14
    #10 0xe12f9a in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/wasm/WasmIonCompile.cpp:3791:16
    #11 0xd2576c in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/wasm/WasmGenerator.cpp:958:14
    #12 0xc9fcae in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/wasm/WasmCompile.cpp:695:12
    #13 0xc9fcae in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:755
    #14 0xc9fcae in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmCompile.cpp:947
    #15 0xe86ee9 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/wasm/WasmJS.cpp:387:27
    #16 0x6056b0 in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5652:14
[...]
Attached file Testcase
Attached file test.js
JS test case. Looking.
A smaller test case:

const {Module} = WebAssembly;

new Module(wasmTextToBinary(`(module
  (type $type0 (func (param i32)))
  (func $f (param $p i32)
    (local $x i32) (local $y i32)
    loop $top
      get_local $x
      get_local $p
      get_local $x
      br_if $top
      i32.const 1
      tee_local $p
      get_local $y
      set_local $x
      i32.add
      call $f
      br_if $top
      return
    end
  )
)`));

GVN figures out that one phi should be eliminated, but it doesn't pass a check because the phi is marked as guardRangeBailout. Investigating a bit more...
Talking about this issue with Hannes made me think it might happen also in JS, under the same conditions. I made a JS test case out of the iongraph of the wasm one, and it triggers the same assertion for the same reason. The check removing phis is looking at phi->hasUses(), but it should probably consider guards too... http://searchfox.org/mozilla-central/source/js/src/jit/ValueNumbering.cpp#493

var count = 0;

function f($p) {
    var $cst2 = 0;
    var $cst3 = 0;
    var $cst4 = 0;

    // Uncomment to make the function terminate.
    //if (count++ > 10000)
    //    return;

    {
        var $phi5 = $p;
        var $phi6 = $cst2;
        while (true) {
            if (!$phi6) {
                var $cst11 = 1;
                var $add12 = $phi5 + $cst11;
                f($add12);
                if (!$phi6)
                    return;
                $phi5 = $cst11;
                $phi6 = $cst3;
            }
        }
    }
}

f(0);
Attached patch guard-phi.patch (obsolete) — Splinter Review
As discussed on irc. This is a bit strange to me, because it ends up creating, in this case, a phi with only one input... (the phi can't be removed since it's a guard)

Could we do better, and set the flag guardRangeBailouts on the phi's single input?
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8810925 - Flags: review?(hv1989)
Component: JavaScript Engine → JavaScript Engine: JIT
The first bad revision is:
changeset:   321902:6c0d7c338607
user:        Hannes Verschore <hv1989@gmail.com>
date:        Mon Nov 07 09:38:05 2016 +0100
summary:     Bug 1314438: IonMonkey - Guard we don't remove instructions where we optimized based on its type, r=nbp

Bug 1314438 added the line in MPhi::foldsTernary, so it should inherit the tracking and affected flags.
Priority: -- → P1
Comment on attachment 8810925 [details] [diff] [review]
guard-phi.patch

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

Good find.

FYI: we have "RangeAnalysis::tryRemovingGuards" which is called later and that will hoist the flags to the inputs. After that pass there shouldn't be a MPhi with guardRangeBailouts anymore.
But that uses range information to decide when to hoist. As a result we cannot to it earlier. But your suggestion that we can always hoist such a guard when it is on a MPhi is correct.
Though it might be a bit complex, since the input could be an MPhi also. Which we also need to hoist ... Not sure if it is worth it. Your patch indeed fixes the issue.

::: js/src/jit/ValueNumbering.cpp
@@ +493,5 @@
> +        while (nextDef_ &&
> +               !nextDef_->hasUses() &&
> +               !nextDef_->isGuardRangeBailouts() &&
> +               !nextDef_->isGuard())
> +        {

Oh I see. No need to test isGuard here. Can you assert that the phi can never be a "Guard" in the body of the while loop?
Attachment #8810925 - Flags: review?(hv1989) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f0f229b8449
Check that phis are not guards before removing them; r=h4writer
https://hg.mozilla.org/mozilla-central/rev/8f0f229b8449
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
tracking as regression in 52.  Benjamin, are you going to request uplift to aurora?
Flags: needinfo?(bbouvier)
Attached patch phis.patchSplinter Review
Thanks for the reminder, jcristau!

Approval Request Comment
[Feature/regressing bug #]: bug 1314438 (landed in 52)
[User impact if declined]: incorrect behavior in javascript/wasm programs
[Describe test coverage new/current, TreeHerder]: all green
[Risks and why]: very low risk (an optimization has been made more conservative)
[String/UUID change made/needed]: n/a
Attachment #8810925 - Attachment is obsolete: true
Flags: needinfo?(bbouvier)
Attachment #8814338 - Flags: review+
Attachment #8814338 - Flags: approval-mozilla-aurora?
Comment on attachment 8814338 [details] [diff] [review]
phis.patch

small patch to fix js jit regression in aurora52
Attachment #8814338 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.