Don't excessively generate default constructors #73683
Conversation
Fixes dotnet#68660. This is a piece of logic that originates from .NET Native. We didn't have a hard stance on `Activator.CreateInstance<T>` at that time, so we took a conservative approach of always generating the default constructors for all types that have type handle. Since we now do static analysis and warn if `Activator` is used with something we couldn't analyze. We can trim more. I would delete the whole table and logic, but I'm not sure if we can't still hit a problem when `CreateInstance<T>` is used with a reflection blocked type - this would affect private reflection within CoreLib (we don't think about reflection when doing `new T()`, so this can happen). With reflection blocking even if dataflow figures out that `BlockedFromReflectionType..ctor` should be reflectable, we won't reflection enable it. We really should just get rid of reflection blocking (dotnet#72570). Then all of this can go away.
Would it be worth it to add an assert to check if there are any tests that hit this situation? I do not think we should have cases like this, and it should be safe to delete this table. |
I do not think that this logic actually works today. For example, this fails with Another point for just deleting that table. |
This table is only used by the type loader as part of the expansion of the So the repro for this would be a generic type that has a I don't know if I can trust testing to run into that. Thinking about this, at minimum this table is needed to support GVM dispatch in the reflection-disabled mode since the promise there was that |
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.
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 #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. * Update tests.proj
ghost
locked as resolved and limited conversation to collaborators
2 participants
Fixes #68660.
This is a piece of logic that originates from .NET Native. We didn't have a hard stance on
Activator.CreateInstance<T>at that time, so we took a conservative approach of always generating the default constructors for all types that have type handle.Since we now do static analysis and warn if
Activatoris used with something we couldn't analyze. We can trim more.I would delete the whole table and logic, but I'm not sure if we can't still hit a problem when
CreateInstance<T>is used with a reflection blocked type - this would affect private reflection within CoreLib (we don't think about reflection when doingnew T(), so this can happen). With reflection blocking even if dataflow figures out thatBlockedFromReflectionType..ctorshould be reflectable, we won't reflection enable it.We really should just get rid of reflection blocking (#72570). Then all of this can go away.
Cc @dotnet/ilc-contrib