Closed Bug 779813 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
Linux
defect
Not set
major

Tracking

()

VERIFIED FIXED
Tracking Status
firefox17 --- unaffected
firefox-esr10 --- unaffected

People

(Reporter: decoder, Assigned: djvj)

References

Details

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

Crash Data

Attachments

(1 file)

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);
  }
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: general → kvijayan
Attached patch Fix the bug.Splinter Review
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]
Duplicate of this bug: 779839
https://hg.mozilla.org/projects/ionmonkey/rev/1c42952f712b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 779814
Duplicate of this bug: 779837
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::ion::MPhi::addInput] [@ internalAppend] → [@ js::ion::MPhi::addInput] [@ internalAppend]
JSBugMon: This bug has been automatically verified fixed.
Crash Signature: [@ js::ion::MPhi::addInput] [@ internalAppend] → [@ js::ion::MPhi::addInput] [@ internalAppend]
Group: core-security
You need to log in before you can comment on or make changes to this bug.