[RISC-V] JIT: Fix emitInsMayWriteToGCReg #110390

tomeksowi · 2024-12-04T11:50:14Z

emitter::emitInsMayWriteToGCReg didn't account for some instructions which broke GC info.

This fixes an intermittent segfault (~3 fails out of 6400 runs) with GCStress=0x3 in Loader/classloader/explicitlayout/objrefandnonobjrefoverlap/case7. The direct cause was the GC ref register loaded in Interlocked.Exchange being reported too late, e.g. here:

runtime/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/MemoryMappedFileBlock.cs

Line 51 in 949c3ec

Interlocked.Exchange(ref _accessor, null)?.Dispose();

with Dispose() inlined from here:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/UnmanagedMemoryAccessor.cs

Lines 103 to 104 in 949c3ec

    
           Dispose(true); 
        
           GC.SuppressFinalize(this);

The asm diff with GC info:

@@ -300,30 +300,31 @@ G_M43658_IG03:        ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
                              ; byrRegs +[a1]
 00007C      mv             a2, zero
 000080      amoswap.d.aqrl s1, a2, (a1)
+                             ; gcrRegs +[s1]
 000084      beqz           s1, G_M43658_IG04
 000088      ld             a1, 0(s1)
                              ; byrRegs -[a1]
 00008C      lui            ra, 520356
 000090      addiw          ra, ra, -1628
 000094      slli           ra, ra, 7
 000098      addi           ra, ra, 40
 00009C      bne            a1, ra, G_M43658_IG07
 0000A0      mv             a0, s1
 0000A4      addi           a1, zero, 1
 0000A8      lui            a2, 520356
 0000AC      addiw          a2, a2, -1627
 0000B0      slli           a2, a2, 7
 0000B4      addi           a2, a2, 72
 0000B8      ld             a2, 0(a2)
 0000BC      jalr           ra, 0(a2)           // System.IO.MemoryMappedFiles.MemoryMappedViewAccessor:Dispose(ubyte):this
-                             ; gcrRegs -[a0] +[s1]
+                             ; gcrRegs -[a0]
                              ; gcr arg pop 0
 0000C0      mv             a0, s1
                              ; gcrRegs +[a0]
 0000C4      lui            a1, 262177
 0000C8      addiw          a1, a1, -346
 0000CC      slli           a1, a1, 8
 0000D0      addi           a1, a1, 116
 0000D4      jalr           ra, 0(a1)           // System.GC:_SuppressFinalize(System.Object)
                              ; gcrRegs -[s1-a0]
                              ; gcr arg pop 0

Part of #84834, cc @dotnet/samsung

…s than indices

risc-vv · 2024-12-04T11:50:57Z

RISC-V Release-CLR-QEMU: 9436 / 9457 (99.78%)

=======================
      passed: 9436
      failed: 4
     skipped: 107
      killed: 17
------------------------
  TOTAL libs: 9564
 TOTAL tests: 9564
   REAL time: 57min 38s 176ms
=======================

Release-CLR-QEMU.md, Release-CLR-QEMU.xml, testclr_output.tar.gz

RISC-V Release-FX-QEMU: 567961 / 594846 (95.48%)

=======================
      passed: 567961
      failed: 244
     skipped: 1517
      killed: 26641
------------------------
  TOTAL libs: 256
 TOTAL tests: 596363
   REAL time: 2h 24min 48s 219ms
=======================

Release-FX-QEMU.md, Release-FX-QEMU.xml, testfx_output.tar.gz

Build information and commands

GIT: f1067fb09c889f9288d597d0bc1d23b8f1e6331b
CI: 64a5c905f307e2e98dbf9ad1d31e22d0eea08eed
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

# CORE_LIBS_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s libs /p:EnableSourceLink=false
# CORE_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s clr+libs+host /p:EnableSourceLink=false

# TESTCLR_BUILD_CMD
runtime/src/tests/build.sh -riscv64 -cross -Release -priority1 -p:UsePublishedCrossgen2=false -p:UseLocalAppHostPack=true
# TESTCLR_CMD
python3 riscv-CI/goci/agent/TestRunner/run.py --core_root ./coreclr.Release/Tests/Core_Root --testhost ./testhost.Release --atest ./coreclr.Release --test ./ --log_dir ./logs  --timeout 2700 --log_level DEBUG --xunit xunit.Release
# TESTCLR_RUN
/godata/pipelines/Release-CLR-QEMU/logs/run_tests.log
cp -R /godata/pipelines/Release-CLR-QEMU/xunit.Release "/_PATH_/_WITH_/_TEST_"/ ; cd "/_PATH_/_WITH_/_TEST_" && ROOTFS_DIR=/crossrootfs/riscv64 QEMU_LD_PREFIX=/crossrootfs/riscv64 __TestDotNetCmd=/godata/pipelines/Release-CLR-QEMU/testhost.Release/dotnet CORE_ROOT=/godata/pipelines/Release-CLR-QEMU/coreclr.Release/Tests/Core_Root  /usr/bin/time -f "exec_time: %e" ./_TEST_BINARY_

# TESTFX_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -rc Release -hc Release -lc Release -s libs.tests --testscope innerloop /p:EnableSourceLink=false /p:UseLocalAppHostPack=true
# TESTFX_CMD
unknown command
# TESTFX_RUN
unknown command

# TEST_ENV
DOTNET_JitStress=;DOTNET_JitStressRegs=;DOTNET_GCStress=;DOTNET_JITMinOpts=;DOTNET_TailcallStress=;DOTNET_TieredCompilation=

RISC-V Release-CLR-VF2: 9436 / 9457 (99.78%)

=======================
      passed: 9436
      failed: 4
     skipped: 107
      killed: 17
------------------------
  TOTAL libs: 9564
 TOTAL tests: 9564
   REAL time: 3h 3min 36s 25ms
=======================

Release-CLR-VF2.md, Release-CLR-VF2.xml, testclr_output.tar.gz

RISC-V Release-FX-VF2: 587473 / 627437 (93.63%)

=======================
      passed: 587473
      failed: 78
     skipped: 1402
      killed: 39886
------------------------
  TOTAL libs: 256
 TOTAL tests: 628839
   REAL time: 2h 52min 16s 635ms
=======================

Release-FX-VF2.md, Release-FX-VF2.xml, testfx_output.tar.gz

Build information and commands

GIT: f1067fb09c889f9288d597d0bc1d23b8f1e6331b
CI: 64a5c905f307e2e98dbf9ad1d31e22d0eea08eed
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

# CORE_LIBS_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s libs /p:EnableSourceLink=false
# CORE_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s clr+libs+host /p:EnableSourceLink=false

# TESTCLR_BUILD_CMD
runtime/src/tests/build.sh -riscv64 -cross -Release -priority1 -p:UsePublishedCrossgen2=false -p:UseLocalAppHostPack=true
# TESTCLR_CMD
python3 riscv-CI/goci/agent/TestRunner/run.py --core_root ./coreclr.Release/Tests/Core_Root --testhost ./testhost.Release --atest ./coreclr.Release --test ./ --log_dir ./logs  --timeout 2700 --log_level DEBUG --xunit xunit.Release
# TESTCLR_RUN
/var/lib/go-agent/pipelines/Release-CLR-VF2/logs/run_tests.log
cp -R /var/lib/go-agent/pipelines/Release-CLR-VF2/xunit.Release "/_PATH_/_WITH_/_TEST_"/ ; cd "/_PATH_/_WITH_/_TEST_" && __TestDotNetCmd=/var/lib/go-agent/pipelines/Release-CLR-VF2/testhost.Release/dotnet CORE_ROOT=/var/lib/go-agent/pipelines/Release-CLR-VF2/coreclr.Release/Tests/Core_Root  /usr/bin/time -f "exec_time: %e" ./_TEST_BINARY_

# TESTFX_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -rc Release -hc Release -lc Release -s libs.tests --testscope innerloop /p:EnableSourceLink=false /p:UseLocalAppHostPack=true
# TESTFX_CMD
unknown command
# TESTFX_RUN
unknown command

# TEST_ENV
DOTNET_JitStress=;DOTNET_JitStressRegs=;DOTNET_GCStress=;DOTNET_JITMinOpts=;DOTNET_TailcallStress=;DOTNET_TieredCompilation=

RISC-V Release-CLR-QEMU: 9440 / 9461 (99.78%)

=======================
      passed: 9440
      failed: 4
     skipped: 107
      killed: 17
------------------------
  TOTAL libs: 9568
 TOTAL tests: 9568
   REAL time: 57min 21s 543ms
=======================

Release-CLR-QEMU.md, Release-CLR-QEMU.xml, testclr_output.tar.gz

RISC-V Release-FX-QEMU: 567961 / 594846 (95.48%)

=======================
      passed: 567961
      failed: 244
     skipped: 1517
      killed: 26641
------------------------
  TOTAL libs: 256
 TOTAL tests: 596363
   REAL time: 2h 24min 48s 219ms
=======================

Release-FX-QEMU.md, Release-FX-QEMU.xml, testfx_output.tar.gz

Build information and commands

GIT: f1067fb09c889f9288d597d0bc1d23b8f1e6331b
CI: 64a5c905f307e2e98dbf9ad1d31e22d0eea08eed
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

# CORE_LIBS_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s libs /p:EnableSourceLink=false
# CORE_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s clr+libs+host /p:EnableSourceLink=false

# TESTCLR_BUILD_CMD
runtime/src/tests/build.sh -riscv64 -cross -Release -priority1 -p:UsePublishedCrossgen2=false -p:UseLocalAppHostPack=true
# TESTCLR_CMD
python3 riscv-CI/goci/agent/TestRunner/run.py --core_root ./coreclr.Release/Tests/Core_Root --testhost ./testhost.Release --atest ./coreclr.Release --test ./ --log_dir ./logs  --timeout 2700 --log_level DEBUG --xunit xunit.Release
# TESTCLR_RUN
/godata/pipelines/Release-CLR-QEMU/logs/run_tests.log
cp -R /godata/pipelines/Release-CLR-QEMU/xunit.Release "/_PATH_/_WITH_/_TEST_"/ ; cd "/_PATH_/_WITH_/_TEST_" && ROOTFS_DIR=/crossrootfs/riscv64 QEMU_LD_PREFIX=/crossrootfs/riscv64 __TestDotNetCmd=/godata/pipelines/Release-CLR-QEMU/testhost.Release/dotnet CORE_ROOT=/godata/pipelines/Release-CLR-QEMU/coreclr.Release/Tests/Core_Root  /usr/bin/time -f "exec_time: %e" ./_TEST_BINARY_

# TESTFX_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -rc Release -hc Release -lc Release -s libs.tests --testscope innerloop /p:EnableSourceLink=false /p:UseLocalAppHostPack=true
# TESTFX_CMD
unknown command
# TESTFX_RUN
unknown command

# TEST_ENV
DOTNET_JitStress=;DOTNET_JitStressRegs=;DOTNET_GCStress=;DOTNET_JITMinOpts=;DOTNET_TailcallStress=;DOTNET_TieredCompilation=

RISC-V Release-CLR-VF2: 9440 / 9461 (99.78%)

=======================
      passed: 9440
      failed: 4
     skipped: 107
      killed: 17
------------------------
  TOTAL libs: 9568
 TOTAL tests: 9568
   REAL time: 3h 7min 53s 763ms
=======================

Release-CLR-VF2.md, Release-CLR-VF2.xml, testclr_output.tar.gz

RISC-V Release-FX-VF2: 587473 / 627437 (93.63%)

=======================
      passed: 587473
      failed: 78
     skipped: 1402
      killed: 39886
------------------------
  TOTAL libs: 256
 TOTAL tests: 628839
   REAL time: 2h 52min 16s 635ms
=======================

Release-FX-VF2.md, Release-FX-VF2.xml, testfx_output.tar.gz

Build information and commands

GIT: f1067fb09c889f9288d597d0bc1d23b8f1e6331b
CI: 64a5c905f307e2e98dbf9ad1d31e22d0eea08eed
REPO: dotnet/runtime
BRANCH: main
CONFIG: Release
LIB_CONFIG: Release

# CORE_LIBS_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s libs /p:EnableSourceLink=false
# CORE_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -s clr+libs+host /p:EnableSourceLink=false

# TESTCLR_BUILD_CMD
runtime/src/tests/build.sh -riscv64 -cross -Release -priority1 -p:UsePublishedCrossgen2=false -p:UseLocalAppHostPack=true
# TESTCLR_CMD
python3 riscv-CI/goci/agent/TestRunner/run.py --core_root ./coreclr.Release/Tests/Core_Root --testhost ./testhost.Release --atest ./coreclr.Release --test ./ --log_dir ./logs  --timeout 2700 --log_level DEBUG --xunit xunit.Release
# TESTCLR_RUN
/var/lib/go-agent/pipelines/Release-CLR-VF2/logs/run_tests.log
cp -R /var/lib/go-agent/pipelines/Release-CLR-VF2/xunit.Release "/_PATH_/_WITH_/_TEST_"/ ; cd "/_PATH_/_WITH_/_TEST_" && __TestDotNetCmd=/var/lib/go-agent/pipelines/Release-CLR-VF2/testhost.Release/dotnet CORE_ROOT=/var/lib/go-agent/pipelines/Release-CLR-VF2/coreclr.Release/Tests/Core_Root  /usr/bin/time -f "exec_time: %e" ./_TEST_BINARY_

# TESTFX_BUILD_CMD
runtime/build.sh --arch riscv64 --cross -c Release -rc Release -hc Release -lc Release -s libs.tests --testscope innerloop /p:EnableSourceLink=false /p:UseLocalAppHostPack=true
# TESTFX_CMD
unknown command
# TESTFX_RUN
unknown command

# TEST_ENV
DOTNET_JitStress=;DOTNET_JitStressRegs=;DOTNET_GCStress=;DOTNET_JITMinOpts=;DOTNET_TailcallStress=;DOTNET_TieredCompilation=

dotnet-policy-service · 2024-12-04T11:50:57Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

sirntar

LGTM

sirntar · 2024-12-13T12:47:45Z

src/coreclr/jit/emitriscv64.cpp

+        case 0b00001: // LOAD-FP
+        case 0b01000: // STORE
+        case 0b01001: // STORE-FP
+        case 0b00011: // MISC-MEM
+        case 0b10000: // MADD
+        case 0b10001: // MSUB
+        case 0b10010: // NMSUB
+        case 0b10011: // NMADD
+        case 0b11000: // BRANCH
+        case 0b11100: // SYSTEM
+            return false;


nit: not exactly in order, but in this case it doesn't matter

tomeksowi · 2024-12-20T09:14:53Z

@jakobbotsch @dotnet/jit-contrib Checks successful, can this be merged?

tomeksowi added 2 commits December 4, 2024 10:57

Remove instructions from privileged ISA

b0dbe5e

Fix emitInsMayWriteToGCReg; it's easier to inspect instruction opcode…

f1067fb

…s than indices

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 4, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 4, 2024

clamp03 assigned tomeksowi Dec 4, 2024

clamp03 added the arch-riscv Related to the RISC-V architecture label Dec 4, 2024

build-analysis bot mentioned this pull request Dec 4, 2024

iOS tests failing with WORKLOAD TIMED OUT - Killing user command. #108103

Open

sirntar approved these changes Dec 13, 2024

View reviewed changes

clamp03 approved these changes Dec 16, 2024

View reviewed changes

clamp03 requested a review from jakobbotsch December 16, 2024 01:45

jakobbotsch approved these changes Dec 16, 2024

View reviewed changes

jakobbotsch closed this Dec 16, 2024

jakobbotsch reopened this Dec 16, 2024

jakobbotsch merged commit 347a568 into dotnet:main Dec 20, 2024
109 of 110 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RISC-V] JIT: Fix emitInsMayWriteToGCReg #110390

[RISC-V] JIT: Fix emitInsMayWriteToGCReg #110390

tomeksowi commented Dec 4, 2024 •

edited

Loading

risc-vv commented Dec 4, 2024 •

edited

Loading

dotnet-policy-service bot commented Dec 4, 2024

sirntar left a comment

sirntar Dec 13, 2024

tomeksowi commented Dec 20, 2024

[RISC-V] JIT: Fix emitInsMayWriteToGCReg #110390

[RISC-V] JIT: Fix emitInsMayWriteToGCReg #110390

Conversation

tomeksowi commented Dec 4, 2024 • edited Loading

risc-vv commented Dec 4, 2024 • edited Loading

dotnet-policy-service bot commented Dec 4, 2024

sirntar left a comment

Choose a reason for hiding this comment

sirntar Dec 13, 2024

Choose a reason for hiding this comment

tomeksowi commented Dec 20, 2024

tomeksowi commented Dec 4, 2024 •

edited

Loading

risc-vv commented Dec 4, 2024 •

edited

Loading