Do not use MarkPrecodeAsStableEntrypoint with portable entrypoints #120467
Conversation
There was a problem hiding this comment.
Pull Request Overview
Fixes an issue where MarkPrecodeAsStableEntrypoint was being called on portable entrypoints, which is not appropriate behavior. The fix adds conditional compilation to ensure this method is only called when FEATURE_PORTABLE_ENTRYPOINTS is not defined.
- Added
#ifndef FEATURE_PORTABLE_ENTRYPOINTSguard aroundMarkPrecodeAsStableEntrypointcall
|
Tagging subscribers to this area: @mangod9 |
|
We were reaching that for instantiating stubs. I assume we don't want to call @AaronRobinsonMSFT @jkotas Are there any other cases which might be relevant? I think if we reach a path, where we don't have IL, we will fail later when trying to call the method and it will be more clear. |
| { | ||
| // The rest of the system assumes that certain methods always have stable entrypoints. | ||
| // Mark the precode as such | ||
| MarkPrecodeAsStableEntrypoint(); |
There was a problem hiding this comment.
This is needed. See the implementation in MarkPrecodeAsStableEntrypoint().
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This doesn't seem related to the referenced issue, #120319, that states
Precode::GetPrecodeFromEntryPoint(). We should be examining the callsite ofPrecode::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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
How many
GetOrCreatePrecodecalls are left inFEATURE_PORTABLE_ENTRYPOINTSbuild? 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()
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Great, thanks for looking at it.
3 participants
Fixes #120319