[release/10.0] [mono][interp] Avoid leaving around pages without access#119885
[release/10.0] [mono][interp] Avoid leaving around pages without access#119885github-actions[bot] wants to merge 2 commits intorelease/10.0from
Conversation
Otherwise it would crash instruments when trying to access it.
github-actions
bot
requested review from
steveisok and
vitek-karas
as code owners
|
Mono implementation of memory_barrier_process_wide_helper_page is not 100% reliable. There are situations where the OS detects that the IPC interrupt is unnecessary and skips sending it. It is why CoreCLR has different implementation - this implementation was built after discussions with Apple. Can this "minor" change in ordering make the implementation even less reliable than it is currently and introduce frequent-enough intermittent crashes in customer apps? |
|
I don't have an answer to this because we are pretty much relying on behavior that is not well defined and not in our control. I don't believe reusing the CoreCLR implementation is on the table, I don't think the code ever ran on ios/maccatalyst and the net10 branch doesn't contain the move of this code to the shared minipal. So, for .net10 we are left either with this PR or with https://gist.github.com/rolfbjarne/26c49b8d8ced32f6688024b7a691fcb6, but I think the same argument could be made about each one of them. I'm fine with the second option, if you think it feels less risky. |
The risk should not be qualified as "low" then. Isn't the actual problem that we are modifying protection of pages from regular C heap? It is not ok to do that. It can cause all sorts of problems. For example, if the C heap uses large pages, we would have a problem as well since the protections would be modified on the whole large page and affected unrelated memory. Should the fix rather be to allocate the page using mmap instead of posix_memalign? I would expect that Instruments will be fine with noaccess pages allocated using mmap. |
|
Yes, normally we should be using mmap here as a fix, but I wasn't sure if that would have even more side effects that would change the behavior. |
|
I think switching the allocation to mmap would be lower risk than changing the protection ordering, and it would be more complete fix for the Instruments issue. With the current fix, there is still window where the page protection is none and the Instruments can crash. |
|
closed in favor of #120293 |
2 participants
Backport of #119754 to release/10.0
/cc @BrzVlad
Customer Impact
Profiling with instruments doesn't work on maui maccatalyst/ios primarily when interpreter is enabled.
Regression
This is not really a regression on maui, but the problematic code is usually executed when interpreter is enabled. Given that on Xamarin the interpreter was less likely to be enabled, this could be a regression for some users.
Testing
Tested locally on ios and maccatalyst with complex enough applications that trigger this.
Risk
Low. This fix just a minor change to the order in which we map pages as part of the implementation for process wide memory barrier.