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/6.0.1xx] Arm support #13378

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

ayakael
Copy link

@ayakael ayakael commented Mar 10, 2022

Expected behavior

As prebuilt SDKs are provided for arm by Microsoft, source-build should support ARM build.

Actual behavior

Per dotnet/source-build#2781, source-build does not support arm.

Proposed modifications

This pull request modifies existing Platform and Architecture logics to parse arch information correctly when BuildArchitecture=arm

Varia

Parallel merge request on Alpine's side
Virtualized 32-bit support: dotnet/runtime#66438

Made as part of Alpine Linux dotnet6 packaging project, see dotnet/source-build#2782

@ayakael
Copy link
Author

ayakael commented Mar 10, 2022

In addition, there's the following patch for runtime:

From 91988b0241a77f1f6524c19e2ce55dda06de7242 Mon Sep 17 00:00:00 2001
Patch-Source: https://src.fedoraproject.org/rpms/dotnet6.0/blob/rawhide/f/runtime-arm64-lld-fix.patch
From: Omair Majid <omajid@redhat.com>
Date: Wed, 16 Feb 2022 18:08:22 +0000
Subject: [PATCH 1/1] arm64 lld fix

---
 eng/native/init-compiler.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/eng/native/init-compiler.sh b/eng/native/init-compiler.sh
index 567d18da474..05245c7b9f8 100755
--- a/eng/native/init-compiler.sh
+++ b/eng/native/init-compiler.sh
@@ -108,11 +108,8 @@ if [[ -z "$CC" ]]; then
 fi
 
 if [[ "$compiler" == "clang" ]]; then
-    if command -v "lld$desired_version" > /dev/null; then
-        # Only lld version >= 9 can be considered stable
-        if [[ "$majorVersion" -ge 9 ]]; then
-            LDFLAGS="-fuse-ld=lld"
-        fi
+    if "$CC" -fuse-ld=lld -Wl,--version >/dev/null 2>&1; then
+        LDFLAGS="-fuse-ld=lld"
     fi
 fi
 
-- 
2.34.1

@omajid Could you share why this is needed / why it hasn't been upstreamed to upstream?

@omajid
Copy link
Member

omajid commented Mar 10, 2022

@omajid Could you share why this is needed / why it hasn't been upstreamed to upstream?

This change does a couple of things:

  • Stop looking for lld$version. That is, in systems where you have just generic lld, but not lld13.0, use lld directly.
  • This was then replaced with just asking if clang can use lld, letting clang itself deal with finding a compatible lld and verifying it works. The previous PR and this one were merged as Support using unversioned lld as linker too arcade#8189
  • Redirecting output to /dev/null was needed because it broke other scripts: Fix init-compiler.sh for older clang arcade#8219
  • I also removed the version check for clang's major version, because, as far as I can tell, majorVersion isn't set when you run clang as just the unversioned clang. In other words, using the simple ./build.sh, LDFLAGS was never set. This needs some tweak to handle the case - as the comment says - of verifying that at least clang 9 is available. It couldn't be upstreamed as-is and I never got around to finding a better solution and sending it upstream. Sorry about that.

@@ -117,7 +117,7 @@
</Target>

<Target Name="GenerateRootFs" Condition="'$(OS)' != 'Windows_NT'">
<Exec Condition="$(Platform.Contains('arm')) AND '$(Platform)' != 'armel' AND '$(BuildArchitecture)' != 'arm64'" Command="$(ArmEnvironmentVariables) $(ProjectDir)cross/build-rootfs.sh" />
<Exec Condition="$(Platform.Contains('arm')) AND '$(Platform)' != 'armel' AND '$(BuildArchitecture)' != 'arm64' AND '$(BuildArchitecture)' != 'arm'" Command="$(ArmEnvironmentVariables) $(ProjectDir)cross/build-rootfs.sh" />
Copy link
Member

Choose a reason for hiding this comment

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

