IonMonkey: crash in large test case

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
Summary: IonMonkey: → IonMonkey: crash in large test case
(Reporter)

Comment 1

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

Comment 2

5 years ago
Created attachment 621737 [details] [diff] [review]
/home/mrosenberg/patches/gc_fix-r0.patch
Attachment #621737 - Flags: review?(dvander)
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.
Attachment #621737 - Flags: review?(dvander)
(Reporter)

Comment 4

5 years ago
Created attachment 622119 [details] [diff] [review]
Patch with much refactoring
Attachment #621737 - Attachment is obsolete: true
Attachment #622119 - Flags: review?(dvander)
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)
Attachment #622119 - Flags: review?(dvander) → review+
(Reporter)

Comment 6

5 years ago
landed:
http://hg.mozilla.org/projects/ionmonkey/rev/8b8ee1dc2342
http://hg.mozilla.org/projects/ionmonkey/rev/6e58d603a684
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.