[wasm][coreclr] Fix prestub of methods with IL helper stubs#124873
[wasm][coreclr] Fix prestub of methods with IL helper stubs#124873radekdoulik wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR fixes a WASM/CoreCLR interpreter prestub issue where methods implemented via managed IL helper stubs could end up publishing an entrypoint that maps to the helper MethodDesc, breaking MethodDesc lookups and access checks (manifesting as MethodAccessException, e.g. around delegate construction).
Changes:
- Update
MethodDesc::DoPrestub(FCall + managed-impl + portable entrypoints) to publish this method’s ownPortableEntryPointand attach interpreter data to it, instead of reusing the helper’s portable entrypoint. - Enable interpreter callsite access-check callouts on WASM by removing the
#ifndef TARGET_WASMguard.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/coreclr/vm/prestub.cpp |
Publishes the current method’s portable entrypoint and sets interpreter data for managed FCall helper-stub scenarios. |
src/coreclr/interpreter/compiler.cpp |
Enables emitting callsite access-check callouts on WASM by removing a WASM-only compile-time exclusion. |
| SetInterpreterCode((InterpByteCodeStart*)ilStubInterpData); | ||
|
|
||
| // Use this method's own PortableEntryPoint rather than the helper's. | ||
| // Setting pCode to the helper's PortableEntryPoint would write a foreign | ||
| // MethodDesc pointer into this method's slot, breaking MethodDesc lookups | ||
| // and access checks (e.g. private Delegate.DelegateConstruct exposed on | ||
| // delegate ctor slots causes MethodAccessException). | ||
| PCODE entryPoint = GetPortableEntryPoint(); | ||
| if (ilStubInterpData != NULL) | ||
| { | ||
| PortableEntryPoint::SetInterpreterData(entryPoint, (PCODE)(TADDR)ilStubInterpData); | ||
| } |
There was a problem hiding this comment.
In the managed-FCall + FEATURE_PORTABLE_ENTRYPOINTS path, pCode is now set to this method’s PortableEntryPoint unconditionally, but PortableEntryPoint::SetInterpreterData is only called when helperMD->GetInterpreterCode() is non-null. If the helper ends up with native/AOT code (so GetInterpreterCode() stays null), the returned entryPoint will have neither interpreter data nor actual code set, which can result in repeatedly re-entering prestub / inability to execute the method. Consider handling the non-interpreter case by setting this method’s PortableEntryPoint actual code to the helper’s actual code (e.g., if PortableEntryPoint::HasNativeEntryPoint(pCode) then SetActualCode(entryPoint, (PCODE)PortableEntryPoint::GetActualCode(pCode))), and/or asserting that one of interpreter/native is always available before publishing pCode.
| SetInterpreterCode((InterpByteCodeStart*)ilStubInterpData); | |
| // Use this method's own PortableEntryPoint rather than the helper's. | |
| // Setting pCode to the helper's PortableEntryPoint would write a foreign | |
| // MethodDesc pointer into this method's slot, breaking MethodDesc lookups | |
| // and access checks (e.g. private Delegate.DelegateConstruct exposed on | |
| // delegate ctor slots causes MethodAccessException). | |
| PCODE entryPoint = GetPortableEntryPoint(); | |
| if (ilStubInterpData != NULL) | |
| { | |
| PortableEntryPoint::SetInterpreterData(entryPoint, (PCODE)(TADDR)ilStubInterpData); | |
| } | |
| if (ilStubInterpData != NULL) | |
| { | |
| SetInterpreterCode((InterpByteCodeStart*)ilStubInterpData); | |
| } | |
| // Use this method's own PortableEntryPoint rather than the helper's. | |
| // Setting pCode to the helper's PortableEntryPoint would write a foreign | |
| // MethodDesc pointer into this method's slot, breaking MethodDesc lookups | |
| // and access checks (e.g. private Delegate.DelegateConstruct exposed on | |
| // delegate ctor slots causes MethodAccessException). | |
| PCODE helperEntryPoint = pCode; | |
| PCODE entryPoint = GetPortableEntryPoint(); | |
| if (ilStubInterpData != NULL) | |
| { | |
| PortableEntryPoint::SetInterpreterData(entryPoint, (PCODE)(TADDR)ilStubInterpData); | |
| } | |
| else if (PortableEntryPoint::HasNativeEntryPoint(helperEntryPoint)) | |
| { | |
| PortableEntryPoint::SetActualCode(entryPoint, (PCODE)PortableEntryPoint::GetActualCode(helperEntryPoint)); | |
| } | |
| else | |
| { | |
| // Managed FCall helpers must have either interpreter or native entrypoint | |
| _ASSERTE(!"Managed FCall helper has neither interpreter nor native entrypoint"); | |
| } |
There was a problem hiding this comment.
it should have IL stub, so it should not be native code entrypoint.
we should handle other scenario though, once #124868 lands. to handle case where the helper method's portable entrypoint gets reset (for example by tiering once we have codegen) and force prestub on it again to force recompilation.
There was a problem hiding this comment.
I will let the CI run finish to make sure it passes. then I should add an assert at least or handle the case described above
Fix #121955