Closed Bug 759312 Opened 12 years ago Closed 12 years ago

IonMonkey: Assertion failure: [infer failure] Missing type in object [0xf77001a0] root_: [0xf7700220], at jsinfer.cpp:320 or Crash [@ markIfUnmarked]

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
Linux
defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: decoder, Assigned: dvander)

References

Details

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

Attachments

(2 files)

Attached file Testcase for shell
The attached testcase asserts on ionmonkey revision 4ce3983a43f4 (run with --ion -n -m --ion-eager).
There is some memory corruption involved here too. Valgrind shows this in a release shell, followed by SIGILL:


==13124== Invalid read of size 4
==13124==    at 0x826287B: js::gc::PushMarkStack(js::GCMarker*, js::BaseShape*) (Heap.h:678)
==13124==    by 0x8262C32: js::gc::ScanShape(js::GCMarker*, js::Shape*) (Marking.cpp:602)
==13124==    by 0x8264E21: js::GCMarker::drainMarkStack(js::SliceBudget&) (Marking.cpp:573)
==13124==    by 0x80C1A8A: GCCycle(JSRuntime*, bool, long long, js::JSGCInvocationKind) (jsgc.cpp:3310)
==13124==    by 0x80C2F01: Collect(JSRuntime*, bool, long long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3769)
==13124==    by 0x80C3248: js::gc::RunDebugGC(JSContext*) (jsgc.cpp:3793)
==13124==    by 0x831C490: js::ion::IonCode::New(JSContext*, unsigned char*, unsigned int, JSC::ExecutablePool*) (jsgcinlines.h:413)
==13124==    by 0x83E1E4D: js::ion::CodeGenerator::generate() (IonLinker.h:86)
==13124==    by 0x831EE61: IonCompile(JSContext*, JSScript*, js::StackFrame*, unsigned char*) (Ion.cpp:749)
==13124==  Address 0x8dfcbc8 is 17,728 bytes inside a block of size 32,768 free'd
==13124==    at 0x77A020B: free (vg_replace_malloc.c:366)
==13124==    by 0x824A75C: js::LifoAlloc::freeAll() (Utility.h:169)
==13124==    by 0x80B8E36: PurgeRuntime(JSTracer*) (jsgc.cpp:2918)
==13124==    by 0x80BF1D4: BeginMarkPhase(JSRuntime*) (jsgc.cpp:2991)
==13124==    by 0x80C1A29: GCCycle(JSRuntime*, bool, long long, js::JSGCInvocationKind) (jsgc.cpp:3306)
==13124==    by 0x80C2F01: Collect(JSRuntime*, bool, long long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3769)
==13124==    by 0x80C3248: js::gc::RunDebugGC(JSContext*) (jsgc.cpp:3793)
==13124==    by 0xFECD3287: ???


GDB just crashes in markIfUnmarked.
Assignee: general → dvander
Status: NEW → ASSIGNED
Attached patch fixSplinter Review
This bug is me forgetting to implement torn value tracing. This patch:
 * Changes LSRA to filter which torn values get added to safe points.
 * Encodes torn nunboxes in safepoints. Compaction is similar to snapshots,
   ideally in the future this could be shared.
 * Removed the LSafepoint::spillRegs optimization since this caused a bug that
   took hours to track down: nunbox regs that were in WrapperMask were elided.
   Can be brought back if it ends up mattering but reg spilling isn't the fast
   path.
Attachment #628202 - Flags: review?(jdemooij)
This patch bloats encoded safe points by at least one byte. Eyeballing the test case here, about 75% of x86 safepoints have no torn nunboxes, and the majority of ones that do, have only one entry. Also out-of-line calls may bloat by a few bytes to push/pop registers that are non-volatile, live, and not holding gc things.
Comment on attachment 628202 [details] [diff] [review]
fix

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

LGTM, r=me with comments addressed.

::: js/src/ion/IonFrames.cpp
@@ +415,5 @@
> +        return *frame.jsFrame()->slotRef(slot);
> +    }
> +    uint32 index = a->toArgument()->index();
> +    uint8 *argv = reinterpret_cast<uint8 *>(frame.jsFrame()->argv());
> +    return *reinterpret_cast<uintptr_t *>(argv + index);

Don't we want "Value *" or something here? I think argv + index is wrong since argv points to Value-sized data, not uint8.

::: js/src/ion/Safepoints.cpp
@@ +162,5 @@
> +//     If ppp = Reg, payload is reg YYYYY
> +//
> +//     If ttt != Reg, type is:
> +//          XXXXX if not 11111, otherwise followed by [vwu]
> +//     If ppp != Reg, payloa is:

Micro nit: payload

@@ +180,5 @@
> +static const uint32 MAX_INFO_VALUE = (1 << PART_INFO_BITS) - 1;
> +static const uint32 TYPE_KIND_SHIFT = 16 - PART_KIND_BITS;
> +static const uint32 PAYLOAD_KIND_SHIFT = TYPE_KIND_SHIFT - PART_KIND_BITS;
> +static const uint32 TYPE_INFO_SHIFT = PAYLOAD_KIND_SHIFT - PART_INFO_BITS;
> +static const uint32 PAYLOAD_INFO_SHIFT = TYPE_INFO_SHIFT - PART_INFO_BITS;

A JS_STATIC_ASSERT(PAYLOAD_INFO_SHIFT == 0); would be good as an extra sanity check.

@@ +206,5 @@
> +        *out = a.toStackSlot()->slot();
> +    else
> +        *out = a.toArgument()->index();
> +
> +    return *out != MAX_INFO_VALUE;

return *out < MAX_INFO_VALUE;

@@ +238,5 @@
> +
> +        uint32 typeVal;
> +        bool typeExtra = !CanEncodeInfoInHeader(entry.type, &typeVal);
> +        if (!typeExtra)
> +            header |= (typeVal << TYPE_INFO_SHIFT);

Don't we have to encode MAX_INFO_VALUE (11111) if typeExtra/payloadExtra?

It may be good to (temporarily) test with MAX_INFO_VALUE == 0 or 1. Maybe we should have some (configure-) flag to set all kinds of unusual limits so that we can exercise these paths better.

@@ +375,5 @@
> +    if (info == MAX_INFO_VALUE)
> +        info = stream.readUnsigned();
> +
> +    if (kind == Part_Stack)
> +        return LStackSlot(info);

JS_ASSERT(kind == Part_Arg); to complain when we end up with an invalid or new |kind|.
Attachment #628202 - Flags: review?(jdemooij) → review+
> > +    uint32 index = a->toArgument()->index();
> > +    uint8 *argv = reinterpret_cast<uint8 *>(frame.jsFrame()->argv());
> > +    return *reinterpret_cast<uintptr_t *>(argv + index);
> 
> Don't we want "Value *" or something here? I think argv + index is wrong
> since argv points to Value-sized data, not uint8.

LArgument's "index" is a byte offset, confusingly. Someday we should change either the name or the way the offset is computed. Thanks for the careful review, I'm testing with MAX_INFO_VALUE == 1 right now.
w/ nits picked: http://hg.mozilla.org/projects/ionmonkey/rev/06a664ebc3cc
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
Group: core-security
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.