[wasm][coreclr] Move DoPrestub call out of CallDescrWorkerInternal#124788
[wasm][coreclr] Move DoPrestub call out of CallDescrWorkerInternal#124788radekdoulik wants to merge 1 commit intodotnet:mainfrom
Conversation
To call it before we potentially put object references on stack, because DoPrestub can trigger GC.
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical GC safety issue in WASM's CoreCLR implementation by moving the DoPrestub call out of CallDescrWorkerInternal to prevent it from triggering a GC after object references have been placed on the stack. The fix introduces a new helper method GetInterpreterCodeWithPrestub() and ensures it is called at all entry points before arguments are marshaled.
Changes:
- Removed the DoPrestub logic from CallDescrWorkerInternal, eliminating the GC safety hazard
- Added GetInterpreterCodeWithPrestub() helper method to centralize prestub logic with GC safety
- Updated all call sites to perform prestub before marshaling arguments onto the stack
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/vm/wasm/calldescrworkerwasm.cpp | Removed DoPrestub call and added assertion that interpreter code is already initialized |
| src/coreclr/vm/method.hpp | Added GetInterpreterCodeWithPrestub() method that calls DoPrestub in preemptive GC mode before arguments are on stack |
| src/coreclr/vm/reflectioninvocation.cpp | Moved pTarget calculation and GetInterpreterCodeWithPrestub call before argument marshaling |
| src/coreclr/vm/callhelpers.cpp | Added GetInterpreterCodeWithPrestub calls to DispatchCallSimple and MethodDescCallSite::CallTargetWorker before arguments are processed |
| CONTRACTL_END; | ||
|
|
||
| #ifdef FEATURE_PORTABLE_ENTRYPOINTS | ||
| MethodDesc* pMethod = PortableEntryPoint::GetMethodDesc(pTargetAddress); |
There was a problem hiding this comment.
There are unprotected object references in SIZE_T *pSrc at this point. This needs to happen before the pSrc array gets populated.
There was a problem hiding this comment.
That's bad, my understanding was that the problem starts when we copy them here around to new location and that the references in pSrc are protected. I will try to see how many places we would need to update if we would need to move up the call stack.
| CONTRACTL_END; | ||
|
|
||
| #ifdef FEATURE_PORTABLE_ENTRYPOINTS | ||
| MethodDesc* pMethod = PortableEntryPoint::GetMethodDesc(m_pCallTarget); |
There was a problem hiding this comment.
Same here. There are unprotected object references in ARG_SLOT *pArguments, array at this point. It needs to happen before pArguments array gets populated.
| callDescrData.hasRetBuff = argit.HasRetBuffArg(); | ||
| #endif // TARGET_WASM | ||
|
|
||
| // This is duplicated logic from MethodDesc::GetCallTarget |
There was a problem hiding this comment.
It should not be necessary to move this. There are no unprotected object references on the stack at this point. The unprotected references on the stack are created only after "NO GC AFTER THIS POINT." comment below.
To call it before we potentially put object references on stack, because DoPrestub can trigger GC.