[RISC-V] Update TODOs for RiscV64 Machine and Architecture in Crossgen2 #97021

ashaurtaev · 2024-01-16T10:40:01Z

Part of #96438
cc @wscho77 @HJLeee @clamp03 @JongHeonChoi @t-mustafin @gbalykov @yurai007 @sirntar @tomeksowi @Bajtazar @brucehoult

Merge after #95980

clamp03 · 2024-01-16T12:30:24Z

If you don't mind, please set to draft before #95980 is merged.

tomeksowi · 2024-01-19T08:30:41Z

#95980 is merged, I think you can rebase and re-open for review.

ashaurtaev · 2024-01-22T14:18:53Z

@am11 After rebasing to the current main and installing the new dotnet-sdk-9.0.100-alpha.1.23615.4, I got the following errors:

/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs(157,30): error CS0117: 'Machine' does not contain a definition for 'RiscV64' [/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ILCompiler.Reflection.ReadyToRun.csproj]
/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs(173,30): error CS0117: 'Machine' does not contain a definition for 'RiscV64' [/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ILCompiler.Reflection.ReadyToRun.csproj]
/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs(189,30): error CS0117: 'Machine' does not contain a definition for 'RiscV64' [/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ILCompiler.Reflection.ReadyToRun.csproj]
/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs(207,30): error CS0117: 'Machine' does not contain a definition for 'RiscV64' [/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ILCompiler.Reflection.ReadyToRun.csproj]
/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs(223,30): error CS0117: 'Machine' does not contain a definition for 'RiscV64' [/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ILCompiler.Reflection.ReadyToRun.csproj]
/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/TransitionBlock.cs(37,30): error CS0117: 'Machine' does not contain a definition for 'RiscV64' [/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ILCompiler.Reflection.ReadyToRun.csproj]
/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunReader.cs(636,30): error CS1061: 'Machine' does not contain a definition for 'RiscV64' and no accessible extension method 'RiscV64' accepting a first argument of type 'Machine' could be found (are you missing a using directive or an assembly reference?) [/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ILCompiler.Reflection.ReadyToRun.csproj]
/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunReader.cs(1420,38): error CS1061: 'Machine' does not contain a definition for 'RiscV64' and no accessible extension method 'RiscV64' accepting a first argument of type 'Machine' could be found (are you missing a using directive or an assembly reference?) [/home/runtime/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ILCompiler.Reflection.ReadyToRun.csproj]
    0 Warning(s)
    8 Error(s)

What would cause it and what can be done to fix it?

am11 · 2024-01-22T14:43:13Z

Normally it would mean to update System.Reflection.Metadata version here:

runtime/eng/Versions.props

Line 131 in 86f25ad

to something like: 9.0.0-alpha.1.24064.3 (which we are using in that file). However, this package, in particular, had required some coordination with other teams in the past: dotnet/arcade#10516. Not sure if that coordination is still needed, since sdk is using 8.0 while runtime is at 7.0, and code flow is working fine. @ViktorHofer, thoughts on updating S.R.M to latest alpha?

ViktorHofer · 2024-01-22T14:54:43Z

Not sure if that coordination is still needed, since sdk is using 8.0 while runtime is at 7.0, and code flow is working fine. @ViktorHofer, thoughts on updating S.R.M to latest alpha?

cc @jaredpar @rainersigwald

am11 · 2024-01-22T15:00:49Z

Ops sorry, the more relevant version is hardcoded to 6.0 here:

runtime/src/installer/managed/Microsoft.NET.HostModel/Microsoft.NET.HostModel.csproj

Lines 22 to 24 in 86f25ad

    
           <!-- we need to keep the version of System.Reflection.Metadata in sync with dotnet/msbuild and dotnet/sdk --> 
        
           <PackageReference Include="System.Reflection.Metadata" Version="6.0.0" /> 
        
           <PackageReference Include="System.Text.Json" Version="$(SystemTextJsonVersion)" />

@ashaurtaev, could you push the change with 9.0.0-alpha.1.24064.3 only in eng/Versions.props for SystemReflectionMetadataVersion (since that one is internal to runtime). Lets see how it well bodes with the CI. :)

ViktorHofer · 2024-01-22T15:20:43Z

Which component needs a 9.0 version of SRM? If it's internal then that component might be better off using the live build of SRM.

Regardless, the HostModel version SRM version and others that are hardcoded should probably be updated to 8.0.0.

ashaurtaev · 2024-01-22T15:48:45Z

@am11 I pushed the commit with an updated version of SystemReflectionMetadataVersion

jaredpar · 2024-01-22T16:53:23Z

Not sure if that coordination is still needed, since sdk is using 8.0 while runtime is at 7.0, and code flow is working fine. @ViktorHofer, thoughts on updating S.R.M to latest alpha?

cc @jaredpar @rainersigwald

Not quite sure what you're looking for here from us.

ViktorHofer · 2024-01-22T17:00:11Z

Not quite sure what you're looking for here from us.

We noticed that some components like Microsoft.NET.HostModel are still on an older SRM:

runtime/src/installer/managed/Microsoft.NET.HostModel/Microsoft.NET.HostModel.csproj

Lines 22 to 24 in 86f25ad

    
           <!-- we need to keep the version of System.Reflection.Metadata in sync with dotnet/msbuild and dotnet/sdk --> 
        
           <PackageReference Include="System.Reflection.Metadata" Version="6.0.0" /> 
        
           <PackageReference Include="System.Text.Json" Version="$(SystemTextJsonVersion)" />

Is it important that we version consistently together? Based on HostModel which has been using 6.0.0 for two years now, I guess not. I assume we just can go higher but lower is fine?

jaredpar · 2024-01-22T17:08:34Z

Hard for me to say cause I don't really know what that component is or what it does here.

am11 · 2024-01-22T17:29:07Z

eng/Versions.props

@@ -128,7 +128,7 @@
    <SystemDrawingCommonVersion>8.0.0</SystemDrawingCommonVersion>
    <SystemIOFileSystemAccessControlVersion>5.0.0</SystemIOFileSystemAccessControlVersion>
    <SystemMemoryVersion>4.5.5</SystemMemoryVersion>
-    <SystemReflectionMetadataVersion>7.0.0</SystemReflectionMetadataVersion>
+    <SystemReflectionMetadataVersion>9.0.0-alpha.1.24064.3</SystemReflectionMetadataVersion>


Same version for SystemCollectionsImmutableVersion on line 125.

Thank you! I changed the version for SystemCollectionsImmutableVersion

Could you apply this patch:

diff --git a/eng/Versions.props b/eng/Versions.props index 9eafadc95df..5a46bfb7ad2 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -122,13 +122,14 @@ <MicrosoftWin32RegistryVersion>5.0.0</MicrosoftWin32RegistryVersion> <StyleCopAnalyzersVersion>1.2.0-beta.507</StyleCopAnalyzersVersion> <SystemBuffersVersion>4.5.1</SystemBuffersVersion> - <SystemCollectionsImmutableVersion>9.0.0-alpha.1.24064.3</SystemCollectionsImmutableVersion> + <SystemCollectionsImmutableVersion>7.0.0</SystemCollectionsImmutableVersion> <SystemComponentModelAnnotationsVersion>5.0.0</SystemComponentModelAnnotationsVersion> <SystemDataSqlClientVersion>4.8.5</SystemDataSqlClientVersion> <SystemDrawingCommonVersion>8.0.0</SystemDrawingCommonVersion> <SystemIOFileSystemAccessControlVersion>5.0.0</SystemIOFileSystemAccessControlVersion> <SystemMemoryVersion>4.5.5</SystemMemoryVersion> <SystemReflectionMetadataVersion>9.0.0-alpha.1.24064.3</SystemReflectionMetadataVersion> + <SystemReflectionMetadata_CrossRepoSync_Version>7.0.0</SystemReflectionMetadata_CrossRepoSync_Version> <SystemSecurityAccessControlVersion>6.0.0</SystemSecurityAccessControlVersion> <SystemSecurityCryptographyCngVersion>5.0.0</SystemSecurityCryptographyCngVersion> <SystemSecurityCryptographyOpenSslVersion>5.0.0</SystemSecurityCryptographyOpenSslVersion> diff --git a/src/installer/managed/Microsoft.NET.HostModel/Microsoft.NET.HostModel.csproj b/src/installer/managed/Microsoft.NET.HostModel/Microsoft.NET.HostModel.csproj index cdd3751e3c2..89f6f938536 100644 --- a/src/installer/managed/Microsoft.NET.HostModel/Microsoft.NET.HostModel.csproj +++ b/src/installer/managed/Microsoft.NET.HostModel/Microsoft.NET.HostModel.csproj @@ -20,7 +20,7 @@ <ItemGroup>  - <PackageReference Include="System.Reflection.Metadata" Version="6.0.0" /> + <PackageReference Include="System.Reflection.Metadata" Version="$(SystemReflectionMetadata_CrossRepoSync_Version)" /> <PackageReference Include="System.Text.Json" Version="$(SystemTextJsonVersion)" /> </ItemGroup> diff --git a/src/tasks/Microsoft.NET.WebAssembly.Webcil/Microsoft.NET.WebAssembly.Webcil.csproj b/src/tasks/Microsoft.NET.WebAssembly.Webcil/Microsoft.NET.WebAssembly.Webcil.csproj index d09ae4a569a..677f9dc6150 100644 --- a/src/tasks/Microsoft.NET.WebAssembly.Webcil/Microsoft.NET.WebAssembly.Webcil.csproj +++ b/src/tasks/Microsoft.NET.WebAssembly.Webcil/Microsoft.NET.WebAssembly.Webcil.csproj @@ -17,7 +17,7 @@ <ItemGroup>  <PackageReference Include="System.Memory" Version="$(SystemMemoryVersion)" /> - <PackageReference Include="System.Reflection.Metadata" Version="$(SystemReflectionMetadataVersion)" /> + <PackageReference Include="System.Reflection.Metadata" Version="$(SystemReflectionMetadata_CrossRepoSync_Version)" /> <PackageReference Include="System.Collections.Immutable" Version="$(SystemCollectionsImmutableVersion)" /> </ItemGroup>

I think this will resolve @ViktorHofer's concern.

Don't you need the latest SCI anymore? nit consider SystemReflectionMetadataToolsetVersion over SystemReflectionMetadata_CrossRepoSync_Version as the property name.

Don't you need the latest SCI anymore?

it is only needed by wasm project. Perhaps other consumers of the package would benefit if it’s decoupled as well? (can be done in a separate PR)

Extracted these changes in a separate PR: #97643. @ashaurtaev if that one gets merged first, you can undo the changes here:
git checkout HEAD~3 -- ':/eng'
and merge upstream main.

Thank you! I'll wait for your PR to be merged and then rebase my PR

ViktorHofer · 2024-01-22T17:36:24Z

@am11 there might be dragons, i.e.

runtime/src/tasks/Microsoft.NET.WebAssembly.Webcil/Microsoft.NET.WebAssembly.Webcil.csproj

Lines 18 to 21 in 48ae7e1

    
           <!-- we need to keep the version of System.Reflection.Metadata in sync with dotnet/msbuild and dotnet/sdk --> 
        
           <PackageReference Include="System.Memory" Version="$(SystemMemoryVersion)" />  
        
           <PackageReference Include="System.Reflection.Metadata" Version="$(SystemReflectionMetadataVersion)" /> 
        
           <PackageReference Include="System.Collections.Immutable" Version="$(SystemCollectionsImmutableVersion)" />

I'm not sure if bumping the centrally defined version is the right thing to do.

am11 · 2024-01-22T17:50:00Z

I'm not sure if bumping the centrally defined version is the right thing to do.

Yes, we can pin it the other way around (for now ™️); use 9.0-alpha in ILCompiler.Reflection.ReadyToRun.csproj instead.

