Fix covariant returns with generic return types #45275
Conversation
When a covariant return type was an uninstantiated generic type, the ClassLoader::IsCompatibleWith was not working properly. In debug builds, it was asserting because there was no MethodTable for that type and in release builds, it resulted in ExecutionEngineException or an internal CLR error. This change fixes it by using TypeHandle::CanCastTo instead of MethodTable::CanCastTo and adds a regression test for two cases where the problem was observed (Assembly.GetTypes() and creating an instance of a type with a covariant return with a problematic kind of type).
|
Also fixes #45082 ? |
|
@benaadams I've just tried and it doesn't fix it. There is yet another issue hidden behind my fix (with my fix, it fails at a different place than before). |
|
I did try changing the runtime to use covariant returns for RuntimeType; but it then crashes with both types of crossgen and jitdiff benaadams@e19bcfd so doesn't complete the build (does do the IL though) |
|
@benaadams can you please create an issue for that? I'll look into it after fixing the #45082. |
|
Raised #45356 |
|
I've updated the PR so that it fixes the #45082 too and added a regression test for that one. |
There were two issues. First, the ClassLoader::ValidateMethodsWithCovariantReturnTypes was called before typeHnd.DoFullyLoad and that resulted in an assert down the call chain of TypeDesc::CanCastTo due to a wrong load level. Second, the SigTypeContext generation for the current MD in the ClassLoader::ValidateMethodsWithCovariantReturnTypes requires the same change for class instantiation as the one that we had for the parent MD.
janvorli
force-pushed
the
fix-covariant-returns-generic-definition
branch
from
5fdaacf
to
1ad1828
Compare
src/coreclr/src/vm/clsload.cpp
Outdated
| DFLPendingList pendingList; | ||
| BOOL fBailed = FALSE; | ||
|
|
||
| typeHnd.DoFullyLoad(NULL, CLASS_LOADED, &pendingList, &fBailed, pInstContext); | ||
|
|
||
| if (!typeHnd.IsTypeDesc()) | ||
| { | ||
| ClassLoader::ValidateMethodsWithCovariantReturnTypes(typeHnd.AsMethodTable()); |
There was a problem hiding this comment.
DoFullyLoad marks the type as fully loaded. Another thread that tries to load the same type may see it as fully loaded and won't perform this validation.
I think this needs to be done before the type is marked as fully loaded.
There was a problem hiding this comment.
Hmm, so I'll need to figure out how to fix the issue that was fixed by this change. The problem was that down the call chain of the ValidateMethodsWithCovariantReturnTypes, a load level assert fired.
There was a problem hiding this comment.
This is the only concern I have with this change.
src/coreclr/src/vm/clsload.cpp
Outdated
| DFLPendingList pendingList; | ||
| BOOL fBailed = FALSE; | ||
|
|
||
| typeHnd.DoFullyLoad(NULL, CLASS_LOADED, &pendingList, &fBailed, pInstContext); | ||
|
|
||
| if (!typeHnd.IsTypeDesc()) | ||
| { | ||
| ClassLoader::ValidateMethodsWithCovariantReturnTypes(typeHnd.AsMethodTable()); |
There was a problem hiding this comment.
This is the only concern I have with this change.
The call to ClassLoader::ValidateMethodsWithCovariantReturnTypes is now in MethodTable::DoFullyLoad. I have also added a test case that verifies a case that David Wrighton has suggested offline, where there are 3 types... A, B and C. C derives from B which derives from A. B has a bad override which should produce an error. Then, cause C to be fully loaded without otherwise triggering a load of B.
|
The |
|
@akoeplinger @safern Could you please disable the failing Mono legs or move them to the staging pipeline so that they do not cause all jobs to be red? |
|
The leg was already disabled with #45529 but you'd need to rebase/merge master to get this change in the PR. I'd just merge. |
|
Thanks @akoeplinger! |
|
/backport to release/5.0 |
|
Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/411777053 |
ghost
locked as resolved and limited conversation to collaborators
When a covariant return type was an uninstantiated generic type, the
ClassLoader::IsCompatibleWith was not working properly. In debug builds,
it was asserting because there was no MethodTable for that type and in
release builds, it resulted in ExecutionEngineException or an internal
CLR error.
This change fixes it by using TypeHandle::CanCastTo instead of
MethodTable::CanCastTo and adds a regression test for two cases where
the problem was observed (Assembly.GetTypes() and creating an instance
of a type with a covariant return with a problematic kind of type).
Close #45037
Close #45082