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

[API Proposal]: Add Arm64 to ProcessorArchitecture and ImageFileMachine enums #58970

Closed
davidwrighton opened this issue Sep 10, 2021 · 4 comments
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Reflection

Comments

@davidwrighton
Copy link
Member

Background and motivation

We now support Arm64 as a target architecture, but not all of the various enums that describe Arm64 PE files have been updated to allow for that support. In particular the System.Reflection.ImageFileMachine enum, and the System.Reflection.ProcessorArchitecture enums are missing arm64 implementation.

API Proposal

namespace System.Reflection
{
     public enum ImageFileMachine
     {
          ...
          // Note that the name is ALLCAPS here to follow the style of the other existing enum values (I386, IA64, AMD64 and ARM)
          ARM64 = 0xAA64
     }

    public ProcessorArchitecture
    {
        ...
        Arm64 = 6
    }
}

API Usage

The impact of a change to ImageFileMachine should be fairly subtle. It effects the behavior of reading an assembly with the System.Reflection.MetadataLoadContext library. Currently, when an Arm64 assembly is loaded, it is given the ProcessorArchitecture of None where it should get the new value Arm64.

Console.WriteLine(new AssemblyName("SomeName, ProcessorArchitecture=Arm64").ProcessorArchitecture);

The above should print Arm64 instead of failing with

Unhandled exception. System.IO.FileLoadException: The given assembly name or codebase was invalid. (0x80131047)
   at System.Reflection.AssemblyName.nInit()
   at System.Reflection.AssemblyName..ctor(String assemblyName)
   at <Program>$.<Main>$(String[] args) in C:\Users\david\source\repos\ConsoleApp6\ConsoleApp6\Program.cs:line 7

In addition if an assembly is compiled as targetting Arm64, its AssemblyName should have a ProcessorArchitecture of Arm64.

Risks

Assembly names are a notoriously complex subject, and changing the processing to report Arm64 instead of None could in theory cause some issues. However, I do not believe that simply adding the new enums is much of a risk, and it does provide the ability for us to fix such issues over time.

@davidwrighton davidwrighton added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 10, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Sep 10, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Reflection and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Sep 10, 2021
@jkotas
Copy link
Member

jkotas commented Sep 11, 2021

These enums are effectively deprecated in .NET Core/5+. .NET Core/5+ runs on ARM64 and Wasm just fine without having the respective values in these enums. These enums are not used for anything relevant anymore. I think it would be better to obsolete them instead.

AssemblyName.ProcessorArchitecture is not a proper part of assembly. It does not roundtrip through AssemblyName string representation. It is ignored by the loader in .NET Core. For example, this runs successfully on .NET 6:

    var an = new AssemblyName("System.Diagnostics.Process");
    an.ProcessorArchitecture = ProcessorArchitecture.IA64; // IA64 is Itanium
    Console.WriteLine(Assembly.Load(an.FullName)); // IA64 is not included in the full name
    Console.WriteLine(Assembly.Load(an)); // Works fine on .NET Core (fails on .NET Framework)

AssemblyName type has other similar thumbtacked properties: AssemblyName.VersionCompatibility, AssemblyName.HashAlgorithm and AssemblyName.CodeBase. I think we should obsolete all of them. #42857 is opened on obsoleting AsssemblyName.CodeBase.

@terrajobst
Copy link
Member

Makes sense. Let's not do that then.

@jkotas
Copy link
Member

jkotas commented Sep 13, 2021

AssemblyName type has other similar thumbtacked properties: AssemblyName.VersionCompatibility, AssemblyName.HashAlgorithm

Opened #59061

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Reflection
Projects
No open projects
Development

No branches or pull requests

3 participants