dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.91k stars 4.63k forks source link

SPC: Set PlatformType property to arm64 instead of AnyCPU for arm64 builds #84424

Closed akoeplinger closed 1 year ago

akoeplinger commented 1 year ago

We're setting AnyCPU as the PlatformTarget for corelib here (in all runtimes):

https://github.com/dotnet/runtime/blob/657865ff3c7a23a97afe24528a746b44a416ba73/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj#L75-L76

Roslyn gained support for setting arm64 as a platform in https://github.com/dotnet/roslyn/commit/e65a6c589409efe007b59c77944bc15b801512ca about 5 years ago. I traced git history and it looks like the AnyCPU value for arm64 has been there from the beginning of coreclr being open-sourced 9 years ago so it was never updated.

I don't know exactly what the impact of changing this would be but thought I'd bring it up for discussion.

/cc @jkotas @vitek-karas

ghost commented 1 year ago

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

Issue Details
We're setting AnyCPU as the PlatformTarget for corelib here (in all runtimes): https://github.com/dotnet/runtime/blob/657865ff3c7a23a97afe24528a746b44a416ba73/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj#L75-L76 Roslyn gained support for setting arm64 as a platform in https://github.com/dotnet/roslyn/commit/e65a6c589409efe007b59c77944bc15b801512ca about 5 years ago. I traced git history and it looks like the AnyCPU value for arm64 has been there from the beginning of coreclr being open-sourced 9 years ago so it was never updated. I don't know exactly what the impact of changing this would be but thought I'd bring it up for discussion. /cc @jkotas @vitek-karas
Author: akoeplinger
Assignees: -
Labels: `arch-arm64`, `area-Meta`
Milestone: -
jkotas commented 1 year ago

Processor architecture on assembly identities is obsolete concept. Properties returning this enum are marked obsolete. We are not adding new architectures to ProcessorArchitecture enum. We cannot really do that even if we wanted to - the enum encoding in metadata does not have enough bits to hold all processor architectures supported by .NET.

The processor architecture on CoreLib is set just for compatibility with .NET Framework. There are (obsolete) APIs that return it. If we were to set processor architecture on CoreLib for Arm64, we would need to revisit the whole thing - add the Arm64 member to the obsolete ProcessorArchitecture, fix all obsolete APIs that deal with it to handle it, etc. Not worth it. It can easily break more things than it would fix.

jkotas commented 1 year ago

Duplicate of https://github.com/dotnet/runtime/issues/58970

akoeplinger commented 1 year ago

I wasn't thinking about changing the ProcessorArchitecture enum, but I see this comment now: https://github.com/dotnet/runtime/blob/657865ff3c7a23a97afe24528a746b44a416ba73/src/coreclr/System.Private.CoreLib/src/System/Reflection/AssemblyName.CoreCLR.cs#L142-L144

I stumbled over this due to https://github.com/dotnet/cecil/pull/60 which uses code like this:

var pe64 = module.Architecture == TargetArchitecture.AMD64 ||
module.Architecture == TargetArchitecture.IA64 ||
module.Architecture == TargetArchitecture.ARM64;

Vitek said it looks weird that we don't set PlatformTarget for ARM64 so I thought I'd better ask 😄

jkotas commented 1 year ago

I stumbled over this due to https://github.com/dotnet/cecil/pull/60 which uses code like this:

This code does not look right to me: https://github.com/dotnet/cecil/pull/60#discussion_r1160055418