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 public Architecture enum value for LoongArch64 #63042

Closed
huoyaoyuan opened this issue Dec 21, 2021 · 10 comments
Closed

[API Proposal]: Add public Architecture enum value for LoongArch64 #63042

huoyaoyuan opened this issue Dec 21, 2021 · 10 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices blocking Marks issues that we want to fast track in order to unblock other important work

Comments

@huoyaoyuan
Copy link
Member

huoyaoyuan commented Dec 21, 2021

Background and motivation

Developers from Loongnix are going to add support for a new architecture LoongArch64 to .NET. The enums for architectures should be extended to support it. The implementation PR is #62888.
Since it's also defined in Windows PE spec, an enum value for PE machine type could also be added.

API Proposal

namespace System.Runtime.InteropServices
{
    public enum Architecture
    {
        X86,
        X64,
        Arm,
        Arm64,
        Wasm,
        S390x,
+        LoongArch64,
    }
}
namespace System.Reflection.PortableExecutable
{
    public enum Machine : ushort
    {
+        LoongArch32 = 0x6232,
+        LoongArch64 = 0x6264
    }
}

API Usage

Code can check if they are running on LoongArch64 architecture.

Alternative Designs

LoongArch32 is also defined in Windows PE spec. Do we need to include it too?

Risks

No response

@huoyaoyuan huoyaoyuan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 21, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Dec 21, 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.

@huoyaoyuan
Copy link
Member Author

Marking as ready for review & blocking since implementation for the arch is ready.

@huoyaoyuan huoyaoyuan added api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Runtime.InteropServices blocking Marks issues that we want to fast track in order to unblock other important work 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 Dec 21, 2021
@jkotas
Copy link
Member

jkotas commented Dec 21, 2021

LoongArch32 is also defined in Windows PE spec. Do we need to include it too?

Yes, it should be included in the Machine enum. The Machine enum is meant to mirror the Windows PE spec. I have edited the proposal above.

@terrajobst
Copy link
Member

terrajobst commented Jan 4, 2022

Video

  • Looks good as proposed
  • We talked about adding an Unknown member to Architecture but decided against it; the implementation can still return a placeholder (e.g. -1).
namespace System.Runtime.InteropServices
{
    public enum Architecture
    {
        // Existing:
        // X86
        // X64
        // Arm
        // Arm64
        // Wasm
        // S390x
        LoongArch64,
    }
}
namespace System.Reflection.PortableExecutable
{
    public enum Machine : ushort
    {
        // Existing:
        // Alpha
        // Alpha64
        // AM33
        // Amd64
        // Arm
        // Arm64
        // ArmThumb2
        // Ebc
        // I386
        // IA64
        // M32R
        // MIPS16
        // MipsFpu
        // MipsFpu16
        // PowerPC
        // PowerPCFP
        // SH3
        // SH3Dsp
        // SH3E
        // SH4
        // SH5
        // Thumb
        // Tricore
        // Unknown
        // WceMipsV2
        LoongArch32 = 0x6232,
        LoongArch64 = 0x6264
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 4, 2022
@GrabYourPitchforks
Copy link
Member

@terrajobst @jkotas @AaronRobinsonMSFT As discussed in API review, we're likely to become an IANA-like registry of these architectures. We should strongly consider publishing formal criteria by which new entries can be added to this enum. Who would have ownership of defining & publishing such criteria?

@jkotas
Copy link
Member

jkotas commented Jan 4, 2022

We have a high-level guide for what it takes to create CoreCLR port at: https://github.com/dotnet/runtime/blob/main/docs/design/coreclr/botr/guide-for-porting.md

We have number of places that require boilerplate changes for new architecture: build scripts, Arcade repo, RID graph, public architecture enum, etc. We expect people working on new ports to do these changes roughly in order. For example, if somebody starts the port by adding the architecture to managed public surface, we will kindly ask them to start with the build scripts first.

Our contribution guidelines encourage new ports: https://github.com/dotnet/runtime/blob/main/CONTRIBUTING.md#contributing-ports. We do not have a formal criteria for starting new ports and I do not think we need one. Anybody with enough motivation can start a new port and we will be happy to work with them.

@huoyaoyuan
Copy link
Member Author

@shushanhf The enum values has been approved and you can update in your PR.

@terrajobst
Copy link
Member

terrajobst commented Jan 5, 2022

@GrabYourPitchforks

As discussed in API review, we're likely to become an IANA-like registry of these architectures. We should strongly consider publishing formal criteria by which new entries can be added to this enum.

I'm OK with that bar being "hey, we're actively working on a port to <architecture X> and thus request an addition to the architecture enums". That bar seems sufficiently high and I don't see a lot of evidence that would suggest this might result in API pollution.

In order to keep things simple for all involved parties, I'm OK with approving these APIs as we go, without asking people to finish their port first because as @jkotas suggests some of these enums are part of the infrastructure that is used to target these architectures.

Please note that there are other examples, such as RID entries and TFMs in NuGet. The situation there is similar, e.g. we recentally added nanoFramework as a TFM. I think we're doing us and the community a service by having a relatively straight-forward process for requesting additions in order to unblock community forks. The alternative would be to invest in decentralized mechanisms (like what we'd like to do with the RID graph) but that's not always workable.

Does this make sense?

@shushanhf
Copy link
Contributor

@shushanhf The enum values has been approved and you can update in your PR.

Thanks.

@huoyaoyuan
Copy link
Member Author

Closing since the implementation is merged.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

No branches or pull requests

5 participants