-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NativeAOT] Initial osx-arm64 bring up #75264
Conversation
@@ -633,7 +633,7 @@ void GCToOSInterface::YieldThread(uint32_t switchCount) | |||
static void* VirtualReserveInner(size_t size, size_t alignment, uint32_t flags, uint32_t hugePagesFlag = 0) | |||
{ | |||
assert(!(flags & VirtualReserveFlags::WriteWatch) && "WriteWatch not supported on Unix"); | |||
if (alignment == 0) | |||
if (alignment < OS_PAGE_SIZE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap under #if defined(HOST_ARM64) && defined(TARGET_OSX) && defined(FEATURE_NATIVEAOT)
to limit the scope of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still like to know why CoreCLR doesn't hit this but I assume that it's because of some GC feature switch (such as FEATURE_MANUALLY_MANAGED_CARD_BUNDLES
).
While I can add the #if
guards the code would still misbehave fatally when alignment < OS_PAGE_SIZE
so I think the change is quite safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still like to know why CoreCLR doesn't hit this
AFAIK gcenv.unix.cpp is only used by CoreCLR when standalone GC is used (i.e. COMPlus_GCName is set to point to a standalone GC instead of the one in coreclr.dll/.so). @mangod9 @dotnet/gc can you have a look at this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the regular CI pipelines do not use this code. We should trigger gc-standalone
pipeline https://dev.azure.com/dnceng/public/_build?definitionId=1017 against this PR to get realistic results (windows arm64 checked
failure in that pipeline is preexisting, the rest should be green). AFAIK, there is no azp run
trigger for that pipeline, someone with access will need to trigger it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, there is no azp run trigger for that pipeline, someone with access will need to trigger it manually.
I tried to trigger it but it's complaining about the pool. It doesn't look like the gc-standalone pipeline ran for a week or so. It coincides with the pool changes. Cc @dotnet/gc.
The interesting thing about the gc-standalone
pipeline is that it doesn't test mac at all. So if there was a arm64 macOS issue with standalone GC, we wouldn't even know it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there was a arm64 macOS issue with standalone GC, we wouldn't even know it.
This change as written is not specific to arm64 macOS, other platforms can break. My original commit was am11@45f3a48, but it was recreated without the guard which has potential to break existing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specific call with the "invalid" alignment
parameter is macOS arm64 specific though. It happens because of 16Kb page sizes and GC_PAGE_SIZE != OS_PAGE_SIZE
.
1c05b75
to
3d54d63
Compare
The last two commits unblock the library tests. It seems to work quite well, so we may try to get a full run on the CI once this is merged. |
builder.RequireInitialAlignment(factory.Target.MinimumFunctionAlignment); | ||
// These need to be aligned the same as method pointers because they show up in same contexts | ||
// (macOS ARM64 has even stricter alignment requirement for the linker, so round up to pointer size) | ||
builder.RequireInitialAlignment(factory.Target.PointerSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is actually hell to diagnose because it produces no error but causes corruption when linking. If absolute relocation of a symbols is 4-byte aligned and spans between two pages in the executable then only the lower part of the relocation is written correctly into the final executable.
This was the root cause of the intermittent problem I've hit earlier.
I ran the library test locally and the results look promising:
Specifically, System.Runtime.Extensions.Tests has single failure related to macOS Ventura (#75371):
System.Security.Cryptography.OpenSsl.Tests fails because of missing OpenSSL library (expected):
System.Diagnostics.Process.Tests has one failure (flaky):
System.Console.Tests has couple of failures related to Remote Executor (missing RemoteExecutor guard):
|
We can enable CI testing in the next PR (after this and #75055 are merged). Thanks a lot @filipnavara for pushing the boundaries! 🐎 |
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
…te symbol relocations to be aligned to native pointer size.
…ain absolute relocations. ld64 seem to silently corrupt them if they are not aligned to pointer size.
c12a7be
to
59b79b3
Compare
Rebased after #75393 merge. |
@filipnavara Thank you! |
@filipnavara thank you for this! A general question: is there an intention to backport this to 7.0, or is it strictly an 8.0 thing? |
It missed the 7.0 deadline by a lot. However, it is possible to consume the ILCompiler package from NuGet so I think the realistic goal is to make it possible to use that with .NET 7 SDK. |
The instructions for using daily build packages are at https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/docs/compiling.md#using-daily-builds |
The alignment changed in #75264, but it's important this is always at least MinimumFunctionAlignment no matter the platform because that's how we distinguish fat pointers from real pointers. This assert is obviously true on every platform we support right now.
The alignment changed in #75264, but it's important this is always at least MinimumFunctionAlignment no matter the platform because that's how we distinguish fat pointers from real pointers. This assert is obviously true on every platform we support right now.
Depends on dotnet/llvm-project#185.
Fixes #67232
Fixes #64337
Thanks to @am11 @MichalStrehovsky and @jkotas for the help with pushing this forward and analysing all the issues that came up.