IonMonkey: Assertion failure: isConstant(), at ion/MIR.h:5344 or Crash [@ internalAppend] or Crash [@ js::ion::MPhi::addInput]

VERIFIED FIXED

Status

()

--
major
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: decoder, Assigned: djvj)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Other Branch
x86
Linux
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 unaffected, firefox-esr10 unaffected)

Details

(Whiteboard: [jsbugmon:update][ion:p1:fx18], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
The following testcase asserts on ionmonkey revision 2169bca0c9a5 (run with --ion -n -m --ion-eager -a):


function contfrac(x, epsilon) {
  let maxerr = x;
  while (maxerr > epsilon)
    i = Math.floor();
}
for each (num in [Math.PI, Math.sqrt(2), 1 / (Math.sqrt(Math.E) - 1)])
  for each (eps in [1e-2, 1e-3, 1e-5, 1e-7, 1e-10]) {
    let frac = contfrac(num, eps);
  }
(Reporter)

Comment 1

6 years ago
S-s due to this opt-crash on 64 bit:

==11684== Invalid write of size 8
==11684==    at 0x77B80D: js::ion::MPhi::addInput(js::ion::MDefinition*) (Vector.h:790)
==11684==    by 0x71B1D7: js::ion::MBasicBlock::setBackedge(js::ion::MBasicBlock*) (MIRGraph.cpp:661)
==11684==    by 0x6D1C83: js::ion::IonBuilder::finishLoop(js::ion::IonBuilder::CFGState&, js::ion::MBasicBlock*) (IonBuilder.cpp:1303)
==11684==    by 0x6E4577: js::ion::IonBuilder::traverseBytecode() (IonBuilder.cpp:1112)
==11684==    by 0x6E62ED: js::ion::IonBuilder::build() (IonBuilder.cpp:344)
==11684==    by 0x6C10DB: js::ion::BuildMIR(js::ion::IonBuilder&, js::ion::MIRGraph&) (Ion.cpp:692)
==11684==    by 0x6C4843: bool js::ion::IonCompile<&(js::ion::TestCompiler(js::ion::IonBuilder&, js::ion::MIRGraph&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (Ion.cpp:839)
==11684==    by 0x6C4C4B: js::ion::CanEnterAtBranch(JSContext*, JSScript*, js::StackFrame*, unsigned char*) (Ion.cpp:992)
==11684==    by 0x4A4CCF: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:1516)
==11684==    by 0x623C84: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1063)
==11684==    by 0x6251DE: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:1094)
==11684==    by 0x4AAC61: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:318)
==11684==  Address 0x433fca0 is not stack'd, malloc'd or (recently) free'd
Crash Signature: [@ js::ion::MPhi::addInput] [@ internalAppend]
Summary: IonMonkey: Assertion failure: isConstant(), at ion/MIR.h:5344 or Crash [@ internalAppend] → IonMonkey: Assertion failure: isConstant(), at ion/MIR.h:5344 or Crash [@ internalAppend] or Crash [@ js::ion::MPhi::addInput]
(Assignee)

Updated

6 years ago
Assignee: general → kvijayan
(Assignee)

Comment 2

6 years ago
Created attachment 648357 [details] [diff] [review]
Fix the bug.

When inlining Math.floor with zero args, we forget to pop |this| as well as the callee function definition.  In the other cases, |discardCall()| handles this job, so it's not obvious that it needs to be done when there are no explicit args.

Change just adds |discardCall()| to clean up the stack as used elsewhere in the code.  One is tempted to special case this with two |pop()|s, but that introduces a potential that any change in call mechanics down the line (and thus changes to |discardCall| behaviour) renders this kind of special handling erroneous.  So I'm just doing it the standard way for future proofing purposes.
Attachment #648357 - Flags: review?(sstangl)
Comment on attachment 648357 [details] [diff] [review]
Fix the bug.

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

A number of other functions need the same change (grep for "MConstant *nan ="):

inlineMathFunction()
inlineMathAbs()
inlineMathRound()
inlineMathSqrt()
inlineMathPow() [but only the first time it occurs]

::: js/src/ion/MCallOptimize.cpp
@@ +338,5 @@
>      // Math.floor() == NaN.
>      if (argc == 0) {
> +        MDefinitionVector argv;
> +        if (!discardCall(argc, argv, current))
> +            return InliningStatus_Error;

It would be better to put the above 3 lines in a helper function, since they will need to be repeated in a bunch of places. Even better if it's infallible and doesn't store into a vector.
Attachment #648357 - Flags: review?(sstangl) → review+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1:fx18]
(Assignee)

Updated

6 years ago
Duplicate of this bug: 779839
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/1c42952f712b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Duplicate of this bug: 779814
(Assignee)

Updated

6 years ago
Duplicate of this bug: 779837
(Reporter)

Updated

6 years ago
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::ion::MPhi::addInput] [@ internalAppend] → [@ js::ion::MPhi::addInput] [@ internalAppend]
(Reporter)

Comment 8

6 years ago
JSBugMon: This bug has been automatically verified fixed.
Crash Signature: [@ js::ion::MPhi::addInput] [@ internalAppend] → [@ js::ion::MPhi::addInput] [@ internalAppend]
status-firefox-esr10: --- → unaffected
status-firefox17: --- → unaffected
Group: core-security
You need to log in before you can comment on or make changes to this bug.