771200 - IonMonkey: Opt-only Crash [@ js::gc::MarkThingOrValueRoot]

Status: RESOLVED DUPLICATE of bug 779390

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Testcase for shell

The attached testcase crashes on ionmonkey revision 9cf3ea112635 (run with --ion -n -m --ion-eager).

Comment 1

[Christian Holler (:decoder)]

Stack:

==17287== Invalid read of size 1
==17287==    at 0x5FFEB9: js::gc::MarkThingOrValueRoot(JSTracer*, unsigned long*, char const*) (Marking.cpp:296)
==17287==    by 0x6D03D2: js::ion::MarkIonActivations(JSRuntime*, JSTracer*) (IonFrames.cpp:461)
==17287==    by 0x46B3B7: _ZN2jsL11MarkRuntimeEP8JSTracerb.clone.0 (jsgc.cpp:2449)
==17287==    by 0x46CA2E: IncrementalMarkSlice(JSRuntime*, long, js::gcreason::Reason, bool*) (jsgc.cpp:3142)
==17287==    by 0x46F8F4: GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3832)
==17287==    by 0x470E94: Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3945)
==17287==    by 0x51C108: js_NewStringCopyN(JSContext*, char const*, unsigned long) (jsgcinlines.h:411)
==17287==    by 0x4AC8A4: js_NumberToStringWithBase(JSContext*, double, int) (jsnum.cpp:1189)
==17287==    by 0x511355: js::ToStringSlow(JSContext*, JS::Value const&) (jsstr.cpp:3254)
==17287==    by 0x512F7B: js_String(JSContext*, unsigned int, JS::Value*) (jsstr.h:128)
==17287==    by 0x4045571: ???
==17287==    by 0xC71C1FF: ???
==17287==  Address 0x18 is not stack'd, malloc'd or (recently) free'd

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Christian, this is the output I get:

SyntaxError: missing } in compound statement
ReferenceError: PST_JAN_1_2005 is not defined
BUGNUMBER: 334807
ReferenceError: printStatus is not defined
ReferenceError: RECORDLOOP is not defined
testfunc : FAILED: expected number ( Infinity ) != actual 
testfunc : FAILED: expected number ( 1.7976931348623157e+308 ) != actual 
testfunc : FAILED: expected number ( 5e-324 ) != actual 
testfunc : FAILED: expected number ( 1.7976931348623157e+308 ) != actual 
testfunc : FAILED: expected number ( 5e-324 ) != actual 
testfunc : FAILED: expected number ( 1 ) != actual 
testfunc : FAILED: expected number ( 1.5707963267948966 ) != actual 
testfunc : FAILED: expected number ( 0.0001 ) != actual 
testfunc : FAILED: expected number ( NaN ) != actual 

Following the instructions in README, on both tip and the given changeset. Can you still reproduce this? If so I might have to ask for a core dump.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Oops, the bot cannot yet "cope with archived testcases yet."

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: an fix

Okay, the approach was working after all. Gary, could you re-test this just to make sure the crash goes away? (It was difficult to reproduce the segfault so I want to make sure.)

This patch instruments the code generation for LStackArg, to track which stack slots get pushed. Every time we visit an instruction that has a safepoint, we add the current pending LStackArg slots to its safepoint.

I feel kind of gross about this, but there aren't many other alternatives. Ideally we'd be able to just scan a delimited range in MarkIonJSFrame. To do that we'd have to push parameters in reverse order (that's not a change we should make right now), or delay LStackArgs until we reach the call, which would introduce longer live ranges. I'd be happy if there was a clearer way to fill the safepoint but I don't see that either.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 642123 [details]
an fix

Review of attachment 642123 [details]:
-----------------------------------------------------------------

Add something to discard pushedArgs at the call-site and r+.

You might also consider the fact that arguments are pre-allocated vector of a predefined length which is initialized contiguously.  This imply that you don't need to store the n arguments of each functions and that you might only push a tuple of the base StackOffset ( framePushed() ) with the number of initialized arguments ( n ).

[ locals | (n - 3) undefined | 3 defined | (n - 1) undefined | 1 defined ]

This can be translated by pushing the StackOffset to pushedArgumentSlots at the MPrepareCall and increment the number of argument initialized — of the last pushedArgumentSlots — in visitStackArg.  Then a visitCall* will just pop the last entry.

::: js/src/ion/CodeGenerator.cpp
@@ +402,5 @@
>      uint32 argslot = lir->argslot();
>      int32 stack_offset = StackOffsetOfPassedArg(argslot);
>  
>      masm.storeValue(val, Address(StackPointer, stack_offset));
> +    return pushedArgumentSlots_.append(StackOffsetToSlot(stack_offset));

Where do you clear the pushedArgumentsSlots_? I think you can use ShrinkBy at the visitCall* which are expecting StackArg.

@@ +2604,5 @@
>  {
>      JSContext *cx = gen->cx;
>  
> +    unsigned slots = graph.localSlotCount() +
> +                     (graph.argumentSlotCount() * sizeof(Value) / STACK_SLOT_SIZE);

Either there is a naming issue or this feel inconsistent.

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +135,4 @@
>      }
>  
>      inline int32 SlotToStackOffset(int32 slot) const {
>          JS_ASSERT(slot > 0 && slot <= int32(graph.localSlotCount()));

Does this assertion hold without argumentSlotCount?

@@ +143,5 @@
> +    inline int32 StackOffsetToSlot(int32 offset) const {
> +        // offset = framePushed - (slot * STACK_SLOT_SIZE)
> +        // offset + (slot * STACK_SLOT_SIZE) = framePushed
> +        // slot * STACK_SLOT_SIZE = framePushed - offset
> +        // slot = (framePushed - offset) / STACK_SLOT_SIZE

nit: // see SlotToStackOffset.

Comment 6

[Nicolas B. Pierron [:nbp]]

I forgot to mention that a good test case might be something which looks like:

var i = 0;
var j = 0;
var k = 0;

function g (a,b,c,d) {
  try {} catch(x) {};
  if (k == 100 && i++ == j) gc();
}

function f () {
  g(
    g(
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) )
    ),
    g(
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) )
    ),
    g(
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) )
    ),
    g(
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) ),
      g( g(1,2,3,4), g(1,2,3,4), g(1,2,3,4), g(1,2,3,4) )
    )
  )
}

for (; j <= 85; j++) {
  i = 0;
  for (k = 0; k <= 100; k++)
     f();
}

Comment 7

[Sean Stangl [:sstangl]]

Comment on attachment 642123 [details]
an fix

Review of attachment 642123 [details]:
-----------------------------------------------------------------

This is OK. It's a little sad that walking over the initialized contiguous slots in the argument vector by index didn't work out. That would have let us keep the distinction between local slots and argument vector slots, which is marginally less complicated.

::: js/src/ion/CodeGenerator.cpp
@@ +1141,5 @@
>          current = graph.getBlock(i);
>          for (LInstructionIterator iter = current->begin(); iter != current->end(); iter++) {
>              IonSpew(IonSpew_Codegen, "instruction %s", iter->opName());
> +            if (iter->safepoint() && pushedArgumentSlots_.length()) {
> +                if (!markArgumentSlots(iter->safepoint()))

As pierron pointed out, pushedArgumentSlots_ needs to be reset by the LCall* instructions. Those instructions must therefore also take care to call markArgumentSlots() themselves.

::: js/src/ion/IonCode.h
@@ +210,5 @@
>  
>      // Local slot count and frame size. Frame size is the value that can
>      // be added to the StackPointer along with the frame prefix to get a
>      // valid IonJSFrameLayout.
> +    uint32 frameSlots_;

Comment doesn't mention that it includes maximum length of nested argument vectors.

::: js/src/ion/Safepoints.cpp
@@ +269,5 @@
>  
>  SafepointReader::SafepointReader(IonScript *script, const SafepointIndex *si)
>    : stream_(script->safepoints() + si->safepointOffset(),
>              script->safepoints() + script->safepointsSize()),
> +    localSlotCount_(script->frameSlots())

Does localSlotCount_ also require renaming now?

::: js/src/ion/shared/CodeGenerator-shared.h
@@ +94,5 @@
>  
>      // Vector of all patchable write pre-barrier offsets.
>      js::Vector<CodeOffsetLabel, 0, SystemAllocPolicy> barrierOffsets_;
>  
> +    // List of pushed argument slots.

nit: // List of pushed slots for MCall arguments.
(as opposed to MParameters).

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 642123 [details]
an fix

And of course, somehow, I posted this in the completely wrong security bug. I've fixed the nits and will post a final corrected patch where it belongs in bug 771398.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Bisect shows this stopped reproducing on a seemingly irrelevant changeset - debugging the original cset, this is definitely bug 779390.

Comment 10

[Christian Holler (:decoder)]

A testcase for this bug was already added in the original bug (bug 779390).