Fix P/Invoke IL stub race in DoPrestub#124579
Fix P/Invoke IL stub race in DoPrestub#124579AaronRobinsonMSFT wants to merge 7 commits intodotnet:mainfrom
Conversation
Re-enable ILStubCache lookup/insert for forward P/Invoke stubs to prevent racing threads from independently creating and JIT-ing duplicate IL stubs for the same target method. PR dotnet#117901 made IsSharedStubScenario return false for forward P/Invokes, which disabled the cache. Multiple threads could then each create their own DynamicMethodDesc and JIT it, producing different PCODE values that violated the assert in DoPrestub. The fix: - Remove the forward P/Invoke exclusion from IsSharedStubScenario so these stubs use the ILStubCache for de-duplication. - In CreateHashBlob, create a minimal hash blob containing just the target MethodDesc pointer for forward P/Invoke stubs, so different P/Invoke methods get distinct cache entries while racing threads for the same method converge on the same DynamicMethodDesc. Fixes dotnet#124530 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/interop-contrib |
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in P/Invoke IL stub generation that was introduced by PR #117901. The root cause was that forward P/Invoke stubs (non-CALLI, non-vararg) were excluded from IsSharedStubScenario, disabling the ILStubCache lookup/insert mechanism. This allowed multiple threads to independently create and JIT duplicate IL stubs for the same target method, resulting in different PCODE values and violating the assert in DoPrestub.
Changes:
- Re-enables
ILStubCacheusage for forward P/Invoke stubs by removing their exclusion fromIsSharedStubScenario - Creates minimal hash blobs keyed by target
MethodDescpointer for forward P/Invoke stubs to ensure proper cache de-duplication - Updates comments to reflect the new caching behavior
|
Could we store the |
I think we could, but I really dislike the idea of having multiple places for this sort of data. Instead of hiding it on the |
|
Yeah this can wait until we do a greater refactoring of the MethodDesc hierarchy if that's the preference. |
There should not be a DynamicMethodDesc for these in the first place. It can be all transient IL. |
… clarifying comments on thread safety during JIT operations
4 participants
Summary
Re-enable ILStubCache lookup/insert for forward P/Invoke stubs to prevent racing threads from independently creating and JIT-ing duplicate IL stubs for the same target method.
Root Cause
PR #117901 made
IsSharedStubScenarioreturnfalsefor forward P/Invokes, which disabled theILStubCachelookup. Multiple threads could then each create their ownDynamicMethodDescand JIT it, producing differentPCODEvalues that violated the assert inDoPrestub.Fix
IsSharedStubScenarioso these stubs use theILStubCachefor de-duplication.CreateHashBlob, create a minimal hash blob containing just the targetMethodDescpointer for forward P/Invoke stubs, so different P/Invoke methods get distinct cache entries while racing threads for the same method converge on the sameDynamicMethodDesc.Fixes #124530