Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combine nearby loops to prevent crash building dotnet/runtime #46980

Closed
wants to merge 1 commit into from
+3 −6
Closed

Combine nearby loops to prevent crash building dotnet/runtime #46980

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1216,19 +1216,14 @@ public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations
if (anyToRemove)
{
var explicitInterfaceImplementationsBuilder = ArrayBuilder<MethodSymbol>.GetInstance();
MethodSymbol uniqueClassOverride = null;
foreach (var method in explicitlyOverriddenMethods)
{
if (method.ContainingType.IsInterface)
{
explicitInterfaceImplementationsBuilder.Add(method);
}
}

explicitInterfaceImplementations = explicitInterfaceImplementationsBuilder.ToImmutableAndFree();

MethodSymbol uniqueClassOverride = null;
foreach (MethodSymbol method in explicitlyOverriddenMethods)
{
if (method.ContainingType.IsClassType())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if [](start = 24, length = 2)

else if

{
if (uniqueClassOverride is { })
Expand All @@ -1242,6 +1237,8 @@ public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations
}
}

explicitInterfaceImplementations = explicitInterfaceImplementationsBuilder.ToImmutableAndFree();
Copy link
Member

@cston cston Aug 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
{
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (uniqueClassOverride is { })
{
Interlocked.CompareExchange(ref AccessUncommonFields()._lazyExplicitClassOverride, uniqueClassOverride, null);
Expand Down