Add RISCV64 TargetArchitecture

[clamp03]

It is for supporting RISCV64 in cecil
Add RISCV64 to TargetArchitecture and update to check for pe64

Part of dotnet/runtime#84834
cc @dotnet/samsung

[jkotas]

In System.Reflection.Metadata, we have been emitting PE64 header only for architectures that are natively supported by Windows. PE64 header is unnecessary for IL-only files outside Windows.

Would it be better to adopt similar strategy for Cecil - flip non-Windows architectures originating in R2R file to AnyCPU when writing the IL-only file? It would save us from introducing this ever-growing list of 64-bit architectures.

[clamp03]

Sorry for late response. I just removed the check codes for pe64. As far as I understand, pe64 is not needed to be set to true for riscv-64 because pe64 header is unnecessary for IL-only on non-Windows. Am I right?

And I cannot understand flip non-Windows architectures originating in R2R file to AnyCPU when writing the IL-only file? well. Does it mean that cecil replace arch to AnyCPU on the case (non-Windows architectures originating in R2R file when writing the IL-only file)? If then, it is not a scope which I planned in this PR.

[jkotas]

pe64 header is unnecessary for IL-only on non-Windows. Am I right?

Right. pe64 header is only necessary for 64-bit architecture specific images.

Does it mean that cecil replace arch to AnyCPU on the case

I assume that the problem you are running into is the exception at https://github.com/dotnet/runtime/blob/fa1164c327052042120bf0d3b6fc81e86cfab69d/src/tools/illink/src/linker/Linker.Steps/OutputStep.cs#L79 .

This logic is not quite right (it won't compute the architecture of the IL image that the crossgened image was created from) and too complicated for not good reason.

It would best to delete this logic and instead change the output for crossgened input binaries to always be AnyCPU here: https://github.com/dotnet/runtime/blob/fa1164c327052042120bf0d3b6fc81e86cfab69d/src/tools/illink/src/linker/Linker.Steps/OutputStep.cs#L128

[am11]

Maybe add LA64 while touching it:

Suggested change

	RiscV64 = 0x5064,
	RiscV64 = 0x5064,
	LoongArch64 = 0x6264,

[clamp03]

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

[jkotas]

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.

[am11]

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?

[jkotas]

Yep, I have just made the same suggestion above.

[clamp03]

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?

[am11]

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 ()

[jkotas]

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

[clamp03]

@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!