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

CollectionsMarshal.AsSpan assertion failure in System.Text.Json tests #96876

Closed
jkotas opened this issue Jan 12, 2024 · 17 comments · Fixed by #97062
Closed

CollectionsMarshal.AsSpan assertion failure in System.Text.Json tests #96876

jkotas opened this issue Jan 12, 2024 · 17 comments · Fixed by #97062
Labels
area-VM-coreclr blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab
Milestone

Comments

@jkotas
Copy link
Member

jkotas commented Jan 12, 2024

Build Information

Build: https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=524311
Build error leg or test failing: System.Text.Json.Tests.WorkItemExecution
Pull request: #96870

Error Message

Fill the error message using step by step known issues guidance.

{
  "ErrorMessage": "Implementation depends on List<T> always using a T[] and not U[] where U : T.",
  "ErrorPattern": "",
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}

Known issue validation

Build: 🔎 https://dev.azure.com/dnceng-public/public/_build/results?buildId=524311
Error message validated: Implementation depends on List<T> always using a T[] and not U[] where U : T.
Result validation: ✅ Known issue matched with the provided build.
Validation performed at: 1/12/2024 1:31:48 AM UTC

Report

Build Definition Test Pull Request
524882 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #95583
525560 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96609
526100 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96880
525785 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96913
525988 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96928
525991 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #89472
525957 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96927
525923 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96926
525892 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96201
525842 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96924
525765 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96921
525686 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96916
525621 dotnet/runtime System.Text.Json.Tests.WorkItemExecution
525570 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96880
525477 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96805
525444 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96867
525362 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96880
525344 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96741
525094 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96894
525249 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96896
525236 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96445
525231 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96890
525196 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96214
525140 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96676
525086 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96893
525001 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96741
524968 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96892
524959 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96558
524862 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96889
524798 dotnet/runtime System.Text.Json.Tests.WorkItemExecution
524790 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #95583
524621 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96860
524464 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96805
524458 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96867
524255 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96829
524448 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96858
524210 dotnet/runtime System.Text.Json.Tests.WorkItemExecution
524311 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96870
524291 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96867
524193 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96858
524168 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96805
524110 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96795
524101 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96699

Summary

24-Hour Hit Count 7-Day Hit Count 1-Month Count
0 43 43
@jkotas jkotas added blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab labels Jan 12, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 12, 2024
@ghost
Copy link

ghost commented Jan 12, 2024

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

Build Information

Build: https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=524311
Build error leg or test failing: System.Text.Json.Tests.WorkItemExecution
Pull request: #96870

Error Message

Fill the error message using step by step known issues guidance.

{
  "ErrorMessage": "Implementation depends on List<T> always using a T[] and not U[] where U : T.",
  "ErrorPattern": "",
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}
Author: jkotas
Assignees: -
Labels:

area-Infrastructure-libraries, blocking-clean-ci, Known Build Error

Milestone: -

@jkotas
Copy link
Member Author

jkotas commented Jan 12, 2024

cc @stephentoub

@stephentoub
Copy link
Member

bleh, the change is flawed. will revert and rethink.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 12, 2024
@jkotas
Copy link
Member Author

jkotas commented Jan 12, 2024

bleh, the change is flawed. will revert and rethink.

Where is the flaw?

@stephentoub
Copy link
Member

Hmm, I was thinking for a moment it would still need to check and actually throw, but with lists being invariant, it shouldn't be possible to specify a T and pass in a List of a derived type. So now I don't know what's going on and will need to look at what the failing test is doing, but I'm away from my computer through the weekend.

@ghost
Copy link

ghost commented Jan 12, 2024

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Build Information

Build: https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=524311
Build error leg or test failing: System.Text.Json.Tests.WorkItemExecution
Pull request: #96870

Error Message

Fill the error message using step by step known issues guidance.

{
  "ErrorMessage": "Implementation depends on List<T> always using a T[] and not U[] where U : T.",
  "ErrorPattern": "",
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}

Known issue validation

Build: 🔎 https://dev.azure.com/dnceng-public/public/_build/results?buildId=524311
Error message validated: Implementation depends on List<T> always using a T[] and not U[] where U : T.
Result validation: ✅ Known issue matched with the provided build.
Validation performed at: 1/12/2024 1:31:48 AM UTC

Report

Build Definition Test Pull Request
524464 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96805
524458 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96867
524255 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96829
524448 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96858
524210 dotnet/runtime System.Text.Json.Tests.WorkItemExecution
524311 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96870
524291 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96867
524193 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96858
524168 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96805
524110 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96795
524101 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96699

Summary

24-Hour Hit Count 7-Day Hit Count 1-Month Count
11 11 11
Author: jkotas
Assignees: -
Labels:

area-System.Text.Json, blocking-clean-ci, untriaged, in-pr, Known Build Error

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Jan 12, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 12, 2024
@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Jan 12, 2024
@jkotas
Copy link
Member Author

jkotas commented Jan 12, 2024

I am not able to reproduce the failure. It seems to be Linux Alpine Arm64 specific.

@Raphtaliyah
Copy link
Contributor

I've tried a few things to reproduce it with no success. However, it reproduces on checked builds on x64 Linux (ubuntu!) (here's one for example). I've just noticed that it only happens on checked runtimes so I'm currently building one to verify it. I do have a guess as to what could be the issue: in the span ctor there is a type check however it gets skipped if the type is a value type, the failing code does the check for value types as well. As the stack traces make no sense I'm not sure if it's in any way realistic that T is ever a value type where it doesn't match the actual array type but that's the only (obvious) difference. I'm currently trying to verify it.

@jkotas
Copy link
Member Author

jkotas commented Jan 12, 2024

This is codegen bug in PGO optimized code. The repro is not deterministic since it is triggered by a particular profile.

I have looked at the dump from runfo get-helix-payload -j ec78045b-ffd3-4800-9c13-a3f3db5a5b72 -w System.Text.Json.Tests -o $HOME/helix_payload/System.Text.Json.Tests.

The codegen bug is in the PGO optimized VerifyWeatherForecastWithPOCOs method. The JIT inlines the OrderBy Linq call graph into the method, including the CollectionsMarshal.AsSpan method called by the Linq.

Here is the disassembly of the inlined CollectionsMarshal.AsSpan with the codegen bug:

...
    // Debug.Assert(items is not null, "Implementation depends on List<T> always having an array.");
    0x7fa2699feef8: testq  %r8, %r8
    0x7fa2699feefb: jne    0x7fa2699fef17
    0x7fa2699feefd: movabsq $0x7fa254205690, %rdi     ; string "Implementation depends on List<T> always having an array."
    0x7fa2699fef07: movabsq $0x7fa254200008, %rsi     ; string ""
    0x7fa2699fef11: callq  *-0x10151d47(%rip)

    // if ((uint)size > (uint)items.Length)
    0x7fa2699fef17: movq   -0x418(%rbp), %r8 items
    0x7fa2699fef1e: movl   -0x1c0(%rbp), %ecx length
    0x7fa2699fef24: cmpl   %ecx, 0x8(%r8) items.Length
    0x7fa2699fef28: jb     0x7fa2699ffe5c

    // Debug.Assert(typeof(T[]) == list._items.GetType(), "Implementation depends on List<T> always using a T[] and not U[] where U : T.");
    // BUG: The JIT incorrectly evaluated typeof(T[]) == list._items.GetType() as constant false and just left the null check
    0x7fa2699fef2e: movq   -0x400(%rbp), %rax
    0x7fa2699fef35: movq   0x8(%rax), %rdi
    0x7fa2699fef39: cmpb   %dil, (%rdi) // null check
    0x7fa2699fef3c: movabsq $0x7fa254228198, %rdi     ; string "Implementation depends on List<T> always using a T[] and not U[] where U : T."
    0x7fa2699fef46: movabsq $0x7fa254200008, %rsi     ; string ""
    0x7fa2699fef50: callq  *-0x10151d86(%rip) System.Diagnostics.Debug.Fail
