Type equivalence support in Crossgen2 #87899
Conversation
…uivalence_crossgen2
…andling so that problematic type equivalence cases will cause a runtime jit fallback
…uivalence_crossgen2_modern
Cache computation of type equivalence on EcmaType Fix build, and do first pass of code review
| return null; | ||
| return _data; | ||
| } | ||
| lock(this) |
There was a problem hiding this comment.
We don't generally lock on type system objects because with enough unchecked locks we'll eventually get deadlocks (see another spot where I'm policing this: #87532 (comment)). Would CompareExchange be unacceptable?
There was a problem hiding this comment.
Yes. This was just a bit of laziness on my part.
|
@trylek the tests are passing, (except for a couple mono issues that are known). Could you take a look and let me know what you can sign off on, and what I need to find someone else to sign off on? |
| } | ||
|
|
||
| // Generic equivalence only allows the instantiation to be non-equal | ||
| return thisType.HasSameTypeDefinition(otherType); |
There was a problem hiding this comment.
Perf nit - wouldn't it be faster to first check whether the two types have the same type definition and only then traverse the instantiations?
There was a problem hiding this comment.
Good point. Changed
| return false; | ||
| if (type1.IsInterface) | ||
| { | ||
| if (!type2.IsInterface) |
There was a problem hiding this comment.
Nit - I believe this check is superfluous; you have already made sure at line 132 that type1.IsInterface is the same as type2.IsInterface so it seems to me that you should be able to safely return true in this conditional statement.
|
|
||
| return true; | ||
| } | ||
| else if ((type1.IsEnum != type2.IsEnum) || (type1.IsValueType != type2.IsValueType)) |
There was a problem hiding this comment.
Nit - it might make the code slightly easier to read if you removed the else statement at line 141 - the previous conditional statements act as "filters" that either directly return the result or proceed to the next statement.
| } | ||
|
|
||
| // Compare the field signatures for equivalence | ||
| // TODO: Technically this comparison should include custom modifiers on the field signatures |
There was a problem hiding this comment.
Is this a real possibility that we should track with an issue?
There was a problem hiding this comment.
Eh, no. We don't even check field types on fields correctly today for normal field references, much less custom modifiers.
| return field; | ||
|
|
||
| // Only public instance fields, and literal fields on enums are permitted in type equivalent structures | ||
| if (!enumMode || field.IsLiteral) |
There was a problem hiding this comment.
I may be missing something but shouldn't this check be negated? I would assume that !field.IsLiteral is what signifies that the type doesn't support equivalence.
There was a problem hiding this comment.
Yes. Good catch, that inspired me to build a whole test suite, so I found some more issues. Sigh. Thank you though.
| @@ -284,6 +294,21 @@ private bool Equals(MethodSignature otherSignature, bool allowCovariantReturn) | |||
| } | |||
|
|
|||
| return false; | |||
|
|
|||
| static bool CompareTypeHelper(TypeDesc type1, TypeDesc type2, bool allowEquivalence, StackOverflowProtect visited) | |||
There was a problem hiding this comment.
Naming nit - both my old C++ background and Compare-like C# APIs typically return plus / minus / zero result of the comparison whereas this feels more like something in the vein of IsTypeEqualHelper. Not a big deal but I had to fight this mental model half a dozen times while reviewing this change.
There was a problem hiding this comment.
In general the Crossgen2 logic looks reasonable to me. I'm not a COM expert so that the craziness around GUIDs is somewhat beyond my comprehension, I guess Aaron would be a great pick if you're after more detailed scrutiny in this area. I have shared a couple of observations including one place that I believe to be a bug but I'm not blocking my approval on that as I'm well aware of your long-standing engineering diligence in addressing all review feedback.
- Share test suite with runtime Fix some issues found in type equivalence comparison found while building said test suite
…ization in crossgen2
| int field; | ||
| } | ||
|
|
||
| // A case where the type is not both public |
There was a problem hiding this comment.
Nit - I'm missing the purpose of the word both in this code comment (unless both public is a technical term I haven't yet come across); dtto for the next two test cases.
ghost
locked as resolved and limited conversation to collaborators
We currently have a small behavior hole on Windows where if a binary uses TypeEquivalence, it is technically possible to get crossgen2 to generate invalid code.
This fixes that by adding support for type equivalence to the managed type system, and adjusting crossgen2 to use it. This is complicated by the detail that this may generate some type references which cannot be referenced from an R2R file, so additional error checking was needed (this additional error checking is what fixes #67855).
However, while basic support for type equivalence has been added to the type system, support for actually performing type equivalent interface resolution has not been, so devirtualization of type equivalent interface calls is disabled. At this time, I do not expect to see a customer need for implementing that fairly complicated feature
Fixes #67855