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

[TIHan]

Resolves this: #62732

Description

ILVerify would fail verification of an assembly when two types, one in the assembly and one in a different assembly, had equivalent TypeIdentifierAttribute data.

Acceptance Criteria

• Add Tests

[MichalStrehovsky]

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?

[MichalStrehovsky]

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.

[MichalStrehovsky]

(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.)

[TIHan]

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.

[MichalStrehovsky]

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

[TIHan]

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.

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Resolves this: #62732

Description

ILVerify would fail verification of an assembly when two types, one in the assembly and one in a different assembly, had equivalent TypeIdentifierAttribute data.

Acceptance Criteria

• Add Tests

Author:	TIHan
Assignees:	TIHan
Labels:	area-crossgen2-coreclr, area-ILVerification
Milestone:	Future

[ghost]

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.