Enable MHR support on OSX #25716
Enable MHR support on OSX #25716
Conversation
* Move JIT_WriteBarrier that is modified at runtime to a dynamically allocated memory instead of making a page in libcoreclr.dylib RWX. * Update PAL to add MEM_JIT flag for allocations and reservations of executable memory. * Update native runtime in EH and stack unwinding areas so that it can unwind from the write barrier copy. That code has no unwind info, so without special handling, runtime would not be able to unwind from it.
| { | ||
| static int isRunningOnMojaveHardenedRuntime = -1; | ||
|
|
||
| if (isRunningOnMojaveHardenedRuntime == -1) |
There was a problem hiding this comment.
Will this always be called from the same thread ?
There was a problem hiding this comment.
It doesn't matter. The worst case is that we would evaluate the stuff multiple times.
There was a problem hiding this comment.
It does matter. Note that the C/C++ spec allows compilers to duplicate the memory load and introduce a subtle race condition in the process. E.g. this can be transformed by the C/C++ compiler to:
static int isRunningOnMojaveHardenedRuntime = -1;
int result = isRunningOnMojaveHardenedRuntime;
if (isRunningOnMojaveHardenedRuntime == -1)
{
...
result = ...;
}
return result;
|
@dotnet/dnceng the OSX legs in this PR have failed with error in an azure devops python script: Can someone please take a look? |
src/pal/src/misc/utils.cpp
Outdated
|
|
||
| if (isRunningOnMojaveHardenedRuntime == -1) | ||
| { | ||
| if (GetDarwinVersion() >= DARWIN_VERSION_MOJAVE) |
There was a problem hiding this comment.
Do we need to bother with this version check at all? Given the Apple ecosystem upgrades very fast, this is going to be just an expensive way to compute "true" in like a year.
|
@janvorli I'm taking a look now and will file issue(s) as appropriate. |
|
This isn't new, but an artifact of the fact that other than OSX 10.14, the OSX Helix machines are still running Python 2 client code while bits like the line you mentioned are now Python-3 specific. As noted in the logs though, the work items failed with this assert: and had they passed, your run would have passed (indeed this has been the state for some time). For instance, see the OSX run in #25691 The challenge here is that updating our on-prem OSX machines is much more work than other types of systems, and most machines will be fully reimaged in the process. @ilyas1974 is working on this and it's tracked via https://github.com/dotnet/core-eng/issues/7003. From chatting with him a rough ETA for when the 10.13 machines would be August (next month) |
|
@MattGal thank you for digging out the assert, that's a problem related to my change. |
|
Strange, I don't get the failure to allocate the memory page for memory barriers on my local Mac, all the pri 1 coreclr tests are passing. I wonder if it is related to the fact that the lab machines don't have Mojave installed yet. I'm looking into it. |
|
@janvorli the CoreCLR tests are explicitly opted-in to running on High Sierra, that's what osx.1013.amd64.open effectively means. You can fix both the AzDO reporting error AND run on Mojave if you like by simply only testing on osx.1014.amd64.open, but do note that until 10.16 comes out, 10.13 is at least supported for security patches and should be considered a place we need to make sure .NET core works. |
|
Ok, now the OSX tests in the lab are passing. There was actually a bug in the IsRunningOnMojaveHardenedRuntime that I have fixed when I've removed the check for the OS version in the last commit that was causing it. |
@janvorli , the OSX tests are only testing the non MHR case right ? We don't have any tests with Sandbox/MHR enabled ? |
No we don't. I am not sure how we would enable that in the lab. For some reason, I have to sign the libcoreclr.dylib from a terminal running in the UI of my Mac. If I try to run the same command line via SSH connection, it fails. But I guess there must be some way. Anyways, I have tested my change with MHR enabled using a coreclr test that exercised all the code paths I've touched. |
|
I will also try find some time in the next few days to backport this change (as we are targeting 2.2 right now) and will verify this change with our product. |
For reference, this is what Mono uses for the tests: https://github.com/mono/mono/pull/15589/files. Obviously the prerequisite is to have macOS Mojave or Catalina on CI which was not the case last time I checked. |
Not part of this change, but I think it would be quite important to have some automated test so we make sure we don't break this in future changes |
|
Any chance this can be merged to 3.0? #18617 |
|
I am reverting this change now. I have found a couple of issues in it and I am leaving for a week of vacation, so I'll be able to fix it once I'm back. The issue is that the JIT_Stelem_Ref_xxxx functions accidentally call the original unpatched JIT_WriteBarrier. |
* Enable MHR support on OSX * Move JIT_WriteBarrier that is modified at runtime to a dynamically allocated memory instead of making a page in libcoreclr.dylib RWX. * Update PAL to add MEM_JIT flag for allocations and reservations of executable memory. * Update native runtime in EH and stack unwinding areas so that it can unwind from the write barrier copy. That code has no unwind info, so without special handling, runtime would not be able to unwind from it. Commit migrated from dotnet/coreclr@7a97084
…eclr#25803) This reverts commit dotnet/coreclr@7a97084. Commit migrated from dotnet/coreclr@1991a9a
Move JIT_WriteBarrier that is modified at runtime to a dynamically
allocated memory instead of making a page in libcoreclr.dylib RWX.
Update PAL to add MEM_JIT flag for allocations and reservations of
executable memory.
Update native runtime in EH and stack unwinding areas so that it can
unwind from the write barrier copy. That code has no unwind info, so
without special handling, runtime would not be able to unwind from
it.