[mono] Fix for issue 109410 #113140
[mono] Fix for issue 109410 #113140
Conversation
ghost
added
the
|
@BrzVlad noticed that we might need a memory barrier to prevent other threads from observing a We haven't seen this on x64, which makes sense because x86 and x64 have a stronger memory model, but I need to figure out whether this particular failure is possible with ARM's weaker memory model or not. |
Remove emitted nullcheck in jitcode (too expensive)
kg
changed the title
| { | ||
| error_init (error); | ||
|
|
||
| g_assert (klass); |
There was a problem hiding this comment.
This assert is not particularly helpful, we will crash immediately at the line below anyway.
There was a problem hiding this comment.
on wasm we wouldn't, but i don't know if this code runs on wasm
| // we need a barrier before publishing data via mrgctx->infos [i] because the contents of data may not | ||
| // have been published to all cores and another thread may read zeroes or partially initialized data | ||
| // out of it, even though we have a barrier before publication of entries in mrgctx->entries below | ||
| mono_memory_barrier(); |
There was a problem hiding this comment.
Could an option be to use a local array in this loop and then do a memcpy into mrgctx->infos after the memory barrier we already fire below (where we copy entries)? That way we will only fire one memory barrier instead of info->num_entries.
There was a problem hiding this comment.
Feels like a breakeven performance optimization due to the cost of making the local array and copying at the end. Do you want me to do it before merging? I don't mind if you believe it's important.
There was a problem hiding this comment.
We will do a full memory barrier so it will have wider impact cross cpu caches. using local stack memory and memcpy won't prevent other cores from running at full speed, but since this is just on the init path and then never again, it might not be worth optimizing. You can keep as is.
|
could you backport to release/9.0 ? |
|
/backport to release/9.0-staging |
|
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/13976774768 |
… multi-threaded scenarios (#113740) Backport of #113140 Issue #109410 appears to be a case where klass is 0 when we perform an isinst operation, but the cache and obj are nonzero and look like valid addresses. klass is either a compile-time (well, jit-time) constant or being fetched out of the cache (it looks like it can be either depending on some sort of rgctx condition). This PR adds null checks in two places along with a memory barrier in the location where we believe an uninitialized cache is being published to other threads. --------- Co-authored-by: Katelyn Gadd <[email protected]>
Issue #109410 appears to be a case where klass is 0 when we perform an isinst operation, but the cache and obj are nonzero and look like valid addresses. klass is either a compile-time (well, jit-time) constant or being fetched out of the cache (it looks like it can be either depending on some sort of rgctx condition).
This PR adds null checks in two places along with a memory barrier in the location where we believe an uninitialized cache is being published to other threads.