[mono][interp] Avoid leaving around pages without access#119754
[mono][interp] Avoid leaving around pages without access#119754BrzVlad merged 2 commits intodotnet:mainfrom
Conversation
Otherwise it would crash instruments when trying to access it.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a crash in instruments by ensuring memory pages don't remain without access permissions after a memory barrier operation. The change reorders memory protection calls to avoid leaving pages in an inaccessible state.
- Moves the memory protection restoration call to after the no-access protection call
- Ensures the helper page is accessible after the memory barrier operation completes
github-actions
bot
added
the
needs-area-label
|
Original fix found thanks to @rolfbjarne in #90394 (comment) |
|
CoreCLR has a completely different way of achieving a process wide memory barrier. This commit avoids any drastic changes to the mechanism behind. Also I'm not certain whether there are official documentations about this guaranteeing the required memory barrier. |
BrzVlad
added
area-VM-meta-mono
and removed
needs-area-label
|
Looks like CoreCLR implements a similar approach, , but it's using mmap instead and locks the page. On Apple platforms CoreCLR does a completely different thing thought. I guess going from read/write to none back to read/write should have the same effects on TLBs and buffers as we have in our initial assumption but maybe is not too bad of an idea to try to use CoreCLR's approach, at least in main since their approach on Apple platforms is different. |
CoreCLR's implementation was introduced here: #44670 and it seems there was a discussion with Apple beforehand: #41993 |
|
minipal_initialize_memory_barrier_process_wide and minipal_memory_barrier_process_wide is in minipal so we should be able to use the same implementation on Mono since minipal is a shared library. |
|
I just stumbled across that PR as well but there doesn't seem to be any significant explanation about it. Maybe @janvorli would have some more context. I wanted to backport this change to .net10, potentially .net9 and wouldn't want to introduce any risky changes. I believe we could move to the minipal one with a follow up PR for .net 11. |
|
/ba-g wasm build tests known error |
|
/backport to release/10.0 |
|
Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/17855750433 |
* [mono][interp] Avoid leaving around pages without access Otherwise it would crash instruments when trying to access it. * add comment
…net#119754)" This reverts commit 9031045.
Otherwise it would crash instruments when trying to access it. This impacts mostly interpreter because we have the memory barrier call in
sgen_client_scan_thread_datawhen interpreter is enabled.Fixes #90394