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

[RISC-V] JIT: Fix emitInsMayWriteToGCReg #110390

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tomeksowi
Copy link
Contributor

@tomeksowi tomeksowi commented Dec 4, 2024

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:


with Dispose() inlined from here:
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

@dotnet-issue-labeler 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 dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 4, 2024
@risc-vv
Copy link

risc-vv commented Dec 4, 2024

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=

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-riscv Related to the RISC-V architecture area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants