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] Another attempt to prevent stripping exported symbols from executables when explicitly specified #86050

Merged
merged 2 commits into from
May 11, 2023

Conversation

ivanpovazan
Copy link
Member

This PR fixes a regression caused by #85293 which was reverted by #85601

Problem

The issue with the initial approach is that we used the following strip command to keep only the symbols exported by the compiler:

strip -i -s <symbols_to_keep.exports> <binary>

However, executing this command produces a warning:

/Applications/Xcode_13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/strip : warning : removing global symbols from a final linked no longer supported.  Use -exported_symbols_list at link time when building: /Users/runner/work/1/s/artifacts/tests/coreclr/osx.x64.Release/nativeaot/GenerateUnmanagedEntryPoints/GenerateUnmanagedEntryPoints/native/GenerateUnmanagedEntryPoints [/Users/runner/work/1/s/src/tests/nativeaot/GenerateUnmanagedEntryPoints/GenerateUnmanagedEntryPoints.csproj] [/Users/runner/work/1/s/src/tests/build.proj]

This can be observed in the runtime CI lane: https://dev.azure.com/dnceng-public/public/_build/results?buildId=258886&view=logs&j=66dceaa3-58e2-55af-b0bc-5748606ebc0e&t=2c1c2642-0b2e-5086-de47-1b8a1c2ab601&l=637

On the other hand, the job osx-x64 Release NativeAOT_Libs in the runtime-extra-platforms CI lane, treats warnings as errors, which results with a build failure when stripping is attempted: https://dev.azure.com/dnceng-public/public/_build/results?buildId=257857&view=logs&j=1b7517dc-3493-5614-db19-9039ca700e8e&t=30be22c2-1ac3-5d1e-cae6-35f9adc3d580&l=6564

Solution

To achieve the desired behaviour, it is needed to:

  1. Export only the desired symbols during native linking, as suggested by the warning message (which we already do)
    <CustomLinkerArg Include="-exported_symbols_list &quot;$(ExportsFile)&quot;" Condition="'$(_IsApplePlatform)' == 'true' and $(ExportsFile) != ''" />
  2. Strip local symbols from the binary:
strip -x <binary>

Fixes: #85600

@ghost
Copy link

ghost commented May 10, 2023

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

Issue Details

This PR fixes a regression caused by #85293 which was reverted by #85601

Problem

The issue with the initial approach is that we used the following strip command to keep only the symbols exported by the compiler:

strip -i -s <symbols_to_keep.exports> <binary>

However, executing this command produces a warning:

/Applications/Xcode_13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/strip : warning : removing global symbols from a final linked no longer supported.  Use -exported_symbols_list at link time when building: /Users/runner/work/1/s/artifacts/tests/coreclr/osx.x64.Release/nativeaot/GenerateUnmanagedEntryPoints/GenerateUnmanagedEntryPoints/native/GenerateUnmanagedEntryPoints [/Users/runner/work/1/s/src/tests/nativeaot/GenerateUnmanagedEntryPoints/GenerateUnmanagedEntryPoints.csproj] [/Users/runner/work/1/s/src/tests/build.proj]

This can be observed in the runtime CI lane: https://dev.azure.com/dnceng-public/public/_build/results?buildId=258886&view=logs&j=66dceaa3-58e2-55af-b0bc-5748606ebc0e&t=2c1c2642-0b2e-5086-de47-1b8a1c2ab601&l=637

On the other hand, the job osx-x64 Release NativeAOT_Libs in the runtime-extra-platforms CI lane, treats warnings as errors, which results with a build failure when stripping is attempted: https://dev.azure.com/dnceng-public/public/_build/results?buildId=257857&view=logs&j=1b7517dc-3493-5614-db19-9039ca700e8e&t=30be22c2-1ac3-5d1e-cae6-35f9adc3d580&l=6564

Solution

To achieve the desired behaviour, it is needed to:

  1. Export only the desired symbols during native linking, as suggested by the warning message (which we already do)
    <CustomLinkerArg Include="-exported_symbols_list &quot;$(ExportsFile)&quot;" Condition="'$(_IsApplePlatform)' == 'true' and $(ExportsFile) != ''" />
  2. Strip local symbols from the binary:
strip -x <binary>

Fixes: #85600

Author: ivanpovazan
Assignees: ivanpovazan
Labels:

area-NativeAOT-coreclr

Milestone: -

@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan
Copy link
Member Author

/cc: @lambdageek @SamMonoRT

@@ -335,7 +335,7 @@ The .NET Foundation licenses this file to you under the MIT license.
<PropertyGroup>
<_IgnoreLinkerWarnings>false</_IgnoreLinkerWarnings>
<_IgnoreLinkerWarnings Condition="'$(_IsApplePlatform)' == 'true'">true</_IgnoreLinkerWarnings>
<StripFlag Condition="'$(_IsApplePlatform)' == 'true' and '$(NativeLib)' == 'Shared'">-x</StripFlag> <!-- keep global symbols in dylib -->
<_StripFlag Condition="'$(_IsApplePlatform)' == 'true' and ('$(NativeLib)' == 'Shared' or '$(IlcExportUnmanagedEntrypoints)' == 'true')">-x</_StripFlag> <!-- keep global symbols in dylib (or if it is explicitly specified) -->
Copy link
Member

@jkotas jkotas May 10, 2023

Choose a reason for hiding this comment

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

Suggested change
<_StripFlag Condition="'$(_IsApplePlatform)' == 'true' and ('$(NativeLib)' == 'Shared' or '$(IlcExportUnmanagedEntrypoints)' == 'true')">-x</_StripFlag> <!-- keep global symbols in dylib (or if it is explicitly specified) -->
<_StripFlag Condition="'$(_IsApplePlatform)' == 'true' and '$(IlcExportUnmanagedEntrypoints)' == 'true'">-x</_StripFlag> <!-- keep global symbols -->

We set IlcExportUnmanagedEntrypoints for Shared lnative libs by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed that, thank you. Fixed.

@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas jkotas merged commit f1481ed into dotnet:main May 11, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 10, 2023
@ivanpovazan ivanpovazan deleted the naot-strip-fix branch August 15, 2023 09:44
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.

OSX build error: Removing global symbols from a final linked no longer supported
2 participants