Skip to content
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

Use latest ILC to AOT-compile ILC #89655

Merged
merged 6 commits into from
Aug 1, 2023
Merged

Use latest ILC to AOT-compile ILC #89655

merged 6 commits into from
Aug 1, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Jul 28, 2023

We need to build with a recent enough ILC that has the fix from #87785. This sets up a dependency on the runtime -> runtime package flow to build using the latest ILC package.

Copy link
Member

@LakshanF LakshanF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I validated that the NativeAOT ILC build with these changes (artifacts\bin\coreclr\windows.x64.Debug\ilc-published) does not cause problems.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a permanent change that is here to stay, or a temporary change that will be reverted after next SDK update?

sbomer and others added 2 commits July 29, 2023 00:05
Co-authored-by: Jeremy Koritzinsky <jkoritzinsky@gmail.com>
@sbomer
Copy link
Member Author

sbomer commented Jul 29, 2023

Is this a permanent change that is here to stay, or a temporary change that will be reverted after next SDK update?

I'm thinking we could make it permanent, so that future ILCompiler fixes flow through the system quickly - do you have an opinion?

@jkotas
Copy link
Member

jkotas commented Jul 29, 2023

I'm thinking we could make it permanent, so that future ILCompiler fixes flow through the system quickly - do you have an opinion?

We can give it a try. This setup may break when we need to push through a change that makes current native AOT incompatible with older SDK.

@am11
Copy link
Member

am11 commented Jul 29, 2023

I'm thinking we could make it permanent

Can we take a step further and bring it on the same plan as crossgen2? i.e. use "live build" of (unpublished) ilc to publish ilc for shipping nuget package. Then use the published bits in runtime and libraries testing as we do today with ilc (but not with crossgen2 yet due to issues on arm32 platform).

@MichalStrehovsky
Copy link
Member

I'm thinking we could make it permanent

Can we take a step further and bring it on the same plan as crossgen2? i.e. use "live build" of (unpublished) ilc to publish ilc for shipping nuget package. Then use the published bits in runtime and libraries testing as we do today with ilc (but not with crossgen2 yet due to issues on arm32 platform).

We scrapped the underlying tracking issue due to funding: #67742 (comment). It would be nicer to do it that way as it would avoid any versioning issue, but the LKG approach also has an advantage (we do all testing with the exact configuration that we're shipping).

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@sbomer
Copy link
Member Author

sbomer commented Jul 31, 2023

The osx-arm64 build is hitting linker errors during AOT-compile of ILC:

Undefined symbols for architecture arm64:
    "_objc_msgSend$AMSymbol", referenced from:
        _GlobalizationNative_GetLocaleInfoStringNative in libSystem.Globalization.Native.a(pal_locale.m.o)
    "_objc_msgSend$PMSymbol", referenced from:
        _GlobalizationNative_GetLocaleInfoStringNative in libSystem.Globalization.Native.a(pal_locale.m.o)
    "_objc_msgSend$UTF8String", referenced from:
        _DetectDefaultAppleLocaleName in libSystem.Globalization.Native.a(pal_locale.m.o)
        _GlobalizationNative_GetLocaleNameNative in libSystem.Globalization.Native.a(pal_locale.m.o)
        _GlobalizationNative_GetLocaleInfoStringNative in libSystem.Globalization.Native.a(pal_locale.m.o)
        _GlobalizationNative_GetLocaleInfoIntNative in libSystem.Globalization.Native.a(pal_locale.m.o)
        _GlobalizationNative_GetLocaleTimeFormatNative in libSystem.Globalization.Native.a(pal_locale.m.o)
    "_objc_msgSend$characterAtIndex:", referenced from:

This appears to be because the official build osx machines use XCode 14, which enables an optimization for Objective-C selectors that requires the selector stubs to be generated at link time. The PR builds use XCode 13 which doesn't support this optimization, but we are trying to link with libraries that were produced from XCode 14 (libSystem.Globalization.Native.a from the latest ILCompiler package).

I suspect this has been the case since #88793, which switched official builds but not PR builds over to the macOS 12 agents that come with XCode 14: https://github.com/actions/runner-images/blob/main/images/macos/macos-12-Readme.md#xcode.
PR builds still use macOS 11 which has XCode 13: https://github.com/actions/runner-images/blob/main/images/macos/macos-11-Readme.md#xcode.

I can build with these changes locally using a newer XCode version.

I would suggest moving PR builds to macOS 12 as well. Are we ok with requiring XCode 14 to build NativeAot apps on mac? @jkotas @MichalStrehovsky

@jkotas
Copy link
Member

jkotas commented Jul 31, 2023

I would suggest moving PR builds to macOS 12 as well. Are we ok with requiring XCode 14 to build NativeAot apps on mac?

I do not see a problem with that.

@sbomer sbomer merged commit a6d10a5 into dotnet:main Aug 1, 2023
174 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 31, 2023
@sbomer sbomer deleted the overrideILC branch November 3, 2023 18:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants