Skip to content

Do not use MarkPrecodeAsStableEntrypoint with portable entrypoints #120467

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Do not use MarkPrecodeAsStableEntrypoint with portable entrypoints #120467

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/coreclr/vm/method.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2711,12 +2711,14 @@ void MethodDesc::SetTemporaryEntryPoint(AllocMemTracker *pamTracker)
_ASSERTE(*pSlot != (PCODE)NULL);
#endif

#ifndef FEATURE_PORTABLE_ENTRYPOINTS
if (RequiresStableEntryPoint())
{
// The rest of the system assumes that certain methods always have stable entrypoints.
// Mark the precode as such
MarkPrecodeAsStableEntrypoint();
Copy link
Member

Choose a reason for hiding this comment

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

This is needed. See the implementation in MarkPrecodeAsStableEntrypoint().

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem related to the referenced issue, #120319, that states Precode::GetPrecodeFromEntryPoint(). We should be examining the callsite of Precode::GetPrecodeFromEntryPoint() and understand its intent.

Copy link
Member

@jkotas jkotas Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with Aaron that we should be cleaning this up more aggressively so that we do not have to be finding and fixing Precode paths one at a time.

For example, can we place all occurrences of HasPrecode/GetPrecode/enum_flag3_HasPrecode under #ifndef FEATURE_PORTABLE_ENTRYPOINTS? If we are not ready this yet, change HasPrecode to return unconditional false and ifdef out enum_flag3_HasPrecode flag at least?

Copy link
Member Author

@radekdoulik radekdoulik Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is needed. See the implementation in MarkPrecodeAsStableEntrypoint().

Could you give me more context here? Besides asserts, it sets the HasStableEntryPoint and HasPrecode flags. My assumption was that we don't want any of these flags set as the portable entrypoints are stable once we pass DoPrestub(...) and we don't have real precode, so we don't want to have that flag set either.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't seem related to the referenced issue, #120319, that states Precode::GetPrecodeFromEntryPoint(). We should be examining the callsite of Precode::GetPrecodeFromEntryPoint() and understand its intent.

What is happening here is that we set HasStableEntrypoint and HasPrecode in MethodDesc::MarkPrecodeAsStableEntrypoint(). Later we reach the assert in Precode::GetPrecodeFromEntryPoint() when MethodDesc::IsPointingToPrestub() checks these flags and because they are set and it calls GetPrecode()->IsPointingToPrestub(). The end of stacktrace looks like this:

Precode::GetPrecodeFromEntryPoint(unsigned long, int) (/Users/rodo/git/runtime-main/src/coreclr/vm/precode_portable.cpp:170)
MethodDesc::GetPrecode() (/Users/rodo/git/runtime-main/src/coreclr/vm/method.hpp:387)
MethodDesc::IsPointingToPrestub() (/Users/rodo/git/runtime-main/src/coreclr/vm/method.cpp:2395)
CallPreStub(MethodDesc*) (/Users/rodo/git/runtime-main/src/coreclr/vm/interpexec.cpp:684)
InterpExecMethod(InterpreterFrame*, InterpMethodContextFrame*, InterpThreadContext*, ExceptionClauseArgs*) 
...

It makes the failing test from #120319 pass.

Copy link
Member

Choose a reason for hiding this comment

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

@radekdoulik I see what you're saying here. This is part of the work that didn't get completed during the initial work I was doing. If you look at precode_portable.hpp, there are many dummy implemntations for the Precode infrastructure. We should work on removing all of those and defining them away. I will work on this tomorrow too.

Copy link
Member Author

Choose a reason for hiding this comment

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

@AaronRobinsonMSFT This sounds great. Do you want me to make more changes here, so we can merge it or do you want to make it part of your changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

How many GetOrCreatePrecode calls are left in FEATURE_PORTABLE_ENTRYPOINTS build? Is it hard to ifdef them?

There are still few places, when I ifdef it out, I am getting

wasm-ld: error: ../../dlls/mscoree/coreclr/libcoreclr_static.a(jitinterface.cpp.o): undefined symbol: MethodDesc::GetOrCreatePrecode()
wasm-ld: error: ../../dlls/mscoree/coreclr/libcoreclr_static.a(jitinterface.cpp.o): undefined symbol: MethodDesc::GetOrCreatePrecode()
wasm-ld: error: ../../dlls/mscoree/coreclr/libcoreclr_static.a(method.cpp.o): undefined symbol: MethodDesc::GetOrCreatePrecode()
wasm-ld: error: ../../dlls/mscoree/coreclr/libcoreclr_static.a(prestub.cpp.o): undefined symbol: MethodDesc::GetOrCreatePrecode()
wasm-ld: error: ../../dlls/mscoree/coreclr/libcoreclr_static.a(dllimport.cpp.o): undefined symbol: MethodDesc::GetOrCreatePrecode()

Copy link
Member

Choose a reason for hiding this comment

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

@AaronRobinsonMSFT This sounds great. Do you want me to make more changes here, so we can merge it or do you want to make it part of your changes?

I would close this PR. There is a non-trivial amount of clean-up here and I've already got a head start on some of it. Thanks for pushing on the issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, thanks for looking at it.

}
#endif // FEATURE_PORTABLE_ENTRYPOINTS
}

void MethodDesc::EnsureTemporaryEntryPoint()
Expand Down
Loading