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

[mono][ios] Add support for stripping during AOT compilation on iOS #87728

Merged

Conversation

kotlarmilos
Copy link
Member

This PR adds support for stripping debug symbols and enabling IL stripping during AOT compilation of the HelloiOS sample app.

  • Added StripSymbolTable="$(StripDebugSymbols)" to the AppleBuild.targets file.
  • Added <ShouldILStrip Condition="'$(RunAOTCompilation)' == 'true' and '$(MonoForceInterpreter)' != 'true'">true</ShouldILStrip> to the Program.csproj file.

This PR should resolve the reported SOD regression.
Screenshot 2023-06-17 at 14 07 05

@ghost
Copy link

ghost commented Jun 17, 2023

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR adds support for stripping debug symbols and enabling IL stripping during AOT compilation of the HelloiOS sample app.

  • Added StripSymbolTable="$(StripDebugSymbols)" to the AppleBuild.targets file.
  • Added <ShouldILStrip Condition="'$(RunAOTCompilation)' == 'true' and '$(MonoForceInterpreter)' != 'true'">true</ShouldILStrip> to the Program.csproj file.

This PR should resolve the reported SOD regression.
Screenshot 2023-06-17 at 14 07 05

Author: kotlarmilos
Assignees: kotlarmilos
Labels:

area-Infrastructure-mono, os-ios

Milestone: 8.0.0

@kotlarmilos
Copy link
Member Author

@steveisok, I noticed the commented ShouldILStrip property in the AppleBuild.props file. Is there an issue on the CI or can it be enabled for the library tests?

@steveisok
Copy link
Member

@steveisok, I noticed the commented ShouldILStrip property in the AppleBuild.props file. Is there an issue on the CI or can it be enabled for the library tests?

I think there was a problem running it on libraries, but does not appear an issue was logged. Try to enable on libraries and see what we get.

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.

LGTM!

Nit:
One small thing worth considering is adding in some top level import (like AppleBuild.props) a default value for StripDebugSymbols like:
<StripDebugSymbols Condition="'$(StripDebugSymbols)' == ''" >false</StripDebugSymbols>
I didn't find it being set anywhere except in the Makefile of the project.

@ivanpovazan
Copy link
Member

ivanpovazan commented Jun 17, 2023

A slightly off-topic comment, but related to HelloiOS SOD regressions:

There was also a regression reported with NativeAOT.
The regression seems to be introduced with: 2835110

However, the regression is justified, as for NativeAOT builds we were using feature switch <InvariantGlobalization>true</InvariantGlobalization> when building the sample project because the support for System.Globalization.Native was not implemented yet. This made the app smaller, but at the same time was not matching how Mono sample was configured.

This has been fixed in the mentioned change: 2835110#diff-41016a8344332e64360007e06a117aa072eac2efe3e0e0109ce27d5466695dccL17-L18
which makes the SOD of the NativeAOT app a bit bigger, but is now aligned with the Mono configuration, making the comparison fairer.

/cc: @SamMonoRT

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kotlarmilos
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@kotlarmilos
Copy link
Member Author

Some of the failures are reported in #73040 and shouldn't be related.

@kotlarmilos kotlarmilos merged commit 9e93f42 into dotnet:main Jun 19, 2023
98 of 106 checks passed
@akoeplinger
Copy link
Member

@steveisok, I noticed the commented ShouldILStrip property in the AppleBuild.props file. Is there an issue on the CI or can it be enabled for the library tests?

I think there was a problem running it on libraries, but does not appear an issue was logged. Try to enable on libraries and see what we get.

Yeah we had problems with ActiveIssue attributes not being retained so we disabled ILStrip a while ago in #59762

I think the underlying issue could have been fixed by #87923

@ghost ghost locked as resolved and limited conversation to collaborators Jul 28, 2023
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.

None yet

4 participants