Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exceptions can't always be checked under silent spill in DFG #28709

Conversation

kmiller68
Copy link
Contributor

@kmiller68 kmiller68 commented May 17, 2024

d1282e0

exceptions can't always be checked under silent spill in DFG
https://bugs.webkit.org/show_bug.cgi?id=274291
rdar://128067350

Reviewed by Yusuke Suzuki.

If we're catching an exception in the same DFG frame it's potentially
not safe to check for exceptions under a silent spill. This is because
the OSR exit ramp does not know about the silent spill. So values will
not be restored. There were a couple of possible fixes:

1) teach the DFGVariableEventStream about exceptions under silent spill.
2) add extra metadata about the fact we’re under a silent spill and silent
   fill before hitting the OSR exit ramp.
3) move the exception to an unused gpr until we can silent fill if needed.

I went with option 3. 1. has the problem that it's complicated and might
be a memory regression. 2. could bloat code size.

I also noticed that my `requires (!OperationHasResult<T>)` checks were not
properly eliminating overloads. This is because when you do e.g.
`requires (!OperationHasResult<int>)` the `OperationHasResult<int>` will
fail SFINAE but that just makes the concept false which then becomes true
in the requirement. Instead we now have a new `OperationIsVoid<T>` concept.

* JSTests/stress/stack-overflow-in-scope-with-catch.js: Added.
(foo.catch.Set.Symbol.hasInstance):
(foo.finally.bar):
(foo.goo.baz):
(foo.goo):
(foo):
* Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h:
* Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h:
* Source/JavaScriptCore/dfg/DFGCallCreateDirectArgumentsSlowPathGenerator.h:
* Source/JavaScriptCore/dfg/DFGSaneStringGetByValSlowPathGenerator.h:
* Source/JavaScriptCore/dfg/DFGSilentRegisterSavePlan.h:
(JSC::DFG::SilentRegisterSavePlan::SilentRegisterSavePlan):
(JSC::DFG::SilentRegisterSavePlan::reg const):
(JSC::DFG::SilentRegisterSavePlan::gpr const):
(JSC::DFG::SilentRegisterSavePlan::fpr const):
* Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:
(JSC::DFG::CallSlowPathGenerator::setUp):
(JSC::DFG::CallSlowPathGenerator::tearDown):
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::exceptionCheck):
(JSC::DFG::SpeculativeJIT::silentSpillImpl):
(JSC::DFG::SpeculativeJIT::silentFillImpl):
(JSC::DFG::SpeculativeJIT::compileToLowerCase):
(JSC::DFG::SpeculativeJIT::silentSpill): Deleted.
(JSC::DFG::SpeculativeJIT::silentFill): Deleted.
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:
(JSC::DFG::SpeculativeJIT::spillPlanInterferesWithReg):
(JSC::DFG::SpeculativeJIT::silentSpill):
(JSC::DFG::SpeculativeJIT::silentFill):
(JSC::DFG::SpeculativeJIT::silentSpillAllRegistersImpl):
(JSC::DFG::SpeculativeJIT::silentFillAllRegisters):
(JSC::DFG::SpeculativeJIT::operationExceptionCheck):
(JSC::DFG::SpeculativeJIT::callOperation):
(JSC::DFG::SpeculativeJIT::tryHandleOrGetExceptionUnderSilentSpill):
(JSC::DFG::SpeculativeJIT::callOperationWithSilentSpill):
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeStrictEq):
(JSC::DFG::SpeculativeJIT::genericJSValueNonPeepholeStrictEq):
(JSC::DFG::SpeculativeJIT::emitCall):
(JSC::DFG::SpeculativeJIT::compileGetByVal):
(JSC::DFG::SpeculativeJIT::compile):
* Source/JavaScriptCore/jit/GPRInfo.h:
(JSC::NoOverlapImpl::noOverlapImpl):
* Source/JavaScriptCore/jit/OperationResult.h:
* Source/JavaScriptCore/jit/Reg.h:

Canonical link: https://commits.webkit.org/279031@main

c50b46c

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 🧪 gtk-wk2
🛠 tv-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge 🛠 watch ✅ 🛠 jsc-armv7
✅ 🛠 watch-sim ❌ 🧪 jsc-armv7-tests

@kmiller68 kmiller68 requested review from a team and JonWBedard as code owners May 17, 2024 15:14
@kmiller68 kmiller68 self-assigned this May 17, 2024
@kmiller68 kmiller68 added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label May 17, 2024
@kmiller68 kmiller68 force-pushed the eng/exceptions-cant-always-be-checked-under-silent-spill-in-DFG branch from e26f77b to 69bd390 Compare May 17, 2024 15:22
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@kmiller68 kmiller68 force-pushed the eng/exceptions-cant-always-be-checked-under-silent-spill-in-DFG branch from 69bd390 to c50b46c Compare May 20, 2024 23:25
@kmiller68 kmiller68 added the merge-queue Applied to send a pull request to merge-queue label May 20, 2024
@webkit-commit-queue webkit-commit-queue force-pushed the eng/exceptions-cant-always-be-checked-under-silent-spill-in-DFG branch from c50b46c to caea0dc Compare May 21, 2024 02:11
https://bugs.webkit.org/show_bug.cgi?id=274291
rdar://128067350

Reviewed by Yusuke Suzuki.

If we're catching an exception in the same DFG frame it's potentially
not safe to check for exceptions under a silent spill. This is because
the OSR exit ramp does not know about the silent spill. So values will
not be restored. There were a couple of possible fixes:

1) teach the DFGVariableEventStream about exceptions under silent spill.
2) add extra metadata about the fact we’re under a silent spill and silent
   fill before hitting the OSR exit ramp.
3) move the exception to an unused gpr until we can silent fill if needed.

I went with option 3. 1. has the problem that it's complicated and might
be a memory regression. 2. could bloat code size.

I also noticed that my `requires (!OperationHasResult<T>)` checks were not
properly eliminating overloads. This is because when you do e.g.
`requires (!OperationHasResult<int>)` the `OperationHasResult<int>` will
fail SFINAE but that just makes the concept false which then becomes true
in the requirement. Instead we now have a new `OperationIsVoid<T>` concept.

* JSTests/stress/stack-overflow-in-scope-with-catch.js: Added.
(foo.catch.Set.Symbol.hasInstance):
(foo.finally.bar):
(foo.goo.baz):
(foo.goo):
(foo):
* Source/JavaScriptCore/dfg/DFGArrayifySlowPathGenerator.h:
* Source/JavaScriptCore/dfg/DFGCallArrayAllocatorSlowPathGenerator.h:
* Source/JavaScriptCore/dfg/DFGCallCreateDirectArgumentsSlowPathGenerator.h:
* Source/JavaScriptCore/dfg/DFGSaneStringGetByValSlowPathGenerator.h:
* Source/JavaScriptCore/dfg/DFGSilentRegisterSavePlan.h:
(JSC::DFG::SilentRegisterSavePlan::SilentRegisterSavePlan):
(JSC::DFG::SilentRegisterSavePlan::reg const):
(JSC::DFG::SilentRegisterSavePlan::gpr const):
(JSC::DFG::SilentRegisterSavePlan::fpr const):
* Source/JavaScriptCore/dfg/DFGSlowPathGenerator.h:
(JSC::DFG::CallSlowPathGenerator::setUp):
(JSC::DFG::CallSlowPathGenerator::tearDown):
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::exceptionCheck):
(JSC::DFG::SpeculativeJIT::silentSpillImpl):
(JSC::DFG::SpeculativeJIT::silentFillImpl):
(JSC::DFG::SpeculativeJIT::compileToLowerCase):
(JSC::DFG::SpeculativeJIT::silentSpill): Deleted.
(JSC::DFG::SpeculativeJIT::silentFill): Deleted.
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:
(JSC::DFG::SpeculativeJIT::spillPlanInterferesWithReg):
(JSC::DFG::SpeculativeJIT::silentSpill):
(JSC::DFG::SpeculativeJIT::silentFill):
(JSC::DFG::SpeculativeJIT::silentSpillAllRegistersImpl):
(JSC::DFG::SpeculativeJIT::silentFillAllRegisters):
(JSC::DFG::SpeculativeJIT::operationExceptionCheck):
(JSC::DFG::SpeculativeJIT::callOperation):
(JSC::DFG::SpeculativeJIT::tryHandleOrGetExceptionUnderSilentSpill):
(JSC::DFG::SpeculativeJIT::callOperationWithSilentSpill):
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeStrictEq):
(JSC::DFG::SpeculativeJIT::genericJSValueNonPeepholeStrictEq):
(JSC::DFG::SpeculativeJIT::emitCall):
(JSC::DFG::SpeculativeJIT::compileGetByVal):
(JSC::DFG::SpeculativeJIT::compile):
* Source/JavaScriptCore/jit/GPRInfo.h:
(JSC::NoOverlapImpl::noOverlapImpl):
* Source/JavaScriptCore/jit/OperationResult.h:
* Source/JavaScriptCore/jit/Reg.h:

Canonical link: https://commits.webkit.org/279031@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/exceptions-cant-always-be-checked-under-silent-spill-in-DFG branch from caea0dc to d1282e0 Compare May 21, 2024 02:13
@webkit-commit-queue
Copy link
Collaborator

Committed 279031@main (d1282e0): https://commits.webkit.org/279031@main

Reviewed commits have been landed. Closing PR #28709 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit d1282e0 into WebKit:main May 21, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
4 participants