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)

Other Branch
x86
Linux
defect
Not set
major

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)

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();
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
Whiteboard: [jsbugmon:update] → [jsbugmon:update][js-triage-needed]
Attached patch fix (obsolete) — Splinter Review
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)
Attached patch v2Splinter Review
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 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+
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 :(
http://hg.mozilla.org/projects/ionmonkey/rev/3359300edfe7
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: core-security
You need to log in before you can comment on or make changes to this bug.