Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: dvander)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Other Branch
x86
Linux
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 627928 [details]
Testcase for shell

The attached testcase asserts on ionmonkey revision 4ce3983a43f4 (run with --ion -n -m --ion-eager).
(Reporter)

Comment 1

5 years ago
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)

Updated

5 years ago
Assignee: general → dvander
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
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.
Attachment #628202 - Flags: review?(jdemooij)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 759211
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+
(Assignee)

Updated

5 years ago
Duplicate of this bug: 759300
(Assignee)

Updated

5 years ago
Duplicate of this bug: 759210
(Assignee)

Comment 8

5 years ago
> > +    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.
(Assignee)

Comment 9

5 years ago
w/ nits picked: http://hg.mozilla.org/projects/ionmonkey/rev/06a664ebc3cc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 10

5 years ago
JSBugMon: This bug has been automatically verified fixed.
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Updated

5 years ago
Group: core-security
(Reporter)

Comment 11

5 years ago
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.