Fix resolving of default interface methods in crossgen2#126351
Fix resolving of default interface methods in crossgen2#126351BrzVlad wants to merge 7 commits intodotnet:mainfrom
Conversation
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).
There was a problem hiding this comment.
@MichalStrehovsky Thanks for the input, I'll see if I can create a local repro to validate this theory. Still, this PR only touches the resolution for static virtual methods, so I would assume that wouldn't be a blocker for this PR, right ?
Having said that, this discussion about versioning got me looking deeper into whether the existing check for versioning in the static virtual case was correct to begin with (I took it at face value without much thought). I believe we would need to check not only that the resolved method is in the version bubble, but also all intermediary types between the exactType where we are resolving the static virtual method and the type of this resolved method.
I was able to reproduce with a simple 3 project solution:
using System;
using System.Runtime.CompilerServices;
// ibase.dll
public interface IBase
{
static virtual int GetValue() => 42;
}
// iderived.dll
public interface IDerived : IBase
{
//static int IBase.GetValue() => 43;
}
// program.dll
public struct MyStruct : IDerived
{
}
public class Program
{
[MethodImpl(MethodImplOptions.NoInlining)]
static int CallGetValue<T>() where T : IBase
{
return T.GetValue();
}
public static int Main()
{
int result = CallGetValue<MyStruct>();
Console.WriteLine($"Result: {result}");
}
}
I built the project with the implementation of MyStruct.GetValue residing in IBase. Then I did a composite r2r compilation only for the program.dll and ibase.dll. With the existing implementation, we hardcode the call to IBase.GetValue. I then uncommented the line in IDerived to override the static DIM method. Rebuit only the iderived project and copied the dll next to the composite r2r images. Running the application still reported 42, rather than 43. This wrong behavior was introduced by this change.
Just double checking that my analysis is correct before I proceed with the fix that checks all the intermediary types. Maybe we are already doing something like this in other places.
There was a problem hiding this comment.
This would be more of a question for @davidwrighton. I'm not sure how we define what is a valid version bubble. If we have 3 assemblies where Program -> IDerived -> IBase (Program references IDerived, which references IBase), it feels problematic to allow defining the bubble as Program+IBase. Other combinations (Program + IDerived, IDerived + IBase) sound less problematic and I think that's the intended use of version bubbles. I've not dealt with this aspect of R2R in the past.
There was a problem hiding this comment.
The general approach for this sort of thing, when we've enabled it, is to allow the calculation to proceed, but to know that we've made a computation that has version bubble constraints. Generally, if ANY of the open types of the interfaces on the type are not defined in the version bubble, you can't really predict the behavior of a default interface resolution (This is notably true in particular in the presence of circular dependency chains, which are the degenerate, difficult to deal with case)
So, if we want to optimize this, and I think it's a good idea, what we do here, is detect that we've done a default interface resolution which isn't entirely closed based on the entire type hierarchy of the type (Excluding System.Object/System.ValueType, since we can rely on them not providing surprising implementations), then create a Check_VirtualFunctionOverride fixup to specify that the compiled function can only be used if the virtual resolution resolves to the right place. Now, that fixup has never been used with an abstract static function, so we may need to enhance it to work with virtual statics, but that's relatively simple.
Logically, this could also be used allow the CORINFO_BOX_THIS transformation to be done in R2R more cases as well. (Basically the throw at
The tricky detail here, is that it is possible for creating the Check_VirtualFunctionOverride signature to be difficult/impossible for us to create. We don't actually use this Fixup today by default in any cases, so it hasn't been extensively battle tested to find out all of the ways it might not be generatable, so that might require some additional work.
There was a problem hiding this comment.
@davidwrighton Thanks for the input. I've added a new commit with the r2r fixup that validates at runtime whether the resolution was correct. Tested locally and it seems to be working correctly. I expected that I would have to add support for static DIM resolution when handling these fixups in jitinterface.cpp, but it seems that the current path going through FindDispatchImpl already handles this. I haven't yet added the conditional checks on types to prevent addition of these fixups, not sure how relevant it is since they seem very lightweight.
Could you take a look whether the changes are what you were expecting
| 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
…foImpl.ReadyToRun.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…tic virtual DIMs In order to prevent cases where the resolution at runtime would differ due to changes to types that are not in the compilation version bubble.
| // emit a Check_VirtualFunctionOverride fixup so that the R2R code is | ||
| // rejected at runtime if the resolution changes (e.g. a DIM override | ||
| // is added in an assembly outside the version bubble). | ||
| MethodWithToken declMethodWithToken = new MethodWithToken(originalMethod, HandleToModuleToken(ref pResolvedToken), null, false, null); |
There was a problem hiding this comment.
declMethodWithToken is created with context: null (and bypasses ComputeMethodWithToken). For MemberRef/MethodSpec tokens, MethodWithToken.OwningType is expected to be the type referred to by the token after applying pResolvedToken.tokenContext (SignatureBuilder emits the owner type from method.OwningType when READYTORUN_METHOD_SIG_OwnerType is set). With context: null, the owning type can be computed incorrectly, potentially producing an invalid/unstable Check_VirtualFunctionOverride signature. Consider constructing declMethodWithToken via ComputeMethodWithToken(originalMethod, ref pResolvedToken, constrainedType: null, unboxing: false) (or otherwise passing the appropriate context from pResolvedToken.tokenContext).
| MethodWithToken declMethodWithToken = new MethodWithToken(originalMethod, HandleToModuleToken(ref pResolvedToken), null, false, null); | |
| MethodWithToken declMethodWithToken = ComputeMethodWithToken(originalMethod, ref pResolvedToken, constrainedType: null, unboxing: false); |
|
/azp list |
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This matches runtime implementation from jitinterface.cpp, which looks like the baseline for the r2r implementation.
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This makes it match the runtime decoding from https://github.com/dotnet/runtime/blob/004c358f4376950ead5ef31b95bf4f91a725b42a/src/coreclr/vm/zapsig.cpp#L901, which always uses the original module and not the one that was overridden when UpdateContext flag is set.
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…rtualFunctionOverride Previous code was throwing in some corner cases. Use instead ResolveVirtualStaticMethod which is meant to handle these types of calls.
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@davidwrighton This now passes the crossgen2 test suite. Last 3 commits on the PR are fixing various issues I've encountered in these tests. Please take a look. |
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