Skip to content

[WIP] [ILVerify] Recognize type equivalence of two types with System.Runtime.InteropServices.TypeIdentifierAttribute #64413

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

Closed
TIHan wants to merge 3 commits into from
Closed

[WIP] [ILVerify] Recognize type equivalence of two types with System.Runtime.InteropServices.TypeIdentifierAttribute #64413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
511f9dc
ILVerify recognizes type equivalence of two types if they have a Syst…
TIHan Jan 27, 2022
6f2c03e
Minor cleanup
TIHan Jan 27, 2022
a30fcb0
Minor cleanup
TIHan Jan 27, 2022
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
20 changes: 18 additions & 2 deletions src/coreclr/tools/Common/TypeSystem/Common/TypeDesc.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ public override bool Equals(object o)
{
// Its only valid to compare two TypeDescs in the same context
Debug.Assert(o is not TypeDesc || object.ReferenceEquals(((TypeDesc)o).Context, this.Context));
if (o is TypeDesc)
return this.IsEquivalentTo((TypeDesc)o);
return object.ReferenceEquals(this, o);
}

Expand All @@ -33,14 +35,18 @@ public override bool Equals(object o)
{
// Its only valid to compare two TypeDescs in the same context
Debug.Assert(left is null || right is null || object.ReferenceEquals(left.Context, right.Context));
return object.ReferenceEquals(left, right);
if (left is null)
Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 28, 2022

Choose a reason for hiding this comment

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

This code only runs in debug builds (see 5 lines above) so that we can assert correct use. Equality comparison is done using referential equality for perf reasons. This fix won't work in release builds.

The fix would have to go somewhere around here:

runtime/src/coreclr/tools/Common/TypeSystem/Common/CastingHelper.cs

Lines 147 to 151 in 014b084

private static bool IsEquivalentTo(this TypeDesc thisType, TypeDesc otherType)
{
// TODO: Once type equivalence is implemented, this implementation needs to be enhanced to honor it.
return thisType == otherType;
}

I think a correct fix won't be trivial. The type system is meant to be general purpose and CastingHelper must not cast anything to EcmaType. It will require some thinking on the shape of the extentibility API. Do we need to fix this now?

Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 28, 2022

Choose a reason for hiding this comment

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

On a second thought, placing the code in IsEquivalentTo won't fix the problem of method resolution. This is a much bigger type system problem that I don't know how to solve without causing a major regression in type system performance. I suggest finding an easier problem to work on. This issue should be moved to Future.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 28, 2022

Choose a reason for hiding this comment

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

(Files under src/coreclr/Common/TypeSystem are shared with tools like crossgen2 that are very sensitive to type system performance. It may be acceptable for ILVerify to get slower, but crossgen2 and the other tools need to be fast.)

Copy link
Contributor Author

@TIHan TIHan Jan 28, 2022

Choose a reason for hiding this comment

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

Thank you for the feedback.

This fix won't work in release builds.

Yea, I missed that. Though, when a test gets added it would hopefully catch it.

placing the code in IsEquivalentTo won't fix the problem of method resolution.

I found that out too. At the moment, it seems the most correct way to do this is the additional equivalency check in == and !=.

It will require some thinking on the shape of the extentibility API.

Could you expand on this? I'm not sure what this is in relation to this change.

The majority of types do not have a TypeIdentifierAttribute, so constantly checking to see if the attribute exists might be too much considering this is being used by crossgen2. Instead, we could get this information a single time when the type gets imported and then look for that information if a type equivalency check fails.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Jan 28, 2022

Choose a reason for hiding this comment

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

Could you expand on this? I'm not sure what this is in relation to this change.

That comment was connected to the existing bool IsEquivalentTo method - it doesn't apply because we don't need to do this just in casting - we need to do this any time we compare two types.

Instead, we could get this information a single time when the type gets imported and then look for that information if a type equivalency check fails.

Types not being equal is a very common scenario in the type system. We often need to check whether two types are equal. Currently this is just a trivial reference equality check. Requiring a fallback that will start inspecting fields on the types to check whether they happen to be type equivalent will regress performance (extra CPU cycles, wasted cache lines, etc.).

AFAIK type equivalence is part of built-in COM interop that is Windows specific and we're trying to phase out. Even the issue that this pull request is trying to fix was hit on .NET Framework, not Core. I don't know whether it's a good enough reason to regress performance and add maintenance cost. That's why I'm suggesting going the "won't fix" route.

Cc @davidwrighton for second opinion

Copy link
Contributor Author

@TIHan TIHan Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

cc @JulieLeeMSFT - see above comment from @MichalStrehovsky

AFAIK type equivalence is part of built-in COM interop that is Windows specific and we're trying to phase out. Even the issue that this pull request is trying to fix was hit on .NET Framework, not Core. I don't know whether it's a good enough reason to regress performance and add maintenance cost. That's why I'm suggesting going the "won't fix" route.

Based on that, I think we can hold off on this change for a bit until we decide what we want to do. It isn't a critical fix.

return object.ReferenceEquals(left, right);
return left.IsEquivalentTo(right);
}

public static bool operator !=(TypeDesc left, TypeDesc right)
{
// Its only valid to compare two TypeDescs in the same context
Debug.Assert(left is null || right is null || object.ReferenceEquals(left.Context, right.Context));
return !object.ReferenceEquals(left, right);
if (left is null)
return !object.ReferenceEquals(left, right);
return !left.IsEquivalentTo(right);
}
#endif

Expand Down Expand Up @@ -692,5 +698,15 @@ public bool IsIDynamicInterfaceCastable
return (GetTypeFlags(TypeFlags.IsIDynamicInterfaceCastable | TypeFlags.IsIDynamicInterfaceCastableComputed) & TypeFlags.IsIDynamicInterfaceCastable) != 0;
}
}

/// <summary>
/// Determines whether two types have the same identity and are eligible for type equivalence.
/// </summary>
public virtual bool IsEquivalentTo(TypeDesc typeDesc)
{
// Its only valid to compare two TypeDescs in the same context
Debug.Assert(typeDesc is null || object.ReferenceEquals(this.Context, typeDesc.Context));
return object.ReferenceEquals(this, typeDesc);
}
}
}
52 changes: 52 additions & 0 deletions src/coreclr/tools/Common/TypeSystem/Ecma/EcmaType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,5 +604,57 @@ public override PInvokeStringFormat PInvokeStringFormat
return (PInvokeStringFormat)(_typeDefinition.Attributes & TypeAttributes.StringFormatMask);
}
}

public override bool IsEquivalentTo(TypeDesc typeDesc)
{
if (base.IsEquivalentTo(typeDesc))
return true;

if (typeDesc is EcmaType)
return CheckEquivalence(this, (EcmaType)typeDesc);

return false;

static bool CheckEquivalence(EcmaType type1, EcmaType type2)
{
if (!type1.HasCustomAttribute("System.Runtime.InteropServices", "TypeIdentifierAttribute"))
return false;

if (!type2.HasCustomAttribute("System.Runtime.InteropServices", "TypeIdentifierAttribute"))
return false;

var attrOpt1 = type1.GetDecodedCustomAttribute("System.Runtime.InteropServices", "TypeIdentifierAttribute");
var attrOpt2 = type2.GetDecodedCustomAttribute("System.Runtime.InteropServices", "TypeIdentifierAttribute");
Debug.Assert(attrOpt1.HasValue);
Debug.Assert(attrOpt2.HasValue);
var attr1 = attrOpt1.Value;
var attr2 = attrOpt2.Value;

if (attr1.FixedArguments.Length != 2 || attr2.FixedArguments.Length != 2)
return false;

var scope1 = attr1.FixedArguments[0].Value as string;
var identifier1 = attr1.FixedArguments[1].Value as string;
var scope2 = attr2.FixedArguments[0].Value as string;
var identifier2 = attr2.FixedArguments[1].Value as string;

if (scope1 == null || identifier1 == null || scope2 == null || identifier2 == null)
return false;

if (identifier1 != identifier2)
return false;

if (!Guid.TryParse(scope1, out var scopeGuidValue1))
return false;

if (!Guid.TryParse(scope2, out var scopeGuidValue2))
return false;

if (scopeGuidValue1 != scopeGuidValue2)
return false;

return true;
}
}
}
}