-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use SystemV ABI for C++ entrypoints for JS LLInt #28723
Use SystemV ABI for C++ entrypoints for JS LLInt #28723
Conversation
EWS run on previous version of this PR (hash 1c534fa) |
1c534fa
to
8036b04
Compare
EWS run on previous version of this PR (hash 8036b04) |
8036b04
to
3aee41b
Compare
EWS run on current version of this PR (hash 3aee41b) |
There was a problem hiding this 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.
@@ -1982,14 +1946,10 @@ else | |||
# The PC base is in t3, as this is what _llint_entry leaves behind through | |||
# initPCRelative(t3) | |||
macro setEntryAddressCommon(kind, index, label, map) | |||
if X86_64 | |||
if X86_64 or X86_64_WIN |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3aee41b
to
4e564b7
Compare
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
4e564b7
to
8f1711c
Compare
Committed 278967@main (8f1711c): https://commits.webkit.org/278967@main Reviewed commits have been landed. Closing PR #28723 and removing active labels. |
8f1711c
3aee41b