[JSC] exception from handleHostCall for tail-call should be handled correctly #22528
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Constellation
added
the
JavaScriptCore
|
EWS run on previous version of this PR (hash 3629fca) |
Constellation
force-pushed
the
eng/JSC-exception-from-handleHostCall-for-tail-call-should-be-handled-correctly
branch
from
3629fca
to
f757fe3
Compare
|
EWS run on previous version of this PR (hash f757fe3) |
MenloDorian
reviewed
Jan 9, 2024
| @@ -96,6 +96,13 @@ inline bool CallFrame::isStackOverflowFrame() const | |||
| return jsCallee() == jsCallee()->globalObject()->stackOverflowFrameCallee(); | |||
| } | |||
|
|
|||
| inline bool CallFrame::isHandleHostCallExceptionFrame() const | |||
There was a problem hiding this comment.
nit: based on this patch, I don't see a need to distinguish handleHostCallExceptionCallee from stackOverflowFrameCallee. We could just rename stackOverflowFrameCallee to partiallyInitializedFrameCallee and just use that instead. This way, we don't have to spend time, code, and memory instantiating and checking for a second instance of this callee per global.
MenloDorian
reviewed
Jan 9, 2024
| jit.copyCalleeSavesToEntryFrameCalleeSavesBuffer(vm.topEntryFrame, GPRInfo::argumentGPR0); | ||
| jit.setupArguments<decltype(operationLookupExceptionHandler)>(CCallHelpers::TrustedImmPtr(&vm)); | ||
| jit.prepareCallOperation(vm); | ||
| jit.move(CCallHelpers::TrustedImmPtr(tagCFunction<OperationPtrTag>(operationLookupExceptionHandler)), GPRInfo::nonArgGPR0); |
There was a problem hiding this comment.
If you convert isStackOverflowFrame to isPartiallyInitializedFrame (which is used both by you here and by stack overflow), you can use operationLookupExceptionHandlerFromCallerFrame here. It is semantically the same except for the extra ASSERTs. However, it communicates more clearly that we intend to throw from the caller, and that the current frame is a partially initialized frame.
There was a problem hiding this comment.
This function can be used for non partially initialized frame case (calling a function caused an error). So we should use operationLookupExceptionHandler
MenloDorian
approved these changes
Jan 9, 2024
Constellation
force-pushed
the
eng/JSC-exception-from-handleHostCall-for-tail-call-should-be-handled-correctly
branch
from
f757fe3
to
69e7f7c
Compare
|
EWS run on current version of this PR (hash 69e7f7c) |
🧪 style
|
EWS run on previous version of this PR (hash 69e7f7c) |
webkit-ews-buildbot
added
the
merging-blocked
Constellation
removed
the
merging-blocked
Constellation
force-pushed
the
eng/JSC-exception-from-handleHostCall-for-tail-call-should-be-handled-correctly
branch
from
69e7f7c
to
fd65834
Compare
|
EWS run on current version of this PR (hash fd65834) |
|
EWS run on previous version of this PR (hash fd65834) |
Constellation
force-pushed
the
eng/JSC-exception-from-handleHostCall-for-tail-call-should-be-handled-correctly
branch
from
fd65834
to
c3a7c4a
Compare
|
EWS run on previous version of this PR (hash c3a7c4a) |
webkit-ews-buildbot
added
the
merging-blocked
Constellation
removed
the
merging-blocked
Constellation
force-pushed
the
eng/JSC-exception-from-handleHostCall-for-tail-call-should-be-handled-correctly
branch
from
c3a7c4a
to
eb6ff6e
Compare
|
EWS run on previous version of this PR (hash eb6ff6e) |
webkit-ews-buildbot
added
the
merging-blocked
Constellation
removed
the
merging-blocked
Constellation
force-pushed
the
eng/JSC-exception-from-handleHostCall-for-tail-call-should-be-handled-correctly
branch
from
eb6ff6e
to
62231e0
Compare
|
EWS run on previous version of this PR (hash 62231e0) |
webkit-ews-buildbot
added
the
merging-blocked
Constellation
removed
the
merging-blocked
Constellation
force-pushed
the
eng/JSC-exception-from-handleHostCall-for-tail-call-should-be-handled-correctly
branch
from
62231e0
to
36f1eab
Compare
|
EWS run on previous version of this PR (hash 36f1eab) |
Constellation
force-pushed
the
eng/JSC-exception-from-handleHostCall-for-tail-call-should-be-handled-correctly
branch
from
36f1eab
to
16d6732
Compare
|
EWS run on previous version of this PR (hash 16d6732) |
Constellation
force-pushed
the
eng/JSC-exception-from-handleHostCall-for-tail-call-should-be-handled-correctly
branch
from
16d6732
to
49b120d
Compare
|
EWS run on current version of this PR (hash 49b120d) |
Constellation
added
the
unsafe-merge-queue
…orrectly https://bugs.webkit.org/show_bug.cgi?id=267249 rdar://120662635 Reviewed by Mark Lam. 272580@main introduced failing in the fast path in polymorphic thunk (calling operationLinkPolymorphicFromRegularCall etc.). In this case, 1. We should anyway use the top-most CallFrame* for NativeCallFrameTracer since it confuses StackVisitor (It assumes vm.topCallFrame is the top-most CallFrame*). We use calleeFrame instead of callerFrame. 2. Then, we should make StackVisitor work with CallFrame* which has non-cell JSCallee (when calling a non-function value). We rename stackOverflowFrameCallee to partiallyInitializedFrameCallee and use it. This tells StackVisitor that it should skip the first frame since it is pre-baked one. Also, make it possible to throw exception from this frame since exception catching code assumes that Callee is some cells. 3. To throw an exception from the current calleeFrame, this patch adds throwExceptionFromCallGenerator thunk, which throws an exception from the current frame when it is called as a normal JS function. * JSTests/stress/tail-call-callee-frame-polymorphic.js: Added. * Source/JavaScriptCore/bytecode/RepatchInlines.h: (JSC::handleHostCall): * Source/JavaScriptCore/interpreter/CallFrame.h: * Source/JavaScriptCore/interpreter/CallFrameInlines.h: (JSC::CallFrame::isHandleHostCallExceptionFrame const): * Source/JavaScriptCore/interpreter/FrameTracers.h: (JSC::NativeCallFrameTracerForTailCall::NativeCallFrameTracerForTailCall): Deleted. * Source/JavaScriptCore/interpreter/StackVisitor.cpp: (JSC::StackVisitor::StackVisitor): * Source/JavaScriptCore/jit/JITOperations.cpp: (JSC::JSC_DEFINE_JIT_OPERATION): * Source/JavaScriptCore/jit/JITThunks.h: * Source/JavaScriptCore/jit/ThunkGenerators.cpp: (JSC::throwExceptionFromCallGenerator): (JSC::polymorphicThunkFor): * Source/JavaScriptCore/jit/ThunkGenerators.h: * Source/JavaScriptCore/runtime/JSGlobalObject.cpp: (JSC::JSGlobalObject::init): (JSC::JSGlobalObject::visitChildrenImpl): * Source/JavaScriptCore/runtime/JSGlobalObject.h: (JSC::JSGlobalObject::handleHostCallExceptionCallee const): * Source/JavaScriptCore/runtime/VMInlines.h: (JSC::VM::topJSCallFrame const): Canonical link: https://commits.webkit.org/272816@main
webkit-commit-queue
force-pushed
the
eng/JSC-exception-from-handleHostCall-for-tail-call-should-be-handled-correctly
branch
from
49b120d
to
e94a54e
Compare
|
Committed 272816@main (e94a54e): https://commits.webkit.org/272816@main Reviewed commits have been landed. Closing PR #22528 and removing active labels. |
webkit-commit-queue
removed
the
unsafe-merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
e94a54e
49b120d
🧪 mac-wk2