Maybe we should add something like SystemReflectionMetadata_CrossRepoSync_Version for Microsoft.NET.HostModel.csproj and Microsoft.NET.WebAssembly.Webcil.csproj? Currently the former has hardcoded version and latter is using central.

ViktorHofer · 2024-01-22T17:52:45Z

Maybe we should add something like SystemReflectionMetadata_CrossRepoSync_Version for Microsoft.NET.HostModel.csproj and Microsoft.NET.WebAssembly.Webcil.csproj? Currently the former has hardcoded version and latter is using central.

Yes I think that would be good. Would you mind opening an issue to track that?

rainersigwald · 2024-01-22T17:52:53Z

The relevant build scenario is:

In .NET Framework MSBuild (or VS):
Some .NET SDK build tasks use HostModel.
HostModel uses core libraries like System.Collections.Immutable and System.Reflection.Metadata.
MSBuild also uses those, and provides binding redirects to the VS version.
This can cause runtime mismatches when the version resolved in the HostModel context (and bumped through MSBuild binding redirects to generally the last released version) doesn't match the version directly referenced by SDK build tasks (generally that's the current version--9.0.0.0 now).

I think if everything in the SDK references a 9.0 version it'll be fine, the problems come up when, for example, HostModel is old or has a stale reference.

ashaurtaev · 2024-01-29T08:47:45Z

@am11, how to proceed? Do I need to wait for #96809 to complete?

am11 · 2024-01-29T09:12:28Z

#97021 (comment) I think this will address @ViktorHofer's concern, and unblock this PR.

ashaurtaev · 2024-02-06T14:32:37Z

Runtime build for riscv64 completed successfully

ashaurtaev · 2024-02-08T10:37:29Z

@am11 @ViktorHofer Please take a look at this pr since #97643 has been merged

ViktorHofer

I'm not the right person to review this change since it touches runtime and the ILCompiler. cc @jkotas @MichalStrehovsky

am11 · 2024-02-08T11:59:29Z

Part of #96438

Should it be Fixes #96438 or is there something still missing?

jkotas

Thanks

ashaurtaev requested a review from MichalStrehovsky as a code owner January 16, 2024 10:40

dotnet-issue-labeler bot added the area-crossgen2-coreclr label Jan 16, 2024

ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 16, 2024

clamp03 assigned ashaurtaev Jan 16, 2024

clamp03 added the arch-riscv Related to the RISC-V architecture label Jan 16, 2024

clamp03 approved these changes Jan 16, 2024

View reviewed changes

gbalykov approved these changes Jan 16, 2024

View reviewed changes

t-mustafin approved these changes Jan 16, 2024

View reviewed changes

tomeksowi approved these changes Jan 16, 2024

View reviewed changes

This was referenced Jan 16, 2024

[browser][MT] various WebWorkerTest CI failures #96628

Closed

[browser][MT] GC deadlock #96190

Closed

ashaurtaev marked this pull request as draft January 16, 2024 14:06

sirntar approved these changes Jan 16, 2024

View reviewed changes

wscho77 approved these changes Jan 16, 2024

View reviewed changes

dkurt mentioned this pull request Jan 19, 2024

Microsoft.NET.Test.Sdk doesn't support RISC-V64 dkurt/dotnet_riscv#8

Open

ashaurtaev force-pushed the update_riscv64_machine_in_crossgen2 branch from abfb512 to 1b0a3d5 Compare January 22, 2024 15:44

am11 reviewed Jan 22, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jan 23, 2024

Tests crashing in CI with no dump: exit code 137 means SIGKILL Killed #97049

Closed

am11 mentioned this pull request Jan 24, 2024

Update pinned System.Reflection.Metadata and System.Collections.Immutable versions to 8.0.0 #96809

Closed

am11 mentioned this pull request Jan 29, 2024

Add separate properties for keep-in-sync versions #97643

Merged

Update TODOs for RiscV64 Machine and Architecture in Crossgen2

73800e9

ashaurtaev force-pushed the update_riscv64_machine_in_crossgen2 branch from 30c1618 to 73800e9 Compare February 6, 2024 13:35

ashaurtaev marked this pull request as ready for review February 6, 2024 14:32

am11 requested a review from jkotas February 8, 2024 11:35

ViktorHofer reviewed Feb 8, 2024

View reviewed changes

jkotas approved these changes Feb 8, 2024

View reviewed changes

jkotas merged commit 5a9e1e7 into dotnet:main Feb 8, 2024
145 of 152 checks passed

ashaurtaev deleted the update_riscv64_machine_in_crossgen2 branch February 8, 2024 14:05

github-actions bot locked and limited conversation to collaborators Mar 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISC-V] Update TODOs for RiscV64 Machine and Architecture in Crossgen2 #97021

[RISC-V] Update TODOs for RiscV64 Machine and Architecture in Crossgen2 #97021

ashaurtaev commented Jan 16, 2024

clamp03 commented Jan 16, 2024

tomeksowi commented Jan 19, 2024

ashaurtaev commented Jan 22, 2024

am11 commented Jan 22, 2024

ViktorHofer commented Jan 22, 2024

am11 commented Jan 22, 2024

ViktorHofer commented Jan 22, 2024

ashaurtaev commented Jan 22, 2024

jaredpar commented Jan 22, 2024

ViktorHofer commented Jan 22, 2024 •

edited

Loading

jaredpar commented Jan 22, 2024

am11 Jan 22, 2024

ashaurtaev Jan 23, 2024

am11 Jan 29, 2024

ViktorHofer Jan 29, 2024 •

edited

Loading

am11 Jan 29, 2024

am11 Jan 29, 2024

ashaurtaev Jan 29, 2024

ViktorHofer commented Jan 22, 2024 •

edited

Loading

am11 commented Jan 22, 2024

ViktorHofer commented Jan 22, 2024

rainersigwald commented Jan 22, 2024

ashaurtaev commented Jan 29, 2024

am11 commented Jan 29, 2024

ashaurtaev commented Feb 6, 2024

ashaurtaev commented Feb 8, 2024

ViktorHofer left a comment

am11 commented Feb 8, 2024

jkotas left a comment

[RISC-V] Update TODOs for RiscV64 Machine and Architecture in Crossgen2 #97021

[RISC-V] Update TODOs for RiscV64 Machine and Architecture in Crossgen2 #97021

Conversation

ashaurtaev commented Jan 16, 2024

clamp03 commented Jan 16, 2024

tomeksowi commented Jan 19, 2024

ashaurtaev commented Jan 22, 2024

am11 commented Jan 22, 2024

ViktorHofer commented Jan 22, 2024

am11 commented Jan 22, 2024

ViktorHofer commented Jan 22, 2024

ashaurtaev commented Jan 22, 2024

jaredpar commented Jan 22, 2024

ViktorHofer commented Jan 22, 2024 • edited Loading

jaredpar commented Jan 22, 2024

am11 Jan 22, 2024

Choose a reason for hiding this comment

ashaurtaev Jan 23, 2024

Choose a reason for hiding this comment

am11 Jan 29, 2024

Choose a reason for hiding this comment

ViktorHofer Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

am11 Jan 29, 2024

Choose a reason for hiding this comment

am11 Jan 29, 2024

Choose a reason for hiding this comment

ashaurtaev Jan 29, 2024

Choose a reason for hiding this comment

ViktorHofer commented Jan 22, 2024 • edited Loading

am11 commented Jan 22, 2024

ViktorHofer commented Jan 22, 2024

rainersigwald commented Jan 22, 2024

ashaurtaev commented Jan 29, 2024

am11 commented Jan 29, 2024

ashaurtaev commented Feb 6, 2024

ashaurtaev commented Feb 8, 2024

ViktorHofer left a comment

Choose a reason for hiding this comment

am11 commented Feb 8, 2024

jkotas left a comment

Choose a reason for hiding this comment

ViktorHofer commented Jan 22, 2024 •

edited

Loading

ViktorHofer Jan 29, 2024 •

edited

Loading

ViktorHofer commented Jan 22, 2024 •

edited

Loading