Skip to content

Conversation

@MichalStrehovsky
Copy link
Member

If we were in a situation like in the regression test, we would devirtualize the GrabObject call to SomeUnallocatedClass.GrabObject. But because SomeUnallocatedClass was never allocated, the scanner didn't scan it, and bad things would happen.

Prevent devirtualizing into types that were not seen as allocated.

This is not a real issue for non-generic (non-shareable) types because the tentative instance method optimization generates throwing bodies for these. But tentative method optimization doesn't run for shared generics:

!method.IsSharedByGenericInstantiations; /* Current impl limitation; can be lifted */

This was rare enough that we haven't seen it until I did #73683 and there was one useless constructor that we stopped generating and triggered this.

This also includes what is essentially a rollback of dotnet/runtimelab#1700. This should have been rolled back with #66145 but I forgot we had this. It was not needed.

If we were in a situation like in the regression test, we would devirtualize the `GrabObject` call to `SomeUnallocatedClass.GrabObject`. But because `SomeUnallocatedClass` was never allocated, the scanner didn't scan it, and bad things would happen.

Prevent devirtualizing into types that were not seen as allocated.

This is not a real issue for non-generic (non-shareable) types because the tentative instance method optimization generates throwing bodies for these. But tentative method optimization doesn't run for shared generics:

https://github.com/dotnet/runtime/blob/4cbe6f99d23e04c56a89251d49de1b0f14000427/src/coreclr/tools/Common/Compiler/MethodExtensions.cs#L115

This was rare enough that we haven't seen it until I did dotnet#73683 and there was one useless constructor that we stopped generating and triggered this.

This also includes what is essentially a rollback of dotnet/runtimelab#1700. This should have been rolled back with dotnet#66145 but I forgot we had this. It was not needed.
@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky MichalStrehovsky merged commit 16f1d26 into dotnet:main Aug 12, 2022
@MichalStrehovsky MichalStrehovsky deleted the devirtbug branch August 12, 2022 12:50
@ghost ghost locked as resolved and limited conversation to collaborators Sep 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants