| Summary: | 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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||
| Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, commit-queue, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, ysuzuki | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Saam Barati
2015-11-18 14:03:57 PST
Created attachment 266320 [details]
patch
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. 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? (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. 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. 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.
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.
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. (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. 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. landed in: http://trac.webkit.org/changeset/193485 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? (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. (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. |