We really ought to have some other way of setting up cross compilation rather than inferring it based on architecture :(

Copy link
Author

Choose a reason for hiding this comment

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

Indeed! Also, now that Plaform.Contains('arm') excludes armel, arm64 and arm, I'm wondering what other possible value of Platform would contain arm.

Copy link
Author

Choose a reason for hiding this comment

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

I tried exploring this suggestion - but as I don't know enough about cross compilation, I'm not comfortable proposing a solution. Would you think it better to open an issue to keep track of this?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's totally unsuitable for fixing as part of this PR. Filed https://github.com/dotnet/installer/issues/13441

@@ -9,7 +9,7 @@

<PropertyGroup>
<BuildArchitecture>$([System.Runtime.InteropServices.RuntimeInformation]::ProcessArchitecture.ToString().ToLowerInvariant())</BuildArchitecture>
<Architecture Condition="'$(Architecture)' == '' AND '$(BuildArchitecture)' == 'arm64'">$(BuildArchitecture)</Architecture>
<Architecture Condition="'$(Architecture)' == '' AND ( '$(BuildArchitecture)' == 'arm64' OR '$(BuildArchitecture)' == 'arm') ">$(BuildArchitecture)</Architecture>
<Architecture Condition="'$(Architecture)' == '' AND '$(BuildArchitecture)' == 's390x'">$(BuildArchitecture)</Architecture>
<Architecture Condition="'$(Architecture)' == ''">x64</Architecture>
Copy link
Member

Choose a reason for hiding this comment

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

(This suggestion is out of scope for this PR)

We use this idiom in a few repos and I think we got it from runtime? IIRC, this was done so we could still be able to use Visual Studio (an x86) application and have it correctly target x64. But that little bit of default behaviour means every other architecture needs to be hard-coded here separately. Perhaps we should consider handling Visual Studio and/or Windows here specifically and let everything else default to $(BuildArchitecture)?

Theoretically, through mono, we also support sparc, ppc and mips.

Copy link
Author

Choose a reason for hiding this comment

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

How does one detect visual studio use? I'm playing with $([MSBuild]::IsOSPlatform('WINDOWS')) for that specific case, but idk if that covers every case where Visual Studio / Windows is used.

eng/Build.props Outdated Show resolved Hide resolved
@ayakael
Copy link
Author

ayakael commented Mar 15, 2022

Reference in new

Looks like current main of runtime has this fix already:

# Only lld version >= 9 can be considered stable
if [[ "$compiler" == "clang" && "$majorVersion" -ge 9 ]]; then
   if "$CC" -fuse-ld=lld -Wl,--version >/dev/null 2>&1; then
       LDFLAGS="-fuse-ld=lld"                  
   fi
fi  

Copy link

@crummel crummel left a comment

Choose a reason for hiding this comment

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

Some minor fixes. I'll re-run the build so I can get a look at the Windows ARM error as well.

Directory.Build.props Outdated Show resolved Hide resolved
src/SourceBuild/tarball/content/Directory.Build.props Outdated Show resolved Hide resolved
src/SourceBuild/tarball/content/repos/known-good.proj Outdated Show resolved Hide resolved
@crummel
Copy link

crummel commented Apr 14, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@crummel
Copy link

crummel commented May 5, 2022

Failing early in the build:

D:\a\_work\1\s\Native.sln.metaproj : error MSB4126: The specified solution configuration "Release|arm" is invalid. Please specify a valid solution configuration using the Configuration and Platform properties (e.g. MSBuild.exe Solution.sln /p:Configuration=Debug /p:Platform="Any CPU") or leave those properties blank to use the default solution configuration. [D:\a\_work\1\s\Native.sln]
##[error]D:\a\_work\1\s\Native.sln.metaproj(0,0): error MSB4126: (NETCORE_ENGINEERING_TELEMETRY=Build) The specified solution configuration "Release|arm" is invalid. Please specify a valid solution configuration using the Configuration and Platform properties (e.g. MSBuild.exe Solution.sln /p:Configuration=Debug /p:Platform="Any CPU") or leave those properties blank to use the default solution configuration.

I think Release|arm and Debug|arm configurations just need to be added to the Native solution file here: https://github.com/dotnet/installer/blob/main/Native.sln.

@MichaelSimons
Copy link
Member

@ayakael - Is this something you are still able to contribute to?

@ayakael
Copy link
Author

ayakael commented Aug 19, 2022

|

Thanks for the bump, the PR fell through the cracks. Pushed the suggested changes. :)

@ayakael
Copy link
Author

ayakael commented Aug 19, 2022

ìnstaller (build Windows_NT Build_Release_arm) fails with LINK : fatal error LNK1181: cannot open input file 'wcautil.lib' [D:\a\_work\1\s\artifacts\obj\finalizer\arm\Release\Finalizer.vcxproj]

Any idea what could cause this?

@ayakael ayakael changed the title Arm support [release/6.0.1xx] Arm support Sep 11, 2022
@ayakael
Copy link
Author

ayakael commented Sep 11, 2022

Check passes in pull request for main here #14470

Directory.Build.props Outdated Show resolved Hide resolved
@ayakael
Copy link
Author

ayakael commented Nov 1, 2022

@crummel Now that this is merged in main, could this merged in release/6.0.1xx as well?

@MichaelSimons
Copy link
Member

@omajid - Can you take another look and signoff if you approve? TIA

Copy link
Member

@omajid omajid left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for getting this working!

@ayakael
Copy link
Author

ayakael commented Feb 18, 2023

@MichaelSimons Who can I ping to merge this as to get it in 6.0.115?

@MichaelSimons MichaelSimons merged commit 8616a1d into dotnet:release/6.0.1xx Feb 21, 2023
@MichaelSimons
Copy link
Member

Who can I ping to merge this as to get it in 6.0.115?

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants