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

[NativeAOT] Do not use private APIs on iOS/macOS #90430

Merged
merged 5 commits into from
Aug 12, 2023

Conversation

filipnavara
Copy link
Member

Fixes #90408.

Tested with upload of empty iOS app through TestFlight process.

platforms since it's a private API and it blocks uploads to TestFlight,
iOS App Store, and Mac App Store.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 11, 2023
@ghost
Copy link

ghost commented Aug 11, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #90408.

Tested with upload of empty iOS app through TestFlight process.

Author: filipnavara
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@ghost
Copy link

ghost commented Aug 11, 2023

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #90408.

Tested with upload of empty iOS app through TestFlight process.

Author: filipnavara
Assignees: -
Labels:

os-ios, community-contribution, area-NativeAOT-coreclr

Milestone: -

@ivanpovazan
Copy link
Member

/azp run runtime-ioslike,runtime-ioslikesimulator

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@akoeplinger
Copy link
Member

There is one testfailure in the iOS nativeaot smoke tests that might be related:

Test Harness Exitcode is : 1
      To run the test:
      > set CORE_ROOT=/tmp/helix/working/BB1E0A01/p
      > /private/tmp/helix/working/BB1E0A01/w/C1E70A94/e/nativeaot/SmokeTests/StackTraceMetadata/StackTraceMetadata/StackTraceMetadata.sh
      Expected: True
      Actual:   False
      Stack Trace:
           at nativeaot_SmokeTests_StackTraceMetadata._StackTraceMetadata_StackTraceMetadata_._StackTraceMetadata_StackTraceMetadata_sh()
           at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
           at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
      Output:
        
        Return code:      1

@filipnavara
Copy link
Member Author

filipnavara commented Aug 12, 2023

There is one testfailure in the iOS nativeaot smoke tests that might be related:

I saw that. Curiously it happened only on tvOS and not on iOS, and even more curiously it actually produced the stack trace for the Assert.

@filipnavara
Copy link
Member Author

filipnavara commented Aug 12, 2023

I got the payload from Helix for the failed tvOS test (runfo get-helix-payload --jobid=d9ecb7b0-3d83-4ac1-81ea-6b1036feac89 -o=tvos-runfo). Unfortunately I don't have Apple TV at hand and it's built for the device, so I cannot run it. I have some doubt that the change broke the test since the unwind info lookup is globally cached and the stack trace for the assert itself is printed correctly thus the unwind info had to be located.

@ivanpovazan
Copy link
Member

ivanpovazan commented Aug 12, 2023

I got the payload from Helix for the failed tvOS test (runfo get-helix-payload --jobid=d9ecb7b0-3d83-4ac1-81ea-6b1036feac89 -o=tvos-runfo). Unfortunately I don't have Apple TV at hand and it's built for the device, so I cannot run it. I have some doubt that the change broke the test since the unwind info lookup is globally cached and the stack trace for the assert itself is printed correctly thus the unwind info had to be located.

I just checked the same failure is visible in different CI run with console log. So that particular test failure is not related to this change.

Copy link
Member

@ivanpovazan ivanpovazan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

Unfortunately, with this change we should expect a size regression of ~2,5Mb for NativeAOT iOS apps.

Estimated regression with HelloiOS app:

HelloiOS main this PR diff-to-main (%) this PR + deadstrip diff-to-main(%)
SOD (Mb) 6,40 8,86 38,44% 8,18 27,81%

I also included the measurement with -dead_strip enabled for reference.

/cc: @kotlarmilos @SamMonoRT

@filipnavara
Copy link
Member Author

I also included the measurement with -dead_strip enabled for reference.

FWIW I found a way to mark the ILC output non-dead-strippable but I didn't get a chance to test it end-to-end.

@jkotas
Copy link
Member

jkotas commented Aug 12, 2023

Unfortunately, with this change we should expect a size regression of ~2,5Mb for NativeAOT iOS apps.

Is that because we are linking a private copy of ICU now?

Does the HybridGlobalization work with native AOT?

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@filipnavara
Copy link
Member Author

Is that because we are linking a private copy of ICU now?

Yep, this just gets on the same plan as MonoVM.

Does the HybridGlobalization work with native AOT?

It should but I don't think it's tested.

@SamMonoRT
Copy link
Member

Is that because we are linking a private copy of ICU now?

Yep, this just gets on the same plan as MonoVM.

Does the HybridGlobalization work with native AOT?

It should but I don't think it's tested.

Correct - We haven't fully tested the HybridGlobalization with NativeAOT. But basic testing on a simple app works fine. Both of these (HybridGlobalization and NativeAOT for iOS/MAUI iOS are opt-in for .NET8)

@akoeplinger akoeplinger merged commit 7a0b4f9 into dotnet:main Aug 12, 2023
108 checks passed
@filipnavara filipnavara deleted the nativeaot-appstore branch August 12, 2023 18:54
@akoeplinger
Copy link
Member

Awesome work, thank you @filipnavara 💟

@filipnavara
Copy link
Member Author

Thanks for handling this so quickly!

@ghost ghost locked as resolved and limited conversation to collaborators Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member os-ios Apple iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IOS TestFlight Non-public API usage error ITMS-90338 for __dyld_find_unwind_sections using NativeAOT
5 participants