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

Assert failure: Compiler optimization assumption invalid: FAILED: pMgr != 0 #70259

Closed
BruceForstall opened this issue Jun 5, 2022 · 9 comments · Fixed by #70269
Closed

Assert failure: Compiler optimization assumption invalid: FAILED: pMgr != 0 #70259

BruceForstall opened this issue Jun 5, 2022 · 9 comments · Fixed by #70269
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI blocking-clean-ci-optional Blocking optional rolling runs
Milestone

Comments

@BruceForstall
Copy link
Member

BruceForstall commented Jun 5, 2022

Pipeline: runtime-coreclr libraries-jitstress
Configs:

  • System.Text.RegularExpressions.Unit.Tests, net7.0-windows-Release-x86-CoreCLR_checked-jitstress1-Windows.10.Amd64.Open
  • System.Text.RegularExpressions.Unit.Tests, net7.0-windows-Release-x86-CoreCLR_checked-jitstress1_tiered-Windows.10.Amd64.Open
  • System.Text.RegularExpressions.Unit.Tests, net7.0-windows-Release-x86-CoreCLR_checked-tailcallstress-Windows.10.Amd64.Open

Also in LibraryImportGenerator.Unit.Tests, same configs.

https://dev.azure.com/dnceng/public/_build/results?buildId=1807559&view=ms.vss-test-web.build-test-results-tab

Looks like a VSD assert.

@dotnet/jit-contrib JIT issue?

@BruceForstall BruceForstall added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI blocking-clean-ci-optional Blocking optional rolling runs labels Jun 5, 2022
@BruceForstall BruceForstall added this to the 7.0.0 milestone Jun 5, 2022
@ghost
Copy link

ghost commented Jun 5, 2022

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

Issue Details

Pipeline: runtime-coreclr libraries-jitstress
Configs:

  • System.Text.RegularExpressions.Unit.Tests, net7.0-windows-Release-x86-CoreCLR_checked-jitstress1-Windows.10.Amd64.Open
  • System.Text.RegularExpressions.Unit.Tests, net7.0-windows-Release-x86-CoreCLR_checked-jitstress1_tiered-Windows.10.Amd64.Open
  • System.Text.RegularExpressions.Unit.Tests, net7.0-windows-Release-x86-CoreCLR_checked-tailcallstress-Windows.10.Amd64.Open

https://dev.azure.com/dnceng/public/_build/results?buildId=1807559&view=ms.vss-test-web.build-test-results-tab

Looks like a VSD assert.

@dotnet/jit-contrib JIT issue?

Author: BruceForstall
Assignees: -
Labels:

area-CodeGen-coreclr, blocking-clean-ci-optional

Milestone: 7.0.0

@jkotas
Copy link
Member

jkotas commented Jun 5, 2022

This looks like JIT stress issue. JIT stress converted delegate call to tailcall. This conversion is not valid when the delegate points to virtual interface method.

@BruceForstall
Copy link
Member Author

@jakobbotsch Related to #69941? #70033?

@jakobbotsch
Copy link
Member

Probably #70033, though it is curious that this seemingly used to work. Maybe something rotted in the period that the JIT helper was not being used.

@jakobbotsch jakobbotsch self-assigned this Jun 5, 2022
@jakobbotsch
Copy link
Member

@jkotas

This looks like JIT stress issue. JIT stress converted delegate call to tailcall. This conversion is not valid when the delegate points to virtual interface method.

What exactly do you mean by virtual interface method here? Do you mean any call that may go through VSD?

@jkotas
Copy link
Member

jkotas commented Jun 5, 2022

Here is what is happening based on the crash dump:

// This delegate points to shuffle thunk that in turn points to VSD stub
// The VSD stub expects the callsite to be VSD DelegateCallSite.
var d = typeof(IFace).GetMethod("Method").CreateDelegate<Action<IFace>>();

var c = new ClassA();

// JIT stress converted this to tailcall. The callsite is not identified as 
// DelegateCallSite anymore and that's why we get the assert.
d(c); 

public class ClassA : IFace
{
    public virtual void Method() { Console.WriteLine("Method"); }
}

public interface IFace
{
    public void Method();
}

it is curious that this seemingly used to work.

These delegates pointing to VSD stubs are very rare. It is quite possible that it was broken for a long time and we just got lucky that the JIT stress did not hit it before.

@jakobbotsch
Copy link
Member

These delegates pointing to VSD stubs are very rare. It is quite possible that it was broken for a long time and we just got lucky that the JIT stress did not hit it before.

I see, thanks for the example. In practice does this mean that tail prefixed delegate calls cannot be done through the old mechanism?

@jkotas
Copy link
Member

jkotas commented Jun 5, 2022

Right.

It may be possible to fix up the x86 JIT_TailCall helper to handle this case, but I do not think that it is worth it.

@jakobbotsch
Copy link
Member

The other half of why we may not have seen this before is that isCallRelativeIndirect inside StubCallSite::StubCallSite needs to pass, i.e. the tailcaller's caller need to be on the form call [rel32]. If that check does not pass the VSD resolver properly sees the call as a delegate call.

jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Jun 5, 2022
It is rare but possible to have delegates that point to VSD stubs. Since
VSD stubs on x86 may disassemble the call-site we cannot allow tail
calling these with the old mechanism.

Fix dotnet#70259
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 5, 2022
BruceForstall pushed a commit that referenced this issue Jun 7, 2022
)

* Always use portable tailcalling mechanism for delegate tailcalls

It is rare but possible to have delegates that point to VSD stubs. Since
VSD stubs on x86 may disassemble the call-site we cannot allow tail
calling these with the old mechanism.

Fix #70259

* Add regression test

* Calm down the test

Tailcall through built-in delegates is best-effort. Instantiating stubs
may break the chain and on ARM32 so may wrapper delegate stubs. The
latter happens for this particular test. Change the test so that it is
just verifying we do not hit the assert.

* Disable regression test on Mono

* Clean up some ildasm output
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 7, 2022
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 blocking-clean-ci-optional Blocking optional rolling runs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants