Uh oh!
There was an error while loading. Please reload this page.
[release/6.0] Fix Crossgen2 bug #61104 and add regression test - #64027
Merged
Conversation
The issue tracks the runtime regression failure where Crossgen2-compiled app is unable to locate a type with non-ASCII characters in its name. The failure was caused by the fact that Crossgen2 was incorrectly zero-extended the individual characters when calculating the hash whereas runtime is sign-extending them. Thanks Tomas
mangod9
approved these changes
Jan 20, 2022
jeffschwMSFT
approved these changes
Jan 20, 2022
jeffschwMSFT
left a comment
Member
There was a problem hiding this comment.
Approved. We should take for consideration in 6.0.x
jeffschwMSFT
commented
Jan 20, 2022
Member
Eek, it is not the right time to merge (quite yet). We need to wait for a checkin window and request approval. @mmitche to help stop this from flowing. |
trylek
commented
Jan 20, 2022
Member
Hmm, sorry about that, I was under the impression that the actual move to the servicing release will happen as a subsequent step. Thanks Jeff for letting me know, I'll remember it the next time. |
Tragetaschen
commented
Feb 11, 2022
Contributor
Are there builds with this fix available yet? |
jeffschwMSFT
commented
Feb 11, 2022
Member
@Tragetaschen not yet, these builds will start to show up on https://github.com/dotnet/installer in the coming week. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #63956 to release/6.0
/cc @mangod9@trylek
Customer Impact
Fixes issue reported by multiple customers where Crossgen2-compiled apps fail runtime reflection lookup for types with non-ASCII characters in their names, in particular:
#61104
#63193
#63481
Testing
As part of fixing the bug I locally reproed the failure in 61104 and I used comparative debugging to observe the native runtime vs. Crossgen2 code path that revealed the subtle difference in type hash code calculation. After fixing the discrepancy I locally observed the hash codes getting in sync between the CoreCLR runtime and Crossgen2 even for types with names containing non-ASCII characters. As part of the fix I created a regression test based on the bug report 61104 and I ran a successful [modulo minor infra hiccups] outerloop run.
Risk
Extremely low - the fix only affects a corner case of type names containing non-ASCII characters; the two algorithms, their previous divergence and its fix in the PR proposed for porting are well understood and quite trivial to reason about; the fix is well localized and literally involves about 20 characters in a single method.