Fix resolving of default interface methods in crossgen2#126351
Fix resolving of default interface methods in crossgen2#126351BrzVlad wants to merge 3 commits intodotnet:mainfrom
Conversation
When doing a static virtual call, we would only lookup implementations on the constrained type. If this resolution fails, we add the fallback of looking for DIMs on the implemented interfaces. Makes Perf_DateTimeCultureInfo.ToString tests 25x faster when having only interpreter fallback
There was a problem hiding this comment.
Pull request overview
Improves crossgen2 handling of constrained static virtual interface calls by adding a fallback to resolve default interface method (DIM) implementations when no concrete implementation is found on the constrained type.
Changes:
- Add DIM fallback resolution for static virtual calls when type-based resolution fails.
- Relax a debug assertion to allow resolved targets whose owning type is an interface when the target is a static DIM.
There was a problem hiding this comment.
It is odd that we have this if (isStaticVirtual) block in the first place. Both jitinterface.cpp and CorInfoImpl.RyuJit.cs (ILC) handle static virtuals with what's here in the else branch (it just goes to TryResolveConstraintMethodApprox).
Should we instead delete the if block? We can re-add the VersionsWithMethodBody check if needed, although I'm not sure why we need that for static virtuals but not for instance methods.
There was a problem hiding this comment.
Sorry for late reply, I wanted to get to testing locally. I pushed a new version which resues TryResolveConstraintMethodApprox, tested locally and it works as expected. I suspect the VersionsWithMethodBody wouldn't be needed for instance methods because instance interface calls are resolved at runtime anyway ? Also, for the devirt case, there does seem to be some versioning checks a few lines below so I guess this scenario is properly guarded. I preserved the check for static virtual case.
I didn't notice that the static virtual method scenario was already handled there. Looking at the commit history I suspect the timeline is that SVM support was initially attempted in https://github.com/dotnet/runtime/pull/54063/changes, which was handling this scenario separately. Then you added the support in https://github.com/dotnet/runtime/pull/72661/changes for native aot. Later, when r2r support was added in https://github.com/dotnet/runtime/pull/87438/changes, this didn't properly reuse the new implementation, following in the path of the original PR.
| var dimResolution = DefaultInterfaceMethodResolution.None; | ||
| MethodDesc directMethod = constrainedType.TryResolveConstraintMethodApprox(exactType, originalMethod, out forceUseRuntimeLookup, ref dimResolution); | ||
| if (isStaticVirtual && directMethod != null && | ||
| !_compilation.NodeFactory.CompilationModuleGroup.VersionsWithMethodBody(directMethod)) |
There was a problem hiding this comment.
Why is the VersionsWithMethodBody check done for isStaticVirtual only?
There was a problem hiding this comment.
I suspect the VersionsWithMethodBody wouldn't be needed for instance methods because instance interface calls are resolved at runtime anyway ? Also, for the devirt case, there does seem to be some versioning checks a few lines below so I guess this scenario is properly guarded. I preserved the check for static virtual case.
Michal pointed the same thing out. I believe the way it was working was correct so, my PR didn't change behavior in this area.
There was a problem hiding this comment.
I've been thinking about this a bit. If we resolved the constraint in the instance method case... the method must be on a valuetype. It would be an IL breaking change for the valuetype to stop implementing the interface or to drop the interface method implementation. So this should be fine even if the valuetype (and the instance method on it) is not in the version bubble.
HOWEVER, there's the constraint resolution corner case where if we have struct Foo<T> : IFoo<T>, IFoo<object> and we're trying to resolve the constraint for IFoo<__Canon>, the resolution actually becomes compile-time ambiguous. So one could do an IL non-breaking change (add an interface) to the struct that invalidates the R2R code if previously this wasn't compile-time ambiguous but in v2 it is. So maybe we need a version check for the instance case if the struct or interface is canonical. This is a @davidwrighton favorite area.
I'm trying to think whether the default interface resolution brings any extra versioning considerations since the inheritance hierarchy there is no longer linear. But I can't come up with anything so maybe just checking the method after resolving the constraint is still correct (the worry is whether we could have a situation where the resolution itself could have crossed a versioning bubble to decide the result but the result was still within the bubble).
…foImpl.ReadyToRun.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
| var dimResolution = DefaultInterfaceMethodResolution.None; | ||
| MethodDesc directMethod = constrainedType.TryResolveConstraintMethodApprox(exactType, originalMethod, out forceUseRuntimeLookup, ref dimResolution); | ||
| if (isStaticVirtual && directMethod != null && | ||
| !_compilation.NodeFactory.CompilationModuleGroup.VersionsWithMethodBody(directMethod)) | ||
| { | ||
| directMethod = constrainedType.ResolveVariantInterfaceMethodToStaticVirtualMethodOnType(originalMethod); | ||
| if (directMethod != null && !_compilation.NodeFactory.CompilationModuleGroup.VersionsWithMethodBody(directMethod)) | ||
| { | ||
| directMethod = null; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| directMethod = constrainedType.TryResolveConstraintMethodApprox(exactType, originalMethod, out forceUseRuntimeLookup); | ||
| directMethod = null; | ||
| } |
There was a problem hiding this comment.
TryResolveConstraintMethodApprox(..., ref dimResolution) can return null while setting dimResolution to Diamond or Reabstraction (ambiguous/reabstracted default interface implementation). This code currently ignores dimResolution and will later throw RequiresRuntimeJitException for unresolved static virtual calls, which is inconsistent with other call sites that treat these as invalid IL and throw BadImageFormat (e.g., ILImporter.Scanner.cs around the staticResolution check). Add an explicit check after the call to throw ThrowHelper.ThrowBadImageFormatException() (or the appropriate InvalidProgram/BIF exception used elsewhere in this JIT interface) when dimResolution is Diamond or Reabstraction.
When doing a static virtual call, we would only lookup implementations on the constrained type. If this resolution fails, we add the fallback of looking for DIMs on the implemented interfaces.
Makes Perf_DateTimeCultureInfo.ToString tests 25x faster when having only interpreter fallback