...

@jkotas jkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 12, 2024
@ghost
Copy link

ghost commented Jan 12, 2024

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

Issue Details

Build Information

Build: https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=524311
Build error leg or test failing: System.Text.Json.Tests.WorkItemExecution
Pull request: #96870

Error Message

Fill the error message using step by step known issues guidance.

{
  "ErrorMessage": "Implementation depends on List<T> always using a T[] and not U[] where U : T.",
  "ErrorPattern": "",
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}

Known issue validation

Build: 🔎 https://dev.azure.com/dnceng-public/public/_build/results?buildId=524311
Error message validated: Implementation depends on List<T> always using a T[] and not U[] where U : T.
Result validation: ✅ Known issue matched with the provided build.
Validation performed at: 1/12/2024 1:31:48 AM UTC

Report

Build Definition Test Pull Request
525560 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96609
525570 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96880
525477 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96805
525444 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96867
525362 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96880
525344 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96741
525094 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96894
525249 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96896
525236 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96445
525231 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96890
525196 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96214
525140 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96676
525086 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96893
525001 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96741
524882 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #95583
524968 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96892
524959 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96558
524862 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96889
524798 dotnet/runtime System.Text.Json.Tests.WorkItemExecution
524790 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #95583
524621 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96860
524464 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96805
524458 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96867
524255 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96829
524448 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96858
524210 dotnet/runtime System.Text.Json.Tests.WorkItemExecution
524311 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96870
524291 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96867
524193 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96858
524168 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96805
524110 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96795
524101 dotnet/runtime System.Text.Json.Tests.WorkItemExecution #96699

Summary

24-Hour Hit Count 7-Day Hit Count 1-Month Count
26 32 32
Author: jkotas
Assignees: -
Labels:

area-System.Text.Json, area-CodeGen-coreclr, blocking-clean-ci, in-pr, Known Build Error

Milestone: 9.0.0

@jkotas
Copy link
Member Author

jkotas commented Jan 13, 2024

I am commenting out the assert in #96877 to workaround the bug. Please add the assert back once the issue is fixed.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 13, 2024
@EgorBo
Copy link
Member

EgorBo commented Jan 13, 2024

Managed to repro locally on Main, investigating...

@EgorBo
Copy link
Member

EgorBo commented Jan 13, 2024

Minimal repro:

class Program
{
    static void Main()
    {
        Console.WriteLine(foo<string>(new string[1]));
        Console.ReadKey();
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static bool foo<T>(string[] list) => typeof(T[]) == list.GetType();
}

Reproduces without PGO, seems like the bug is in gtFoldTypeCompare

@EgorBo
Copy link
Member

EgorBo commented Jan 13, 2024

This is the current culprit:

image

same VM call in .NET 8.0 (where bug is not manifested) returns May

@jakobbotsch
Copy link
Member

From a bisection, the regression seems to have been introduced in range 82ac648...23a93aa.

@EgorBo
Copy link
Member

EgorBo commented Jan 13, 2024

From a bisection, the regression seems to have been introduced in range 82ac648...23a93aa.

cc @davidwrighton @jkotas Could it be #96466 ?

TLDR: it seems that JIT-EE compareTypesForEquality(string[], _Canon[]) returns MustNot now.

@jkotas
Copy link
Member Author

jkotas commented Jan 13, 2024

Could it be #96466 ?

Yes, that change included a fix in HasSameTypeDefAs that broke implementation of compareTypesForEquality. Thank you for tracking it down!

jkotas added a commit to jkotas/runtime that referenced this issue Jan 17, 2024
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 17, 2024
@JulieLeeMSFT JulieLeeMSFT added area-VM-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Jan 17, 2024
@jkotas jkotas closed this as completed in 5415bd2 Jan 18, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 18, 2024
tmds pushed a commit to tmds/runtime that referenced this issue Jan 23, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants