Skip to content

JIT: Fix register move ordering in genConsumeBlockOp when srcAddr is in dstReg#126133

Closed
Copilot wants to merge 2 commits intomainfrom
copilot/fix-passing-variable-tags-parameters-test-again
Closed

JIT: Fix register move ordering in genConsumeBlockOp when srcAddr is in dstReg#126133
Copilot wants to merge 2 commits intomainfrom
copilot/fix-passing-variable-tags-parameters-test-again

Conversation

Copy link
Contributor

Copilot AI commented Mar 25, 2026

MetricsTests.PassingVariableTagsParametersTest fails under JitStress=2 + JitStressRegs={2,0x80} on Linux x64 because the JIT emits incorrect code for GC-aware block copies (CpObjUnroll) when the LSRA allocates the source address to the same register required for the destination (rdi).

Description

Bug: genConsumeBlockOp unconditionally performs the destination move before the source move. When the source address is in dstReg, this silently overwrites it:

; LSRA under stress: srcAddr=rdi, dstAddr=rax
mov  rdi, rax     ; dst → dstReg — clobbers srcAddr already in rdi!
mov  rsi, rdi     ; src → srcReg — copies destination value, not source!

Fix (src/coreclr/jit/codegenlinear.cpp, genConsumeBlockOp):

  • When the source address (from a GT_IND CopyBlk) is currently in dstReg, emit the source move to srcReg first, then move the destination to dstReg.
  • Add assert(dstAddr->GetRegNum() != srcReg) to guard the unhandled cyclic-swap case (srcAddr in dstReg and dstAddr in srcReg simultaneously), which would need a temp register. The assert makes this fail loudly rather than silently corrupting data.
  • Update the existing comment that incorrectly claimed the LSRA always guarantees interference-free copies to required registers in execution order — this bug disproves that claim.

The fix is generic: it applies to all architectures that use genConsumeBlockOp with fixed destination/source register requirements (REG_RDI/REG_RSI on x64, REG_WRITE_BARRIER_DST_BYREF/REG_WRITE_BARRIER_SRC_BYREF on ARM64/LoongArch64/RISCV64).

Original prompt

This section details on the original issue you should resolve

<issue_title>Test failure: System.Diagnostics.Metrics.Tests.MetricsTests.PassingVariableTagsParametersTest</issue_title>
<issue_description>Failed in: runtime-coreclr libraries-jitstress2-jitstressregs 20260307.1

Failed tests:

net11.0-linux-Release-x64-jitstress2_jitstressregs2-AzureLinux.3.Amd64.Open
- System.Diagnostics.Metrics.Tests.MetricsTests.PassingVariableTagsParametersTest
net11.0-linux-Release-x64-jitstress2_jitstressregs0x80-AzureLinux.3.Amd64.Open
- System.Diagnostics.Metrics.Tests.MetricsTests.PassingVariableTagsParametersTest

Error message:

Microsoft.DotNet.RemoteExecutor.RemoteExecutionException : Remote process failed with an unhandled exception.

Stack trace:


Child exception:
  Xunit.Sdk.EqualException: Assert.Equal() Failure: Collections differ
           ↓ (pos 0)
Expected: [["K1"] = "V1", ["K2"] = "V2", ["K3"] = "V3", ["K4"] = "V4", ["K5"] = "V5", ···]
Actual:   [[null] = null, ["K2"] = "V2", ["K3"] = "V3", ["K4"] = "V4", ["K5"] = "V5", ···]
           ↑ (pos 0)
   at System.Diagnostics.Metrics.Tests.MetricsTests.<>c__DisplayClass32_0`1.<InstrumentPassingVariableTagsParametersTest>b__1(Instrument inst, T measurement, ReadOnlySpan`1 tags, Object state) in /_/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs:line 1965
   at System.Diagnostics.Metrics.Instrument`1.RecordMeasurement(T measurement, ReadOnlySpan`1 tags) in /_/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.common.cs:line 99
   at System.Diagnostics.Metrics.Tests.MetricsTests.PublishCounterMeasurement[T](Counter`1 counter, T value, KeyValuePair`2[] tags) in /_/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs:line 1791
   at System.Diagnostics.Metrics.Tests.MetricsTests.<PassingVariableTagsParametersTest>b__10_3(Instrument`1 instrument, Int32 value, KeyValuePair`2[] tags) in /_/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs:line 751
   at System.Diagnostics.Metrics.Tests.MetricsTests.InstrumentPassingVariableTagsParametersTest[T](Instrument`1 instrument, Func`4 record) in /_/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs:line 2015
   at System.Diagnostics.Metrics.Tests.MetricsTests.<PassingVariableTagsParametersTest>b__10_0() in /_/src/libraries/System.Diagnostics.DiagnosticSource/tests/MetricsTests.cs:line 749
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in /_/src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.CoreCLR.cs:line 36
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.cs:line 57

Child process:
  System.Diagnostics.DiagnosticSource.Tests, Version=11.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Diagnostics.Metrics.Tests.MetricsTests Void <PassingVariableTagsParametersTest>b__10_0()
```</issue_description>

<agent_instructions>Please make a fix based on the analysis on the issue. </agent_instructions>

## Comments on the Issue (you are @copilot in this section)

<comments>
<comment_new><author>@mangod9</author><body>
Here is some copilot analysis for this failure:

```text 
Crash Context

  ┌────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Field      │ Value                                                                                             │
  ├────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ Build      │ runtime-coreclr libraries-jitstress2-jitstressregs 20260307.1                                     │
  │            │ (https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_buil                   │
  │            │ d/results?buildId=1324993)                                                                        │
  ├────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ Helix Job  │ 3f3cc108-6550-42e6-80a5-5e70ed9c556a                                                              │
  │ (regs2)    │                                                                                                   │
  ├────────────┼───────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ Helix Job  │ acfbbeb5-9555-4c5b-b5ea-892f475ef22b                                                              │
  │ (regs0x80) │                                                                                                   │
  ├────────────┼───────────────────────────────────────────────────────────────────────...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes dotnet/runtime#125312

<!-- START COPILOT CODING AGENT TIPS -->
---

⌨️ Start Copilot coding agent tasks without leaving your editor — available in [VS Code](https://gh.io/cca-vs-code-docs), [Visual Studio](https://gh.io/cca-visual-studio-docs), [JetBrains IDEs](https://gh.io/cca-jetbrains-docs) and [Eclipse](https://gh.io/cca-eclipse-docs).

@github-actions github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 25, 2026
@dotnet-policy-service
Copy link
Contributor

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

When the LSRA allocates the source address to the same register that
the destination address needs (dstReg), the original code would
incorrectly clobber the source address by copying the destination first:

  mov  rdi, rax   ; dst -> dstReg (CLOBBERS srcAddr already in rdi!)
  mov  rsi, rdi   ; src -> srcReg (copies destination, not source!)

Fix by detecting this case and moving the source to srcReg first when
srcAddr is currently in dstReg. Also update the existing comment which
incorrectly claimed the LSRA always guarantees safe copy ordering, and
add an assert to catch the unhandled swap case (srcAddr in dstReg AND
dstAddr in srcReg simultaneously) which would produce incorrect code.

Fixes: System.Diagnostics.Metrics.Tests.MetricsTests.PassingVariableTagsParametersTest
under JitStress=2 + JitStressRegs={2,0x80} on Linux x64.

Co-authored-by: mangod9 <61718172+mangod9@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1b13221b-a0c1-4742-8445-5ada9974b0de
Copilot AI changed the title [WIP] Fix test failure in PassingVariableTagsParametersTest JIT: Fix register move ordering in genConsumeBlockOp when srcAddr is in dstReg Mar 25, 2026
Copilot AI requested a review from mangod9 March 25, 2026 23:48
@jakobbotsch
Copy link
Member

I don't think this is the right fix. Likely what I am working on in #125333 will fix this in a better way.

@mangod9
Copy link
Member

mangod9 commented Mar 26, 2026

Ok will close this PR then if you are already working on a fix. Thx!

@mangod9
Copy link
Member

mangod9 commented Mar 26, 2026

btw @jakobbotsch is the analysis on the issue incorrect or the proposed fix is not complete. Looks like as part of the analysis it validated the fix worked for a sample test case it had built.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants