Assertion failed 'verCurrentState.esStackDepth == 0 && impOpcodeIsCallOpcode(opcode)'

[BruceForstall]

CoreFX, TailcallStress=1, TieredCompilation=0,

System.Linq.Expressions.Tests

Windows and Linux x64.

Executed on c000NQD

C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Work\00888a6e-ae65-4554-a90f-2a506a823817\Exec>set __TestEnv=C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Payload\SetStressModes_tailcallstress.cmd 

C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Work\00888a6e-ae65-4554-a90f-2a506a823817\Exec>type C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Payload\SetStressModes_tailcallstress.cmd 
set COMPlus_TieredCompilation=0
set COMPlus_TailcallStress=1

C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Work\00888a6e-ae65-4554-a90f-2a506a823817\Exec>call C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Payload\SetStressModes_tailcallstress.cmd 

C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Work\00888a6e-ae65-4554-a90f-2a506a823817\Exec>set COMPlus_TieredCompilation=0 

C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Work\00888a6e-ae65-4554-a90f-2a506a823817\Exec>set COMPlus_TailcallStress=1 

C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Work\00888a6e-ae65-4554-a90f-2a506a823817\Exec>set COMPlus 
COMPlus_TailcallStress=1
COMPlus_TieredCompilation=0

C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Work\00888a6e-ae65-4554-a90f-2a506a823817\Exec>call RunTests.cmd -r C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Payload --rsp-file C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Payload\CoreFX.issues.rsp  
----- start  1:10:52.12 ===============  To repro directly: ===================================================== 
pushd C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Work\00888a6e-ae65-4554-a90f-2a506a823817\Exec\
"C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Payload\dotnet.exe" xunit.console.dll System.Linq.Expressions.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=nonnetcoreapptests -notrait category=nonwindowstests -notrait category=OuterLoop -notrait category=failing @C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Payload\CoreFX.issues.rsp
popd
===========================================================================================================

C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Work\00888a6e-ae65-4554-a90f-2a506a823817\Exec>"C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Payload\dotnet.exe" xunit.console.dll System.Linq.Expressions.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=nonnetcoreapptests -notrait category=nonwindowstests -notrait category=OuterLoop -notrait category=failing @C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Payload\CoreFX.issues.rsp 
  Discovering: System.Linq.Expressions.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Linq.Expressions.Tests (found 5632 of 5641 test cases)
  Starting:    System.Linq.Expressions.Tests (parallel test collections = on, max threads = 2)

Assert failure(PID 992 [0x000003e0], Thread: 3724 [0x0e8c]): Assertion failed 'verCurrentState.esStackDepth == 0 && impOpcodeIsCallOpcode(opcode)' in 'DynamicClass:lambda_method(ref):ref' (IL size 18)

    File: f:\workspace\_work\1\s\src\jit\importer.cpp Line: 16670
    Image: C:\dotnetbuild\work\f0098ede-5f7f-47a5-a227-bf5292f7fb0c\Payload\dotnet.exe

----- end  1:16:13.94 ----- exit code -1073740286 ----------------------------------------------------------

[BruceForstall]

@dotnet/jit-contrib

[AndyAyersMS]

I'll investigate

[AndyAyersMS]

@BruceForstall was this a debug or release corefx?

[BruceForstall]

Would have been release corefx with Checked coreclr. The corefx came from the test assets they publish for us. dotnet/coreclr#24962 should have the same test runs, but this test has been disabled there.

[AndyAyersMS]

Thanks, I can repro this now. Needed checked runtime and corelib, not just checked jit...

Compiling 77142 DynamicClass::lambda_method, IL size = 18, hsh=0x1fb7cd59

Assert failure(PID 13004 [0x000032cc], Thread: 8872 [0x22a8]): Assertion failed 'verCurrentState.esStackDepth == 0 && impOpcodeIsCallOpcode(opcode)' in 'DynamicClass:lambda_method(ref):ref' (IL size 18)

    File: D:\repos\coreclr\src\jit\importer.cpp Line: 16670
    Image: D:\repos\corefx\artifacts\bin\testhost\netcoreapp-Windows_NT-Release-x64\dotnet.exe

[AndyAyersMS]

Think this is test that fails:

https://github.com/dotnet/corefx/blob/8135319caa7e457ed61053ca1418313b88057b51/src/System.Linq.Expressions/tests/MemberInit/ListBindTests.cs#L175-L193

Debug info from jit:

IL_0000  73 02 00 00 06    newobj       0x6000002        ;; obj        list .ctor
IL_0005  25                dup                           ;; obj, obj
IL_0006  28 03 00 00 06    call         0x6000003        ;; obj, obj   static list property          
IL_000b  16                ldc.i4.0                      ;; 0, obj, obj
IL_000c  6f 04 00 00 06    callvirt     0x6000004        ;; obj
IL_0011  2a                ret

We've just imported the callvirt

[000004] ------------              *  STMT      void  (IL 0x000...  ???)
[000003] -A----------              \--*  ASG       ref
[000002] D------N----                 +--*  LCL_VAR   ref    V02 tmp1
[000001] ------------                 \--*  ALLOCOBJ  ref
[000000] ------------                    \--*  CNS_INT(h) long   0x7ffdca0601f8 method

[000007] ------------              *  STMT      void  (IL   ???...  ???)
[000006] I-C-G-------              \--*  CALL      void   ListWrapper`1[Int32][System.Int32]..ctor (exactContextHnd=0x00007FFDCA0601F9)
[000005] ------------ this in rcx     \--*  LCL_VAR   ref    V02 tmp1

[000011] ------------              *  STMT      void  (IL 0x005...  ???)
[000010] I-C-G-------              \--*  CALL      ref    ListWrapper`1[Int32][System.Int32].get_StaticListProperty (exactContextHnd=0x00007FFDCA0601F9)

[000014] I-C-G-------              \--*  CALL nullcheck void   System.Collections.Generic.List`1[Int32][System.Int32].Add (exactContextHnd=0x00007FFDC9F61579)
[000012] --C--------- this in rcx     +--*  RET_EXPR  ref   (inl return from call [000010])
[000013] ------------ arg1            \--*  CNS_INT   int    0

Stack holds: 

[000008] ------------              *  LCL_VAR   ref    V02 tmp1

It looks like this test is intentionally creating bad IL, and this trips up tail call stress.

I think tail call stress needs to anticipate this and avoid marking the call as a tail call. Or we could perhaps water down the assert.

This is not a bug a user would normally hit -- it is tail call stress mode only.

[AndyAyersMS]

verCheckTailCallConstraint has the right logic but doesn't use it to indicate the tail call in invalid. Odd.

[AndyAyersMS]

Direct repro (once you've copied in the checked jit and set the right complus options):

cd D:\repos\corefx\src\System.Linq.Expressions\tests

dotnet msbuild /t:Test /p:ForceRunTests=true /p:ConfigurationGroup=Release 
/p:XunitMethodName=System.Linq.Expressions.Tests.ListBindTests.StaticListProperty

verCheckTailCallConstraint has the right logic but doesn't use it to indicate the tail call in invalid.

Looks like that bit is working properly, and we reject tail calling before we even get to the stack check since the last call is a void return but the method has a return value.

The issue is that under tail call stress, newBBcreatedForTailcallStress is set true even if we're not actually doing a tail call, and this leads to confusion in the jit.

[AndyAyersMS]

More specifically -- under tail call stress when forming the flow graph, we do a partial recognition for tail call candidates -- this is simply looking for a call followed by a ret. When we see this we set up the flow graph so that the ret following the call is in a separate BB. But we can't do a more complete assessment here because we haven't resolved the call token and so don't know the method sig. So we defer of the rest of the analysis until later.

In the importer, under tail call stress, we re-recognize this case and set newBBCreatedForTailcallStress. We then do some further validation via verCheckTailCallConstraint and by querying the runtime via canTailCall. In this particular instance, we recognize the call is not a valid tail call candidate because it returns void and the method returns non-void. We then drop out of the stress mode clause.

We then immediately reconsider the call as a potential implicit tail call candidate. Here we run a less thorough validation (for reasons described below) and so conclude this call is indeed still a viable (implicit) tail call candidate.

Because we've set newBBCreatedForTailcallStress, we then drop into impImportReturn to handle the (implicit) return and here we assert that the stack must be empty.

So the confusion is that under stress we may mark a call as an implicit tail call candidate when it's invalid, and this leads to asserts for some invalid IL, when handling the implict ret following the call. We don't hit this assert under normal compilation as the flow graph will not have been split, and instead we cleanly badcode somewhere else.

One fix is to feed the stress mode results into the implicit tail call recognition logic -- we could for instance note that this call failed stress validation and so shouldn't be considered as an implicit tail call candidate either. But it seems a bit odd to have a fact that we only discover during a stress mode influence a general decision.

Another option is to always run the more thorough validation for implicit tail calls, but there are various comments indicating that this ends up duplicating work (since at this point in the importer we still haven't resolved the token to check the sig, and/or checked with the runtime via canTailCall, and if we do these now, for successful candidates, we'll end up doing the same work over again in impImportCall).

So as odd as it seems, it looks like it makes sense to record the early evaluation result when under tail call stress and use that to inhibit implicit recognition. We'll do duplicate work in tail call stress mode in some cases, but not in normal jitting.

Longer term it would be nice to disentangle this....