Last Comment Bug 750925 - IonMonkey: crash in large test case
: IonMonkey: crash in large test case
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-01 15:25 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2012-07-30 08:59 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
large testcase that will fail (859.77 KB, application/x-xz)
2012-05-01 15:25 PDT, Marty Rosenberg [:mjrosenb]
no flags Details
/home/mrosenberg/patches/gc_fix-r0.patch (14.83 KB, patch)
2012-05-07 14:21 PDT, Marty Rosenberg [:mjrosenb]
no flags Details | Diff | Splinter Review
Patch with much refactoring (22.77 KB, patch)
2012-05-08 13:27 PDT, Marty Rosenberg [:mjrosenb]
dvander: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-05-01 15:25:37 PDT
Created attachment 620096 [details]
large testcase that will fail

When running the attached testcase, we eventually segfault.  As near as I can tell, the issue is from not tracing the invalidator when all existing jitted code has been invalidated, since the invalidated code cannot be traced.
opt/shell/js --ion-gvn=off -e "gcparam('maxBytes', 1024*1024*1024);" ./sqlite.js
I believe that --ion-gvn=off is completely unrelated to the failure.
Also, I suspect that this problem can also exist for the arguments rectifier.
Comment 1 Marty Rosenberg [:mjrosenb] 2012-05-03 07:27:15 PDT
Addendum: Not only is the invalidator not marked when we find an invalidated frame on the stack, but invalidated frames are never even seen when they are the only frames in an activation.  This fix is going to be a bit more complex than first anticipated.
Comment 2 Marty Rosenberg [:mjrosenb] 2012-05-07 14:21:24 PDT
Created attachment 621737 [details] [diff] [review]
/home/mrosenberg/patches/gc_fix-r0.patch
Comment 3 David Anderson [:dvander] 2012-05-07 15:16:59 PDT
Comment on attachment 621737 [details] [diff] [review]
/home/mrosenberg/patches/gc_fix-r0.patch

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

Thanks for taking the time to try this approach. Patch looks good - I think one quick round of refactoring would help a lot, especially to make future ExitFrame modification easier.

::: js/src/ion/CodeGenerator.cpp
@@ +479,5 @@
>      Label success, exception;
>      masm.branchTest32(Assembler::Zero, ReturnReg, ReturnReg, &exception);
>  
>      // Load the outparam vp[0] into output register(s).
> +    masm.loadValue(Address(StackPointer, IonExitFrameLayout::Size() + sizeof(IonCode*)), JSReturnOperand);

Here and below, let's wrap this computation in an IonExitFrameLayout::FrameSize() function or something (Size() is really just the prefix/header size).

::: js/src/ion/IonFrames.cpp
@@ +455,5 @@
>            case IonFrame_Exit:
> +            // The exit frame will not be ignored.
> +            tmp = ((IonCode**)frames.fp())-1;
> +            if (*tmp != NULL)
> +                MarkIonCodeRoot(trc, tmp, "Invalidator");

Can you hide this pointer sneaking, by adding a member function to IonExitFrameLayout?

@@ +465,5 @@
>              // We don't bother marking rectifier frames; its data is duplicated
>              // in the callee, and upon returning from the call, the rectifier's
>              // local storage is immediately removed.
> +            // However, the rectifier is still on the stack, and there aren't
> +            // any guarantees that it will be marked elsewhere!

Both of these comments should just be replaced with marking code. Just call MarkIonCode on the rectifier (you can get the cx from trc->context). It's important to get the compartment from the right place though - pass the IonActivation into this function and get activation->compartment().

::: js/src/ion/IonLinker.h
@@ +94,5 @@
> +        if (masm.exitCodePatch.offset() != 0) {
> +            Assembler::patchDataWithValueCheck(CodeLocationLabel(code, masm.exitCodePatch),
> +                                               ImmWord(uintptr_t(code)),
> +                                               ImmWord(uintptr_t(-1)));
> +        }

nit: move this to IonMacroAssembler, maybe in a function called link(IonCode *) or something.

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +191,5 @@
>  {
>      masm.linkExitFrame();
>  
> +    //Push the ioncode for this bailout onto the stack
> +    masm.exitCodePatch = masm.pushWithPatch(ImmWord(-1));

Given that all callers of linkExitFrame need to provide this word, I'd just modify linkExitFrame, so that it takes in an ImmWord, and sets exitCodePatch_ automatically.

@@ +248,5 @@
>      masm.testl(eax, eax);
>      masm.j(Assembler::Zero, &exception);
>  
> +    // remove the exitCode pointer from the stack.
> +    masm.addl(Imm32(sizeof(IonCode *)), esp);

And this ideally could be abstracted into popExitFrame or something. Note that the push/addl combo doesn't maintain the internal stack balance - I don't know if that matters.
Comment 4 Marty Rosenberg [:mjrosenb] 2012-05-08 13:27:56 PDT
Created attachment 622119 [details] [diff] [review]
Patch with much refactoring
Comment 5 David Anderson [:dvander] 2012-05-08 14:47:40 PDT
Comment on attachment 622119 [details] [diff] [review]
Patch with much refactoring

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

::: js/src/gc/Statistics.cpp
@@ +555,5 @@
>      jschar *tmp = runtime->gcStats.formatMessage();
>      char buff[16384];
>      size_t len = 16384;
>      DeflateStringToBuffer(NULL, tmp, js_strlen(tmp), buff, &len);
> +    //printf("gc end\n%s\n", buff);

nit: don't forget to remove this

::: js/src/ion/Ion.cpp
@@ +750,5 @@
>  IonCompile(JSContext *cx, JSScript *script, StackFrame *fp, jsbytecode *osrPc)
>  {
>      struct timeval start, end;
>      gettimeofday(&start, NULL);
> +    //printf("compiling function at line: %d\n", script->lineno);

nit: don't forget to remove this, and below

::: js/src/ion/IonFrameIterator.h
@@ +96,5 @@
>      inline IonCommonFrameLayout *current() const;
>      inline uint8 *returnAddress() const;
>  
>      IonJSFrameLayout *jsFrame() const {
> +        JS_ASSERT(type() == IonFrame_Exit);

And this should be IonFrame_JS?

@@ +105,1 @@
>          JS_ASSERT(type() == IonFrame_JS);

This should be IonFrame_Exit, right?

::: js/src/ion/IonMacroAssembler.h
@@ +398,5 @@
> +
> +    // If the IonCode that created this assembler needs to transition into the VM,
> +    // we want to store the IonCode on the stack in order to mark it during a GC.
> +    // This is a reference to a patch location where the IonCode* will be written.
> +    CodeOffsetLabel exitCodePatch;

nit: exitCodePatch can be moved to the private section now (and suffixed with _)

::: js/src/ion/MIR.cpp
@@ +290,2 @@
>      }
> +    //printf("total: %d\n", tot);

nit: Don't forget to remove these

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +2428,5 @@
>  MacroAssemblerARMCompat::linkExitFrame() {
>      uint8 *dest = ((uint8*)GetIonContext()->cx->runtime) + offsetof(JSRuntime, ionTop);
>      movePtr(ImmWord(dest), ScratchRegister);
>      ma_str(StackPointer, Operand(ScratchRegister, 0));
> +    

nit: extraneous newline

::: js/src/ion/shared/MacroAssembler-x86-shared.h
@@ +178,5 @@
>          push(t);
>          framePushed_ += sizeof(double);
>      }
> +    CodeOffsetLabel PushWithPatch(const ImmWord &word) {
> +        return pushWithPatch(word);

Did you mean to adjust the stack counting in here? (same on ARM)

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