Closed
Bug 779813
Opened 12 years ago
Closed 12 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)
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)
787 bytes,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
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•12 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•12 years ago
|
Assignee: general → kvijayan
Assignee | ||
Comment 2•12 years ago
|
||
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 3•12 years ago
|
||
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+
Updated•12 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1:fx18]
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/1c42952f712b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::ion::MPhi::addInput]
[@ internalAppend] → [@ js::ion::MPhi::addInput]
[@ internalAppend]
Reporter | ||
Comment 8•12 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•12 years ago
|
Crash Signature: [@ js::ion::MPhi::addInput]
[@ internalAppend] → [@ js::ion::MPhi::addInput]
[@ internalAppend]
status-firefox-esr10:
--- → unaffected
status-firefox17:
--- → unaffected
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•