Skip to content

Use SystemV ABI for C++ entrypoints for JS LLInt#28723

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
iangrunert:ig/sysvabi-js-llint
May 19, 2024
Merged

Use SystemV ABI for C++ entrypoints for JS LLInt#28723
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
iangrunert:ig/sysvabi-js-llint

Conversation

@iangrunert
Copy link
Contributor

@iangrunert iangrunert commented May 17, 2024 •
edited by webkit-early-warning-system
Loading

8f1711c

Use SystemV ABI for C++ entrypoints for JS LLInt
https://bugs.webkit.org/show_bug.cgi?id=274064

Reviewed by Yusuke Suzuki.

Switched register mapping on Windows to match the other x86-64 platforms
Added SystemV ABI function annotation to C++ entrypoints for JS LLInt
Disabed WebAssembly LLInt, as it doesn't work without JIT anyway, so we
can review the necessary changes there in another pull request.

* Source/JavaScriptCore/assembler/MaxFrameExtentForSlowPathCall.h:
* Source/JavaScriptCore/assembler/X86_64Registers.h:
* Source/JavaScriptCore/heap/MachineStackMarker.cpp:
(JSC::osRedZoneAdjustment):
* Source/JavaScriptCore/interpreter/VMEntryRecord.h:
* Source/JavaScriptCore/jit/AssemblyHelpers.h:
(JSC::AssemblyHelpers::selectScratchGPR):
* Source/JavaScriptCore/jit/GPRInfo.h:
(JSC::GPRInfo::toRegister):
(JSC::GPRInfo::toArgumentRegister):
(JSC::GPRInfo::toIndex):
(JSC::PreferredArgumentImpl::preferredArgumentJSR):
* Source/JavaScriptCore/jit/RegisterSet.cpp:
(JSC::RegisterSetBuilder::vmCalleeSaveRegisters):
(JSC::RegisterSetBuilder::llintBaselineCalleeSaveRegisters):
(JSC::RegisterSetBuilder::dfgCalleeSaveRegisters):
* Source/JavaScriptCore/llint/LLIntData.cpp:
* Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:
(JSC::LLInt::llint_trace_operand):
(JSC::LLInt::llint_trace_value):
(JSC::LLInt::llint_default_call):
(JSC::LLInt::llint_virtual_call):
(JSC::LLInt::llint_slow_path_checkpoint_osr_exit_from_inlined_call):
(JSC::LLInt::llint_slow_path_checkpoint_osr_exit):
(JSC::LLInt::llint_throw_stack_overflow_error):
(JSC::LLInt::llint_stack_check_at_vm_entry):
(JSC::LLInt::llint_check_vm_entry_permission):
(JSC::LLInt::llint_dump_value):
(JSC::LLInt::llint_crash):
* Source/JavaScriptCore/llint/LLIntSlowPaths.h:
* Source/JavaScriptCore/llint/LLIntThunks.h:
* Source/JavaScriptCore/llint/LowLevelInterpreter.asm:
* Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:
* Source/JavaScriptCore/offlineasm/x86.rb:
* Source/JavaScriptCore/runtime/MachineContext.h:
(JSC::MachineContext::llintInstructionPointer):
* Source/JavaScriptCore/runtime/PutPropertySlot.h:
* Source/JavaScriptCore/runtime/VM.h:
* Source/WTF/wtf/CodePtr.h:
(WTF::CodePtr::CodePtr):
* Source/WTF/wtf/FunctionPtr.h:
* Source/WTF/wtf/PlatformCallingConventions.h:

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

3aee41b

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
✅ 🛠 watch ✅ 🛠 jsc-armv7
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 jsc-armv7-tests

@iangrunert iangrunert requested a review from a team as a code owner May 17, 2024 19:12
@iangrunert iangrunert added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label May 17, 2024
@iangrunert iangrunert self-assigned this May 17, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 17, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 1c534fa)

Details

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 17, 2024
@iangrunert iangrunert force-pushed the ig/sysvabi-js-llint branch from 1c534fa to 8036b04 Compare May 17, 2024 19:39
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 17, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 8036b04)

Details

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

@iangrunert iangrunert force-pushed the ig/sysvabi-js-llint branch from 8036b04 to 3aee41b Compare May 17, 2024 21:09
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 17, 2024 •
edited
Loading

EWS run on current version of this PR (hash 3aee41b)

Details

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
✅ 🛠 watch ✅ 🛠 jsc-armv7
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ✅ 🧪 jsc-armv7-tests

Constellation
Constellation approved these changes May 18, 2024
View reviewed changes
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 with one comment.

Source/JavaScriptCore/llint/LowLevelInterpreter.asm Outdated
Copy link
Member

@Constellation Constellation May 18, 2024

Choose a reason for hiding this comment

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

How about X86_64_WIN definition in LLInt completely and unify both to X86_64?

Copy link
Contributor

@kmiller68 kmiller68 May 18, 2024

Choose a reason for hiding this comment

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

Agreed. If we're going to have the same ABI on Windows we shouldn't bother with a different setting.

