Skip to content

Commit

Permalink
Consider existence of EETypes and metadata for typeof checks (#107347)
Browse files Browse the repository at this point in the history
The problem was that when we optimize `typeof(Foo) == somevalue` checks, we were looking at the presence of a constructed `MethodTable` of `Foo` in the whole program view. If it didn't exist, we'd declare the equality comparison can't succeed. There is however an edge case where someone could obtain a `System.Type` representing a type by browsing the reflection metadata. Such `System.Type` might not have a workable constructed `MethodTable`, but should still compare equal to the `typeof`.

An example of when this could happen is when the type is used in a custom attribute metadata blob - such type may even have no `MethodTable` at all.

The fix is to look not just at `MethodTable` but also at the metadata for the type.
  • Loading branch information
MichalStrehovsky committed Sep 10, 2024
1 parent 4ee9789 commit a817006
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ public class SubstitutedILProvider : ILProvider
private readonly ILProvider _nestedILProvider;
private readonly SubstitutionProvider _substitutionProvider;
private readonly DevirtualizationManager _devirtualizationManager;
private readonly MetadataManager _metadataManager;

public SubstitutedILProvider(ILProvider nestedILProvider, SubstitutionProvider substitutionProvider, DevirtualizationManager devirtualizationManager)
public SubstitutedILProvider(ILProvider nestedILProvider, SubstitutionProvider substitutionProvider, DevirtualizationManager devirtualizationManager, MetadataManager metadataManager = null)
{
_nestedILProvider = nestedILProvider;
_substitutionProvider = substitutionProvider;
_devirtualizationManager = devirtualizationManager;
_metadataManager = metadataManager;
}

public override MethodIL GetMethodIL(MethodDesc method)
Expand Down Expand Up @@ -989,9 +991,15 @@ private bool TryExpandTypeEquality(in TypeEqualityPatternAnalyzer analyzer, Meth
if (!ConstructedEETypeNode.CreationAllowed(knownType))
return false;

// If a constructed MethodTable for this type exists, the comparison could succeed.
if (_devirtualizationManager.CanReferenceConstructedTypeOrCanonicalFormOfType(knownType.NormalizeInstantiation()))
return false;

// If we can have metadata for the type the comparison could succeed even if no MethodTable present.
// (This is the case of metadata-only types, where we were able to optimize the MethodTable away.)
if (_metadataManager != null && knownType.GetTypeDefinition() is MetadataType mdType && _metadataManager.CanGenerateMetadata(mdType))
return false;

constant = 0;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,16 @@ private void ImportBranch(ILOpcode opcode, BasicBlock target, BasicBlock fallthr
&& ConstructedEETypeNode.CreationAllowed(typeEqualityCheckType)
&& !typeEqualityCheckType.ConvertToCanonForm(CanonicalFormKind.Specific).IsCanonicalSubtype(CanonicalFormKind.Any))
{
condition = _factory.MaximallyConstructableType(typeEqualityCheckType);
// If the type could generate metadata, we set the condition to the presence of the metadata.
// This covers situations where the typeof is compared against metadata-only types.
// Note this assumes a constructed MethodTable always implies having metadata.
// This will likely remain true because anyone can call Object.GetType on a constructed type.
// If the type cannot generate metadata, we only condition on the MethodTable itself.
if (!_factory.MetadataManager.IsReflectionBlocked(typeEqualityCheckType)
&& typeEqualityCheckType.GetTypeDefinition() is MetadataType typeEqualityCheckMetadataType)
condition = _factory.TypeMetadata(typeEqualityCheckMetadataType);
else
condition = _factory.MaximallyConstructableType(typeEqualityCheckType);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/aot/ILCompiler/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ void RunScanner()

interopStubManager = scanResults.GetInteropStubManager(interopStateManager, pinvokePolicy);

ilProvider = new SubstitutedILProvider(unsubstitutedILProvider, substitutionProvider, devirtualizationManager);
ilProvider = new SubstitutedILProvider(unsubstitutedILProvider, substitutionProvider, devirtualizationManager, metadataManager);

// Use a more precise IL provider that uses whole program analysis for dead branch elimination
builder.UseILProvider(ilProvider);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -529,13 +529,27 @@ class TestTypeEqualityDeadBranchScanRemoval
{
class NeverAllocated1 { }
class NeverAllocated2 { }
class NeverAllocated3 { }

class PossiblyAllocated1 { }
class PossiblyAllocated2 { }

class MyAttribute : Attribute
{
public Type TheType;

public MyAttribute(Type t) => TheType = t;
}

[My(typeof(NeverAllocated3))]
class AttributeHolder { }

[MethodImpl(MethodImplOptions.NoInlining)]
static Type GetNeverObject() => null;

[MethodImpl(MethodImplOptions.NoInlining)]
static Type GetNeverAllocated3Type() => typeof(AttributeHolder).GetCustomAttribute<MyAttribute>().TheType;

static volatile Type s_sink;

public static void Run()
Expand All @@ -557,6 +571,15 @@ public static void Run()
}
if (Environment.GetEnvironmentVariable("SURETHING") != null)
s_sink = typeof(PossiblyAllocated1);

if (GetNeverAllocated3Type() == typeof(NeverAllocated3))
{
Console.WriteLine($"{nameof(NeverAllocated3)} check succeeded");
}
else
{
throw new Exception();
}
}
}

Expand Down

0 comments on commit a817006

Please sign in to comment.