Closed
Bug 772903
Opened 12 years ago
Closed 12 years ago
IonMonkey: Assertion failure: !isStackSlot(), at ../ion/SnapshotReader.h:130 or Crash [@ js::gc::MarkInternal]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
firefox16 | --- | unaffected |
firefox-esr10 | --- | unaffected |
People
(Reporter: decoder, Assigned: dvander)
References
Details
(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update][js-triage-needed])
Attachments
(1 file, 1 obsolete file)
26.96 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
The following testcase asserts on ionmonkey revision 3ff7d89ec13d (run with --ion -n -m --ion-eager): function reportCompare (expected, actual, description) {} function f() { var ss = []; } function fannkuch() { for (var j = 0; j < 50; reportCompare(f(true))) { for (var i = 0; i < (null ); i++) { arguments, Math; } } } fannkuch();
Reporter | ||
Comment 1•12 years ago
|
||
S-s due to GC-related crash and Valgrind errors in opt-build: ==3997== Invalid read of size 4 ==3997== at 0x825ED27: void js::gc::MarkInternal<JSObject>(JSTracer*, JSObject**) (Heap.h:964) ==3997== by 0x826494A: js::gc::MarkValueRoot(JSTracer*, JS::Value*, char const*) (Marking.cpp:370) ==3997== by 0x81FDC9B: js::StackSpace::markFrameValues(JSTracer*, js::StackFrame*, JS::Value*, unsigned char*) (Stack.cpp:634) ==3997== by 0x81FDF7F: js::StackSpace::mark(JSTracer*) (Stack.cpp:689) ==3997== by 0x80BDCAC: _ZN2jsL11MarkRuntimeEP8JSTracerb.clone.0 (jsgc.cpp:2448) ==3997== by 0x80BECF5: IncrementalMarkSlice(JSRuntime*, long long, js::gcreason::Reason, bool*) (jsgc.cpp:3149) ==3997== by 0x80BFFB1: GCCycle(JSRuntime*, bool, long long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3862) ==3997== by 0x80C1651: Collect(JSRuntime*, bool, long long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3975) ==3997== by 0x80C1A23: js::GCSlice(JSRuntime*, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:4005) ==3997== by 0x808BC49: js_HandleExecutionInterrupt(JSContext*) (jscntxt.cpp:927) ==3997== by 0x83801CF: js::ion::InterruptCheck(JSContext*) (VMFunctions.cpp:373) ==3997== by 0x7B7A62B: ??? ==3997== Address 0x8cd3000 is 16 bytes before a block of size 240 alloc'd ==3997== at 0x77A082F: malloc (vg_replace_malloc.c:236) ==3997== by 0x81724A6: js::InflateString(JSContext*, char const*, unsigned int*, js::FlationCoding) (Utility.h:157) ==3997== by 0x808394B: js_Atomize(JSContext*, char const*, unsigned int, js::InternBehavior, js::FlationCoding) (jsatom.cpp:401) ==3997== by 0x80AFBFE: JS_DefineFunctionsWithHelp (jsfriendapi.cpp:228) ==3997== by 0x804B597: NewGlobalObject(JSContext*) (js.cpp:4571) ==3997== by 0x805138B: Shell(JSContext*, js::cli::OptionParser*, char**) (js.cpp:4807) ==3997== by 0x8052FDB: main (js.cpp:5027) ==3997== ==3997== Invalid read of size 4 ==3997== at 0x825ED29: void js::gc::MarkInternal<JSObject>(JSTracer*, JSObject**) (jscntxt.h:596) ==3997== by 0x826494A: js::gc::MarkValueRoot(JSTracer*, JS::Value*, char const*) (Marking.cpp:370) ==3997== by 0x81FDC9B: js::StackSpace::markFrameValues(JSTracer*, js::StackFrame*, JS::Value*, unsigned char*) (Stack.cpp:634) ==3997== by 0x81FDF7F: js::StackSpace::mark(JSTracer*) (Stack.cpp:689) ==3997== by 0x80BDCAC: _ZN2jsL11MarkRuntimeEP8JSTracerb.clone.0 (jsgc.cpp:2448) ==3997== by 0x80BECF5: IncrementalMarkSlice(JSRuntime*, long long, js::gcreason::Reason, bool*) (jsgc.cpp:3149) ==3997== by 0x80BFFB1: GCCycle(JSRuntime*, bool, long long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3862) ==3997== by 0x80C1651: Collect(JSRuntime*, bool, long long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3975) ==3997== by 0x80C1A23: js::GCSlice(JSRuntime*, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:4005) ==3997== by 0x808BC49: js_HandleExecutionInterrupt(JSContext*) (jscntxt.cpp:927) ==3997== by 0x83801CF: js::ion::InterruptCheck(JSContext*) (VMFunctions.cpp:373) ==3997== by 0x7B7A62B: ??? ==3997== Address 0x0 is not stack'd, malloc'd or (recently) free'd
Updated•12 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][js-triage-needed]
Assignee | ||
Comment 2•12 years ago
|
||
This is a bug somewhere in the snapshot handling of lazy arguments. Reading over IonBuilder today, I'm convinced that we don't need MIRType_ArgsObj: (1) OPTIMIZED_ARGUMENTS is the only magic value ever allowed to have near-unrestricted flow on the stack. We are not going to support other magic types in the near future. (2) Because the lazy arguments value is a constant, it does not need to be reified anywhere in code. (3) There is no expectation that MArguments* will operate on other argument object types. (4) The identity of the lazy arguments object is guaranteed by type inference, so there's no need to guard on it. This patch removes MIRType_ArgsObj and creates a canonical lazy arguments constant at the start of MIR construction. During snapshot building we assume that any magic type is lazy arguments and just insert it as a constant.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #641269 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 3•12 years ago
|
||
This version ensures that OSR deduces lazy arguments, and that phis with lazy arguments are eliminated. Now we should be guaranteed that we never need to reify the value. (If that turns out to be false, we can just MBox lazyArguments_.)
Attachment #641269 -
Attachment is obsolete: true
Attachment #641269 -
Flags: review?(nicolas.b.pierron)
Attachment #641306 -
Flags: review?(nicolas.b.pierron)
Comment 4•12 years ago
|
||
Comment on attachment 641306 [details] [diff] [review] v2 Review of attachment 641306 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this patch. You might close Bug 771460 for being invalid after landing this patch. ::: js/src/ion/IonBuilder.cpp @@ +343,5 @@ > > // Recompile to inline calls if this function is hot. > insertRecompileCheck(); > > + if (script->argumentsHasVarBinding() && !script->needsArgsObj()) { We are not compiling the script if needsArgsObj() is true, you might remove this condition. script->argumentsHasLocalBinding() should be enough. @@ +3802,5 @@ > osrBlock->add(actual); > osrBlock->rewriteSlot(i, actual); > + } else if (type == MIRType_Magic) { > + JS_ASSERT(lazyArguments_); > + osrBlock->rewriteSlot(i, lazyArguments_); The OSR is another entry point, there is no issue about rewriting the slot to something which is coming from another branch? Ok, we have no reasons to use the value before the merge with basic block which has this definition, but this sounds a bit weird. @@ +4770,5 @@ > bool > IonBuilder::jsop_arguments_length() > { > + // Type Inference has guaranteed this is an optimized arguments object. > + current->pop(); Might be worth adding an assertion, especially if all are MConstant and all phis are removed due to the previous trick on the OSR path. DebugOnly<MDefintion*> arguments = current->pop(); JS_ASSERT(arguments->isConstant() && arguments->toConstant()->value().isMagic(JS_OPTIMIZED_ARGSUMENTS)); ::: js/src/ion/shared/CodeGenerator-shared.cpp @@ +158,5 @@ > { > + uint32 index; > + if (!graph.addConstantToPool(MagicValue(JS_OPTIMIZED_ARGUMENTS), &index)) > + return false; > + snapshots_.addConstantPoolSlot(index); Nice. So you should remove the JSVAL_TYPE_MAGIC case in SnapshotIterator::FromTypedPayload.
Attachment #641306 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 5•12 years ago
|
||
Thanks for the quick review! > > + osrBlock->rewriteSlot(i, lazyArguments_); > > The OSR is another entry point, there is no issue about rewriting the slot > to something which is coming from another branch? Ok, we have no reasons to > use the value before the merge with basic block which has this definition, > but this sounds a bit weird. Yeah I agree it's weird. It's just so the OSR phi will get eliminated. > Might be worth adding an assertion, especially if all are MConstant and all > phis are removed due to the previous trick on the OSR path. > > DebugOnly<MDefintion*> arguments = current->pop(); > JS_ASSERT(arguments->isConstant() && > arguments->toConstant()->value().isMagic(JS_OPTIMIZED_ARGSUMENTS)); Unfortunately the elimination happens after MIR building :(
Assignee | ||
Comment 6•12 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/3359300edfe7
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
status-firefox16:
--- → unaffected
Reporter | ||
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 7•12 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•