Bug 151406 – OSR exits that are exception handlers should emit less code eagerly in the thunk generator, and instead, should defer as much code generation as possible to be lazily generated in the exit itself

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

Created attachment 266320 [details]
patch

[Geoffrey Garen]

Comment on attachment 266320 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266320&action=review

r=me

> Source/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp:120
> +        // This means that our frame isn't set to the actual frame of the function
> +        // we're exiting from, but to some callee down the call stack. This is fine,

I would just say, "When this happens, exec is the frame pointer for the callee that threw, and not for the caller that will do the OSR exit."

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:549
> +        // We may arrive at this OSR exit compilation from genericUnwind.
> +        // This means that our frame isn't set to the actual frame of the function
> +        // we're exiting from, but to some callee down the call stack. This is fine,

Ditto.

[Filip Pizlo]

Comment on attachment 266320 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266320&action=review

> Source/JavaScriptCore/ftl/FTLCompile.cpp:813
> -                        exit->spillRegistersToSpillSlot(slowPathJIT, jsCallThatMightThrowSpillOffset);
> +                        exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);

This appears to spill a ton of stuff, and we emit this code eagerly.  Why can't this be emitted lazily?  Do you have a bug for that?

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 266320 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266320&action=review
> 
> > Source/JavaScriptCore/ftl/FTLCompile.cpp:813
> > -                        exit->spillRegistersToSpillSlot(slowPathJIT, jsCallThatMightThrowSpillOffset);
> > +                        exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
> 
> This appears to spill a ton of stuff, and we emit this code eagerly.  Why
> can't this be emitted lazily?  Do you have a bug for that?

Never mind, I think I get it now.  These are the volatile registers.

[Saam Barati]

Im holding off on landing this because it's currently breaking 
32-bit. I've found out why. For some reason (I don't know why yet)
some path that runs jumpToExceptionHandler after genericUnwind has
a stack pointer that isn't 16-byte aligned. This makes us crash in
the actual compileOSRExit C call.
This was working before because we reset the stack pointer before 
the C call.

[Saam Barati]

Created attachment 266495 [details]
patch

Back up for review because I've changed the architecture of the patch to do frame/stack pointer resetting in
the exit generation thunk generation before the compileOSRExit C call.

[WebKit Commit Bot]

Attachment 266495 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGThunks.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/dfg/DFGThunks.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 266495 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266495&action=review

> Source/JavaScriptCore/ftl/FTLThunks.cpp:124
> -        vm, compileFTLOSRExit, "FTL OSR exit generation thunk", extraPopsToRestore);
> +        vm, compileFTLOSRExit, "FTL OSR exit generation thunk", extraPopsToRestore, true);

WebKit style doesn't take kindly to passing true or false like this.  You should either use an enum, or name the variable like:

bool doFrameAndStackAdjustment = true;
return genericGenerationThunkGenerator(
    vm, compileFTLOSRExit, "FTL OSR exit generation thunk", extraPopsToRestore, doFrameAndStackAdjustment);

> Source/JavaScriptCore/ftl/FTLThunks.cpp:131
> -        vm, compileFTLLazySlowPath, "FTL lazy slow path generation thunk", extraPopsToRestore);
> +        vm, compileFTLLazySlowPath, "FTL lazy slow path generation thunk", extraPopsToRestore, false);

Ditto.

[Saam Barati]

(In reply to comment #8)
> Comment on attachment 266495 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266495&action=review
> 
> > Source/JavaScriptCore/ftl/FTLThunks.cpp:124
> > -        vm, compileFTLOSRExit, "FTL OSR exit generation thunk", extraPopsToRestore);
> > +        vm, compileFTLOSRExit, "FTL OSR exit generation thunk", extraPopsToRestore, true);
> 
> WebKit style doesn't take kindly to passing true or false like this.  You
> should either use an enum, or name the variable like:
> 
> bool doFrameAndStackAdjustment = true;
> return genericGenerationThunkGenerator(
>     vm, compileFTLOSRExit, "FTL OSR exit generation thunk",
> extraPopsToRestore, doFrameAndStackAdjustment);
> 
> > Source/JavaScriptCore/ftl/FTLThunks.cpp:131
> > -        vm, compileFTLLazySlowPath, "FTL lazy slow path generation thunk", extraPopsToRestore);
> > +        vm, compileFTLLazySlowPath, "FTL lazy slow path generation thunk", extraPopsToRestore, false);
> 
> Ditto.
Thanks for the review. I'll go with an enum.

[Saam Barati]

Comment on attachment 266495 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266495&action=review

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.h:44
> +void adjustFrameAndStackInOSRExitCompilerThunk(MacroAssembler& jit, VM* vm, bool isFTLOSRExit)

I'll enum this as well.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/193485

[Filip Pizlo]

Comment on attachment 266495 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266495&action=review

> Source/JavaScriptCore/ftl/FTLThunks.cpp:52
> +
> +    if (doFrameAndStackAdjustment) {
> +        // This needs to happen before we use the scratch buffer because this function also uses the scratch buffer.
> +        adjustFrameAndStackInOSRExitCompilerThunk<FTL::JITCode>(jit, vm, true);
> +    }

This is a problem.  adjustFrameAndStackInOSRExitCompilerThunk() clobbers scratch registers.

Why can't it happen after saving all registers, below?

[Filip Pizlo]

(In reply to comment #12)
> Comment on attachment 266495 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266495&action=review
> 
> > Source/JavaScriptCore/ftl/FTLThunks.cpp:52
> > +
> > +    if (doFrameAndStackAdjustment) {
> > +        // This needs to happen before we use the scratch buffer because this function also uses the scratch buffer.
> > +        adjustFrameAndStackInOSRExitCompilerThunk<FTL::JITCode>(jit, vm, true);
> > +    }
> 
> This is a problem.  adjustFrameAndStackInOSRExitCompilerThunk() clobbers
> scratch registers.
> 
> Why can't it happen after saving all registers, below?

Actually, why do we do this for all OSR exits?  Isn't this only needed for those OSR exits that are reached by generic unwind?

It's important that we are very precise about when this is needed.  Getting this to not use a scratch register is going to be very tricky.  We could get around this problem if we only did this when the OSR exit is reached from generic unwind, since those will have volatile registers available anyway.

Also, is it really the case that the purpose of this function is to ensure that the stack is not clobbered by the OSR exit compiler?  If so, then this function appears to fail at this, because by the time we get here, the FTL would have pushed something onto the stack.  So, if the stack height was not as big as it should be, then this store would clobber something that OSR exit would want.

But, I could be completely wrong about what this function actually does.  I don't think that its name says anything about why it exists, and it's not clear to me what invariant it ensures.

[Saam Barati]

(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 266495 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=266495&action=review
> > 
> > > Source/JavaScriptCore/ftl/FTLThunks.cpp:52
> > > +
> > > +    if (doFrameAndStackAdjustment) {
> > > +        // This needs to happen before we use the scratch buffer because this function also uses the scratch buffer.
> > > +        adjustFrameAndStackInOSRExitCompilerThunk<FTL::JITCode>(jit, vm, true);
> > > +    }
> > 
> > This is a problem.  adjustFrameAndStackInOSRExitCompilerThunk() clobbers
> > scratch registers.
> > 
> > Why can't it happen after saving all registers, below?
> 
> Actually, why do we do this for all OSR exits?  Isn't this only needed for
> those OSR exits that are reached by generic unwind?
It's only needed for those particular unwind exits.

> It's important that we are very precise about when this is needed.  Getting
> this to not use a scratch register is going to be very tricky.  We could get
> around this problem if we only did this when the OSR exit is reached from
> generic unwind, since those will have volatile registers available anyway.
Correct. We could be way more precise here. I remember us discussing a while
ago when I wrote this patch if this should be done always or just when strictly
necessary, and we came to the consensus that it's better to do it all the time
because we have complete control. But given that we've found a bug, maybe that's
a fragile design. I also vaguely remember that we thought that it's safe
to always increment SP from FP to what the FTL call frame excepts because
we could theoretically get here from some other side code that did threw
an exception and didn't reset SP if it happened to store things to the
stack (I could totally be making this up). 
 
> Also, is it really the case that the purpose of this function is to ensure
> that the stack is not clobbered by the OSR exit compiler?  If so, then this
> function appears to fail at this, because by the time we get here, the FTL
> would have pushed something onto the stack.  So, if the stack height was not
> as big as it should be, then this store would clobber something that OSR
> exit would want.
After this code executes, we will strictly decrease the stack (or maintain its original value), we never increase it. We could theoretically reuse the SP from where the
exception was thrown, but this seems shady/incorrect especially when the exception
was a stack overflow error.

This codes job was to ensure that FP/SP are always at the frame
where we're exiting from. Most importantly, it is at the frame that's
catching the exception. This is needed because when we make the C call to
compile the OSR exit, we read CodeBlock, etc, from the ExecState* argument,
so we must make sure FP is at callFrameForCatch. In the case where an exception
is not thrown, we end up just moving SP back to where it already should have been.

> 
> But, I could be completely wrong about what this function actually does.  I
> don't think that its name says anything about why it exists, and it's not
> clear to me what invariant it ensures.