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

[release/8.0-staging] Always keep global symbols on ApplePlatforms #99650

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Mar 12, 2024

Backport of #99504 to release/8.0-staging

/cc @mikem8361 @am11

Customer Impact

  • Customer reported
  • Found internally

The Native AOT SOS support will not work on MacOS without this fix.

Regression

  • Yes
  • No

Testing

Manual testing by creating a Native AOT MacOS core dump with this change and running the Native AOT SOS support.

Risk

Low. It only changes the symbol stripping logic to not strip export symbols.

Copy link
Contributor

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

@mikem8361 mikem8361 self-assigned this Mar 12, 2024
@mikem8361 mikem8361 added this to the 8.0.x milestone Mar 12, 2024
@mikem8361 mikem8361 requested a review from jkotas March 12, 2024 22:51
@mikem8361 mikem8361 added the Servicing-consider Issue for next servicing release review label Mar 12, 2024
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will treat this one as tell mode. please get a code review

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

@jeffschwMSFT
Copy link
Member

quick question before merging - what, if any, is the expected impact on output size? Marginal increase?

@jkotas
Copy link
Member

jkotas commented Mar 13, 2024

quick question before merging - what, if any, is the expected impact on output size? Marginal increase?

@am11 Did you happen to check the binary size impact of this change?

@am11
Copy link
Member

am11 commented Mar 13, 2024

The problem was that on macOS, we weren't exporting DotNetRuntimeDebugHeader with the default StripSymbols=true. It was an unintentional behavior difference compared to linux and Windows.

@am11 Did you happen to check the binary size impact of this change?

Yes #99504 (comment) (32-48 bytes increase depending on alignment?). We are only exporting one symbol by default for executables:

<IlcArg Condition="'$(_targetOS)' != 'win' and '$(DebuggerSupport)' != 'false'" Include="--export-dynamic-symbol:DotNetRuntimeDebugHeader" />

If user is setting IlcExportUnmanagedEntrypoints=true without OutputType=Exe, and the app has lots of exported symbols, then it will add 32-48 bytes for each. Those preconditions are unlikely and the size overhead is negligible.

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 13, 2024
@jeffschwMSFT jeffschwMSFT modified the milestones: 8.0.x, 8.0.5 Mar 13, 2024
@jeffschwMSFT
Copy link
Member

failures are known issues

@jeffschwMSFT jeffschwMSFT merged commit f09cd44 into release/8.0-staging Mar 13, 2024
107 of 116 checks passed
@jkotas jkotas deleted the backport/pr-99504-to-release/8.0-staging branch March 15, 2024 23:36
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants