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][8.0] Use ld_classic in ILC build and in build integration #97856

Merged
merged 4 commits into from
Feb 12, 2024

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Feb 2, 2024

Fixes: #97745

Customer Impact

Programs produced in an environment running XCode version 15 or higher will experience various crashes upon starting.

Also, if a build machine is upgraded to xcode 15+ it can't complete the build successfully as ILC, being a NativeAOT component, will not be runnable, thus stages such as building tests would fail.

Analysis

XCode 15 has introduced a new linker that contains a number of incompatibilities. Some of those incompatibilities may be unintentional and may eventually be fixed. It seems possible that point releases (i.e 15.0 vs. 15.2) have different set of issues.
For the time being there is a way to force the use of the old (aka "classic") linker and get around incompatibilities.

While we can chase incompatibilities and adapt to them in the active development branch, in 8.0 branch it makes more sense to opt for "classic" behavior.

Testing

Manual testing locally with XCode 15.2 and regular lab run with XCode 14 that is still used by the lab.

Risk

Low. This change conditionally reintroduces preexisting linker behavior.

There is a risk that at some point "classic" linker will no longer be supported.
It will likely be far enough in the future that the "prime" linker becomes more stable and the runtime/compiler could be adapted to work with it at that time.

@ghost
Copy link

ghost commented Feb 2, 2024

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

Issue Details

Fixes: #97745

Author: VSadov
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@filipnavara
Copy link
Member

filipnavara commented Feb 5, 2024

I think we should do it. Xamarin workloads in .NET 8 already force the old linker, and the new linker appears quite buggy. We can try to workaround/fix things for .NET 9 and backport them if Apple decides to ditch the old linker.

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.

The OSX builds are failing with ld: library not found for -ld_classic.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 5, 2024
@MichalPetryka
Copy link
Contributor

The OSX builds are failing with ld: library not found for -ld_classic.

Probably the code needs to test if XCode is 15 or later?

@filipnavara
Copy link
Member

Probably the code needs to test if XCode is 15 or later?

Possibly, and use -Wl, prefix.

@@ -79,6 +79,7 @@

<ItemGroup Condition="'$(NativeAotSupported)' == 'true'">
<CustomLinkerArg Condition="'$(CrossBuild)' == 'true' and '$(_hostArchitecture)' == '$(_targetArchitecture)' and '$(_hostOS)' != 'windows' and '$(_IsApplePlatform)' != 'true'" Include="--gcc-toolchain=$(ROOTFS_DIR)/usr" />
<CustomLinkerArg Condition="'$(_IsApplePlatform)' == 'true'" Include="-ld_classic" />
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed?

Copy link
Member

Choose a reason for hiding this comment

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

IIRC it uses the released package, not the one we just built, so it is necessary.

@VSadov
Copy link
Member Author

VSadov commented Feb 5, 2024

Probably the code needs to test if XCode is 15 or later?

Is there a way to check for that?
Or perhaps we should use -ld64 and ignore the warning that -ld-classic is preferred.

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 5, 2024
@am11
Copy link
Member

am11 commented Feb 5, 2024

Is there a way to check for that?

We added major version detection for lld (for linker script):

<Exec Command="&quot;$(CppLinker)&quot; -fuse-ld=lld -Wl,--version" Condition="'$(LinkerFlavor)' == 'lld'" IgnoreExitCode="true" StandardOutputImportance="Low" ConsoleToMSBuild="true">
<Output TaskParameter="ExitCode" PropertyName="_LinkerVersionStringExitCode" />
<Output TaskParameter="ConsoleOutput" PropertyName="_LinkerVersionString" />
</Exec>
<PropertyGroup Condition="'$(_LinkerVersionStringExitCode)' == '0' and '$(_LinkerVersionString)' != ''">
<_LinkerVersion>$([System.Text.RegularExpressions.Regex]::Match($(_LinkerVersionString), '[1-9]\d*'))</_LinkerVersion>

exact same regex returns 15 with xcodebuild -version command on my machine ™️.

@VSadov VSadov marked this pull request as ready for review February 7, 2024 06:41
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.

LGTM modulo feedback.

@VSadov Could you please get it approved for servicing once ready?

Also, we will want to apply it to main as well: #98117 (comment)

@carlossanlop
Copy link
Member

Friendly reminder that Monday February 12th is the Code Complete deadline for the March Release. Please make sure to address the feedback and then send the email to Tactics requesting approval.

…e.Unix.targets


PR feedback

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

VSadov commented Feb 9, 2024

I've re-validated that the fix still works after the PR feedback changes and that UseLdClassicXCodeLinker also works - right now export UseLdClassicXCodeLinker=false results in test build failures, as expected

@VSadov VSadov added the Servicing-consider Issue for next servicing release review label Feb 9, 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 could consider this a tell mode change to accommodate a coming xcode change

@VSadov VSadov added this to the 8.0.x milestone Feb 10, 2024
steveisok pushed a commit to steveisok/runtime that referenced this pull request Feb 12, 2024
@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 12, 2024
@jeffschwMSFT jeffschwMSFT modified the milestones: 8.0.x, 8.0.3 Feb 12, 2024
@jeffschwMSFT jeffschwMSFT merged commit bb540a8 into dotnet:release/8.0-staging Feb 12, 2024
110 of 114 checks passed
@VSadov
Copy link
Member Author

VSadov commented Feb 12, 2024

Thanks!!

@VSadov VSadov deleted the 80stage branch February 12, 2024 17:49
jkotas added a commit to jkotas/runtime that referenced this pull request Feb 20, 2024
…dotnet#97856)

* use ld_classic in ILC build and in build integration

* PR feedback

* Maybe fix the build for non-apple

* Update src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.Unix.targets

PR feedback

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
jkotas added a commit that referenced this pull request Feb 21, 2024
steveisok added a commit that referenced this pull request Feb 29, 2024
This pool is only available internally for now, so let's use it!

* Add workaround from #97856 to use classic linker on macos
@github-actions github-actions bot locked and limited conversation to collaborators Mar 14, 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.

7 participants