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

Cleanup unused JIT stubs in vm #111237

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

huoyaoyuan
Copy link
Member

The JIT_TailCall helpers are only used on Windows x86. Removing all stub definitions on other platforms.
GetCONTEXTFromRedirectedStubStackFrame(DISPATCH_CONTEXT) is only used on Windows.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 9, 2025
# <NOTE>
#
# </NOTE>
LEAF_ENTRY JIT_ProfilerEnterLeaveTailcallStub, _TEXT
Copy link
Member Author

Choose a reason for hiding this comment

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

UNIX_AMD64 was the only platform not implementing this in ASM.
What's the requirement for this method? Can it be implemented in C for all platforms?

Copy link
Member

Choose a reason for hiding this comment

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

This method has custom calling convention - it has to preserve more register than the default calling convention is guaranteed to preserve. It cannot be implemented in C in general.

I think we just got lucky on UNIX_AMD64 or this profiling scenario is not tested on Unix Amd64.

@@ -89,9 +89,9 @@ EXTERN_C LPVOID STDCALL COMPlusEndCatch(LPVOID ebp, DWORD ebx, DWORD edi, DWORD
// RedirectedHandledJITCaseForXXX_Stub's.
//
PTR_CONTEXT GetCONTEXTFromRedirectedStubStackFrame(CONTEXT * pContext);
#ifdef FEATURE_EH_FUNCLETS
#if defined(FEATURE_EH_FUNCLETS) && !defined(TARGET_UNIX)
Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be possible to port Windows x86 to new EH in the future?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is something we have in our backlog

Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to delete this since there is no implementation.

This method has to more to do with low-level implementation of EH and thread suspension than funclets. If I am reading the code correctly, this method is only called from FixRedirectContextHandler that is only used on Windows x64. It can be probably deleted everywhere else as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be probably deleted everywhere else as well.

I do plan to delete it on UNIX. Including in this PR should be fine.
For Windows x86, leaving a stub may be useful for future implementation. Deleting it should also be fine because there should be more missing definitions for new EH on Windows x86.

Copy link
Member

Choose a reason for hiding this comment

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

I do not think it will be needed for new EH on Windows x86


void STDMETHODCALLTYPE JIT_ProfilerEnterLeaveTailcallStub(UINT_PTR ProfilerHandle);
#if !defined(TARGET_ARM64) && !defined(TARGET_LOONGARCH64) && !defined(TARGET_RISCV64)
void STDCALL JIT_StackProbe();
#endif // TARGET_ARM64
#endif // !defined(TARGET_ARM64) && !defined(TARGET_LOONGARCH64) && !defined(TARGET_RISCV64)
Copy link
Member Author

Choose a reason for hiding this comment

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

Should the condition here be inverted? Is the function meaningful for RISC platforms?

Copy link
Member

@jkotas jkotas Jan 9, 2025

Choose a reason for hiding this comment

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

Is the function meaningful for RISC platforms?

I think that it is. You can link it to #13519

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants