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

JIT: Properly handle a switch opt case during early flow opts #40434

Merged
merged 1 commit into from
Aug 6, 2020

Conversation

AndyAyersMS
Copy link
Member

Don't try rethreading statement lists if we're doing and early flow opt.

Fixes #40195.

Don't try rethreading statement lists if we're doing and early flow opt.

Fixes dotnet#40195.
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 6, 2020
@AndyAyersMS
Copy link
Member Author

cc @dotnet/jit-contrib @eiriktsarpalis

Libraries test now completes on x64 OSX:

  Discovering: System.Formats.Cbor.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Formats.Cbor.Tests (found 338 test cases)
  Starting:    System.Formats.Cbor.Tests (parallel test collections = on, max threads = 4)
  Finished:    System.Formats.Cbor.Tests
=== TEST EXECUTION SUMMARY ===
   System.Formats.Cbor.Tests  Total: 1778, Errors: 0, Failed: 0, Skipped: 0, Time: 5.127s

x64 windows PMI over FSharp.Core.dll gets further, but now hits an assert in impNormStructVal. I'll open a new issue for this.

Assert failure(PID 51772 [0x0000ca3c], Thread: 38752 [0x9760]): Assertion failed 'structVal->gtType == structType' in 'Microsoft.FSharp.Collections.Array3DModule:Iterate(Microsoft.FSharp.Core.FSharpFunc`2[[System.Numerics.Vector`1[[System.Single, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[Microsoft.FSharp.Core.Unit, FSharp.Core, Version=4.7.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a]],System.Numerics.Vector`1[System.Single][,,])' during 'Importation' (IL size 145)

    File: C:\repos\runtime0\src\coreclr\src\jit\importer.cpp Line: 1696
    Image: c:\repos\runtime0\artifacts\tests\coreclr\Windows_NT.x64.Checked\Tests\Core_Root\CoreRun.exe

@AndyAyersMS
Copy link
Member Author

Looked over other callers of fgSetStmtSeq and did not see any others that were unguarded. No diffs expected, otherwise we'd have also hit the assert seen in #40195.

This is likely a regression from #1309 which has been in the code for 8 months now. The assert hit in #40195 is a noway_assert so presumably any case that hit this it in a release jit would have fallen back to minopts where it would have jitted ok as we would not have tried the problematic switch opt again. That likely explains why we haven't gotten upstack reports of this; we don't have any noway assert telemetry and don't have any upstack use of checked jits.

@AndyAyersMS
Copy link
Member Author

Follow-on issue for PMI of FSharp.Core.dll is #40440.

@AndyAyersMS
Copy link
Member Author

One failure in x64 to investigate...

  Discovered:  baseservices.exceptions.XUnitWrapper
Unhandled exception.   Discovering: baseservices.TieredCompilation.XUnitWrapper
System.InvalidOperationException: Collection was modified after the enumerator was instantiated.
   at System.Collections.Generic.Stack`1.Enumerator.MoveNext()
   at Xunit.Sdk.DisposalTracker.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\DisposalTracker.cs:line 26
   at Xunit.Sdk.ExtensibilityPointFactory.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\ExtensibilityPointFactory.cs:line 53
   at Xunit.Sdk.TestFramework.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\TestFramework.cs:line 48
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__140_1(Object state)
   at System.Threading.QueueUserWorkItemCallbackDefaultContext.Execute()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback()
  Discovered:  baseservices.TieredCompilation.XUnitWrapper
  Starting:    baseservices.TieredCompilation.XUnitWrapper

Guess I should also validate that this is no diff. While we won't hit the asserting path the jit might could behave a bit differently if this flow opt happens once statements are threaded.

@AndyAyersMS
Copy link
Member Author

No diffs, so failure above must be something unrelated. Am going to rerun.

@echesakov
Copy link
Contributor

One failure in x64 to investigate...

  Discovered:  baseservices.exceptions.XUnitWrapper
Unhandled exception.   Discovering: baseservices.TieredCompilation.XUnitWrapper
System.InvalidOperationException: Collection was modified after the enumerator was instantiated.
   at System.Collections.Generic.Stack`1.Enumerator.MoveNext()
   at Xunit.Sdk.DisposalTracker.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\DisposalTracker.cs:line 26
   at Xunit.Sdk.ExtensibilityPointFactory.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\ExtensibilityPointFactory.cs:line 53
   at Xunit.Sdk.TestFramework.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\TestFramework.cs:line 48
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__140_1(Object state)
   at System.Threading.QueueUserWorkItemCallbackDefaultContext.Execute()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback()
  Discovered:  baseservices.TieredCompilation.XUnitWrapper
  Starting:    baseservices.TieredCompilation.XUnitWrapper

Guess I should also validate that this is no diff. While we won't hit the asserting path the jit might could behave a bit differently if this flow opt happens once statements are threaded.

I haven't seen this call stack for a while, perhaps it is the same as #11063 ?

@AndyAyersMS
Copy link
Member Author

Linker test failure looks similar in spirit to #40398.

Installer test failure almost certainly unrelated.

System.ComponentModel.Win32Exception : Text file busy
   at System.Diagnostics.Process.ForkAndExecProcess(String filename, String[] argv, String[] envp, String cwd, Boolean redirectStdin, Boolean redirectStdout, Boolean redirectStderr, Boolean setCredentials, UInt32 userId, UInt32 groupId, UInt32[] groups, Int32& stdinFd, Int32& stdoutFd, Int32& stderrFd, Boolean usesTerminal, Boolean throwOnNoExec)
   at System.Diagnostics.Process.StartCore(ProcessStartInfo startInfo)
   at System.Diagnostics.Process.Start()
   at Microsoft.DotNet.Cli.Build.Framework.Command.Start() in /_/src/installer/tests/TestUtils/Command.cs:line 199
   at AppHost.Bundle.Tests.BundleExtractToSpecificPath.Bundle_extraction_is_reused() in /_/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/BundleExtractToSpecificPath.cs:line 96

@AndyAyersMS AndyAyersMS merged commit 8978800 into dotnet:master Aug 6, 2020
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
…#40434)

Don't try rethreading statement lists if we're doing and early flow opt.

Fixes dotnet#40195.
@karelz karelz modified the milestones: 6.0.0, 5.0.0 Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

JIT assertion failure when running tests in OSX
5 participants