Skip to content

Add RISCV64 TargetArchitecture #171

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Add RISCV64 TargetArchitecture #171

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d169837
Add RISCV64 TargetArchitecture
clamp03 Mar 25, 2024
ac7c07f
Revert pe64 check
clamp03 Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Mono.Cecil/ModuleKind.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public enum TargetArchitecture {
ARM = 0x01c0,
ARMv7 = 0x01c4,
ARM64 = 0xaa64,
RiscV64 = 0x5064,
Copy link
Member

@am11 am11 Apr 14, 2024

Choose a reason for hiding this comment

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

Maybe add LA64 while touching it:

Suggested change
RiscV64 = 0x5064,
RiscV64 = 0x5064,
LoongArch64 = 0x6264,

Copy link
Member Author

@clamp03 clamp03 Apr 15, 2024

Choose a reason for hiding this comment

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

Thank you. I will add AnyCPU for RiscV64 and LoongArch64 as jkotas reviewed.

Copy link
Member

@jkotas jkotas Apr 14, 2024

Choose a reason for hiding this comment

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

Why do you need to define the enum value when it is no longer used in the rest Cecil?

It is ok to have unnamed enum values.

Copy link
Member

@am11 am11 Apr 14, 2024

Choose a reason for hiding this comment

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

It is checked in the linker here: https://github.com/dotnet/runtime/blob/fa1164c327052042120bf0d3b6fc81e86cfab69d/src/tools/illink/src/linker/Linker.Steps/OutputStep.cs#L79. Perhaps we can remove that validation and cleanup the enum?

Copy link
Member

@jkotas jkotas Apr 14, 2024

Choose a reason for hiding this comment

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

Yep, I have just made the same suggestion above.

Copy link
Member Author

@clamp03 clamp03 Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you so much. I have a question.
If CalculateArchitecture is removed, I think the enum values are not needed for .NET runtime.
As far as I know, cecil is used in other many codes as well as .NET runtime. I am not sure but some codes can use the enum values. Maybe I think it is hard to remove the enum values in jbevain/cecil.
Is it okay to have different public enum from the original cecil?

Copy link
Member

@am11 am11 Apr 15, 2024

Choose a reason for hiding this comment

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

Cecil uses it to determine Windows native image offsets, while linker is using it for Windows PE machine type, which is irrelevant in .NET Core. I think returning 0 for new architectures from https://github.com/dotnet/runtime/blob/fa1164c327052042120bf0d3b6fc81e86cfab69d/src/tools/illink/src/linker/Linker.Steps/OutputStep.cs#L128 (while keeping the existing ones for .NET Framework compat) is more desirable.

See how relaxed the readers are implemented: https://github.com/dotnet/runtime/blob/fa1164c327052042120bf0d3b6fc81e86cfab69d/src/coreclr/System.Private.CoreLib/src/System/Reflection/AssemblyName.CoreCLR.cs#L144 and similar discussion in dotnet/runtime#77697.

I think this patch +/- test adjustment will do it for now:

--- a/runtime/src/tools/illink/src/linker/Linker.Steps/OutputStep.cs
+++ b/runtime/src/tools/illink/src/linker/Linker.Steps/OutputStep.cs
@@ -76,7 +76,8 @@ TargetArchitecture CalculateArchitecture (TargetArchitecture readyToRunArch)
                        if (architectureMap.TryGetValue ((ushort) readyToRunArch, out TargetArchitecture pureILArch)) {
                                return pureILArch;
                        }
-                       throw new BadImageFormatException ("unrecognized module attributes");
+
+                       return 0; // Unknown architecture which ultimately translates to AnyCPU in .NET Core
                }
 
                protected override bool ConditionToProcess ()

Copy link
Member

@jkotas jkotas Apr 15, 2024

Choose a reason for hiding this comment

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

I would go even further, delete the CalculateArchitecture method, and change this line to module.Architecture = 0; // Unknown architecture which ultimately translates to AnyCPU

Copy link
Member Author

@clamp03 clamp03 Apr 15, 2024

Choose a reason for hiding this comment

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

@jkotas @am11 I think I misunderstood something.
My plan was Add AnyCPU to TargetArchitecutre enum in cecil (with enum value or just enum name) and then set TargetArchitecture.AnyCPU to module.Architecutre in the line of runtime code.

I would go even further, delete the CalculateArchitecture method, and change this line to module.Architecture = 0; // Unknown architecture which ultimately translates to AnyCPU

I think it is good. I made a PR. dotnet/runtime#101038
I will close cecil PRs.
Thank you!

}

[Flags]
Expand Down