Last Comment Bug 759312 - IonMonkey: Assertion failure: [infer failure] Missing type in object [0xf77001a0] root_: [0xf7700220], at jsinfer.cpp:320 or Crash [@ markIfUnmarked]
: IonMonkey: Assertion failure: [infer failure] Missing type in object [0xf7700...
Status: VERIFIED FIXED
[jsbugmon:update]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Linux
: -- major (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
: 759210 759211 759300 (view as bug list)
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-05-29 05:58 PDT by Christian Holler (:decoder)
Modified: 2013-02-07 05:18 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase for shell (1.38 KB, text/javascript)
2012-05-29 05:58 PDT, Christian Holler (:decoder)
no flags Details
fix (15.41 KB, patch)
2012-05-29 20:49 PDT, David Anderson [:dvander]
jdemooij: review+
Details | Diff | Review

Description Christian Holler (:decoder) 2012-05-29 05:58:29 PDT
Created attachment 627928 [details]
Testcase for shell

The attached testcase asserts on ionmonkey revision 4ce3983a43f4 (run with --ion -n -m --ion-eager).
Comment 1 Christian Holler (:decoder) 2012-05-29 05:59:33 PDT
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.
Comment 2 David Anderson [:dvander] 2012-05-29 20:49:29 PDT
Created attachment 628202 [details] [diff] [review]
fix

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.
Comment 3 David Anderson [:dvander] 2012-05-29 20:56:38 PDT
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 4 David Anderson [:dvander] 2012-05-30 10:21:24 PDT
*** Bug 759211 has been marked as a duplicate of this bug. ***
Comment 5 Jan de Mooij [:jandem] 2012-05-30 10:33:41 PDT
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|.
Comment 6 David Anderson [:dvander] 2012-05-30 10:40:59 PDT
*** Bug 759300 has been marked as a duplicate of this bug. ***
Comment 7 David Anderson [:dvander] 2012-05-30 10:43:44 PDT
*** Bug 759210 has been marked as a duplicate of this bug. ***
Comment 8 David Anderson [:dvander] 2012-05-30 10:56:53 PDT
> > +    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.
Comment 9 David Anderson [:dvander] 2012-05-30 11:00:28 PDT
w/ nits picked: http://hg.mozilla.org/projects/ionmonkey/rev/06a664ebc3cc
Comment 10 Christian Holler (:decoder) 2012-05-30 12:11:18 PDT
JSBugMon: This bug has been automatically verified fixed.
Comment 11 Christian Holler (:decoder) 2013-02-07 05:18:35 PST
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397

Note You need to log in before you can comment on or make changes to this bug.