Copy link
Contributor Author

@iangrunert iangrunert May 18, 2024

Choose a reason for hiding this comment

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

Yep I agree with that goal, but I think we're a couple of steps away from it.

There is unfortunately still one place in LowLevelInterpreter.asm (line 329) and one in LowLevelInterpreter64.asm (line 696) where we do something different on X86_64_WIN, I think both are due to UNIFIED_AND_FREEZABLE_CONFIG_RECORD. There might also be some in WebAssembly LLInt, so I'd like to get WebAssembly LLInt re-enabled with SystemV first to get the full scope of how X86_64_WIN is used.

If the differences are all tied to UNIFIED_AND_FREEZABLE_CONFIG_RECORD, we can switch those places to use that setting directly and get rid of X86_64_WIN. Also we potentially might be able to get rid of UNIFIED_AND_FREEZABLE_CONFIG_RECORD all together - I think it might be feasible now #28538 has landed, but I haven't given it a shot yet.

Copy link
Member

@Constellation Constellation May 19, 2024

Choose a reason for hiding this comment

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

OK, let's go with this for now.

@Constellation Constellation added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels May 19, 2024
@iangrunert @Constellation
Use SystemV ABI for C++ entrypoints for JS LLInt
8f1711c 
https://bugs.webkit.org/show_bug.cgi?id=274064

Reviewed by Yusuke Suzuki.

Switched register mapping on Windows to match the other x86-64 platforms
Added SystemV ABI function annotation to C++ entrypoints for JS LLInt
Disabed WebAssembly LLInt, as it doesn't work without JIT anyway, so we
can review the necessary changes there in another pull request.

* Source/JavaScriptCore/assembler/MaxFrameExtentForSlowPathCall.h:
* Source/JavaScriptCore/assembler/X86_64Registers.h:
* Source/JavaScriptCore/heap/MachineStackMarker.cpp:
(JSC::osRedZoneAdjustment):
* Source/JavaScriptCore/interpreter/VMEntryRecord.h:
* Source/JavaScriptCore/jit/AssemblyHelpers.h:
(JSC::AssemblyHelpers::selectScratchGPR):
* Source/JavaScriptCore/jit/GPRInfo.h:
(JSC::GPRInfo::toRegister):
(JSC::GPRInfo::toArgumentRegister):
(JSC::GPRInfo::toIndex):
(JSC::PreferredArgumentImpl::preferredArgumentJSR):
* Source/JavaScriptCore/jit/RegisterSet.cpp:
(JSC::RegisterSetBuilder::vmCalleeSaveRegisters):
(JSC::RegisterSetBuilder::llintBaselineCalleeSaveRegisters):
(JSC::RegisterSetBuilder::dfgCalleeSaveRegisters):
* Source/JavaScriptCore/llint/LLIntData.cpp:
* Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:
(JSC::LLInt::llint_trace_operand):
(JSC::LLInt::llint_trace_value):
(JSC::LLInt::llint_default_call):
(JSC::LLInt::llint_virtual_call):
(JSC::LLInt::llint_slow_path_checkpoint_osr_exit_from_inlined_call):
(JSC::LLInt::llint_slow_path_checkpoint_osr_exit):
(JSC::LLInt::llint_throw_stack_overflow_error):
(JSC::LLInt::llint_stack_check_at_vm_entry):
(JSC::LLInt::llint_check_vm_entry_permission):
(JSC::LLInt::llint_dump_value):
(JSC::LLInt::llint_crash):
* Source/JavaScriptCore/llint/LLIntSlowPaths.h:
* Source/JavaScriptCore/llint/LLIntThunks.h:
* Source/JavaScriptCore/llint/LowLevelInterpreter.asm:
* Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:
* Source/JavaScriptCore/offlineasm/x86.rb:
* Source/JavaScriptCore/runtime/MachineContext.h:
(JSC::MachineContext::llintInstructionPointer):
* Source/JavaScriptCore/runtime/PutPropertySlot.h:
* Source/JavaScriptCore/runtime/VM.h:
* Source/WTF/wtf/CodePtr.h:
(WTF::CodePtr::CodePtr):
* Source/WTF/wtf/FunctionPtr.h:
* Source/WTF/wtf/PlatformCallingConventions.h:

Canonical link: https://commits.webkit.org/278967@main
@webkit-commit-queue
Copy link
Collaborator

webkit-commit-queue commented May 19, 2024

Committed 278967@main (8f1711c): https://commits.webkit.org/278967@main

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

@webkit-commit-queue webkit-commit-queue merged commit 8f1711c into WebKit:main May 19, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 19, 2024
@Jarred-Sumner Jarred-Sumner mentioned this pull request Jun 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Constellation Constellation Constellation approved these changes

@kmiller68 kmiller68 kmiller68 left review comments

Assignees

@iangrunert iangrunert

Labels

JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@iangrunert @webkit-early-warning-system @webkit-commit-queue @Constellation @kmiller68 @webkit-ews-buildbot