Combine nearby loops to prevent crash building dotnet/runtime #46980
Conversation
|
|
||
| MethodSymbol uniqueClassOverride = null; | ||
| foreach (MethodSymbol method in explicitlyOverriddenMethods) | ||
| { | ||
| if (method.ContainingType.IsClassType()) |
There was a problem hiding this comment.
if [](start = 24, length = 2)
else if
| @@ -1242,6 +1237,8 @@ public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations | |||
| } | |||
| } | |||
|
|
|||
| explicitInterfaceImplementations = explicitInterfaceImplementationsBuilder.ToImmutableAndFree(); | |||
There was a problem hiding this comment.
explicitInterfaceImplementations [](start = 20, length = 32)
Would it be easier to remove the loop entirely from the method, to avoid the complexity of handling both in the same loop:
explicitInterfaceImplementations = explicitlyOverriddenMethods.WhereAsArray(
m => m.ContainingType.IsInterface);
MethodSymbol uniqueClassOverride = getUniqueClassOverride(explicitlyOverriddenMethods);
static MethodSymbol getUniqueClassOverride(ImmutableArray<MethodSymbol> explicitlyOverriddenMethods)
{
...
}
There was a problem hiding this comment.
We're skipping any trailing explicit interface implementations. Is that an issue? Consider adding a test.
Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs:1233 in fd91c43. [](commit_id = fd91c43, deletion_comment = False)
Thanks for catching this. I did change semantics by accident. 😉 I am thinking perhaps we should try @davidwrighton's suggestion for now to just apply [MethodImpl(MethodImplOptions.NoOptimization)] to this method as well as perhaps a WorkItem link to track removing it once the related JIT bug is fixed.
Related to #46575 (not closing till we are able to root-cause and make sure we have a tracking issue in the appropriate place).
I have verified locally that this resolves the crash when building dotnet/runtime. It's somewhat inconvenient to publish this build where dotnet/runtime can readily consume it, so I won't be able to verify in CI until after this is merged to master-vs-deps.
We currently believe this is working around a JIT bug.