Skip to content
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.

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

[release/7.0] Fix bug in String.Equals unrolling for CJK single-char strings #78234

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Nov 11, 2022

Manual backport of #78190 to release/7.0

Customer Impact

Fixes #78169 regression introduced in the String.Equals/StartsWith unrolling optimization in NET 7.0.

const String v1 = "";
var v2 = v1;
Console.WriteLine($"{v1 == v2}, {v1.Equals(v2, StringComparison.Ordinal)}, {v1.Equals(v2,StringComparison.OrdinalIgnoreCase)}");

Prints different values depending on code being optimized or not (by jit). All comparisons against single-char literals of certain symbols (all two-bytes chars which pass this check: bool IsAffected(char c) => (((ushort)c) >> 15) == 1;) of CJK languages are affected so we probably want to land the fix it as soon as possible.

Testing

We had a pretty big test coverage for all sorts of inputs for that optimizations, unfortunately, for the non-ASCII one we didn't use any of CJK symbols. Added CJK and did a fuzzy testing locally via a hand-written test generator for this API.

Risk

Low

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 11, 2022
@carlossanlop
Copy link
Member

carlossanlop commented Nov 11, 2022

@EgorBo when this is ready, please send the email to Tactics, add the servicing-consider label, and get a code review sign-off. The deadline for merging backports to include in the December release is Monday.

@ghost ghost assigned EgorBo Nov 11, 2022
@ghost
Copy link

ghost commented Nov 11, 2022

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

Issue Details

Backport of #78190 to release/7.0

Customer Impact

Fixes #78169 .NET 7.0 regression introduced in the String.Equals unrolling optimization.

const String v1 = "";
var v2 = v1;
Console.WriteLine($"{v1 == v2}, {v1.Equals(v2, StringComparison.Ordinal)}, {v1.Equals(v2,StringComparison.OrdinalIgnoreCase)}");

Prints different values depending on code being optimized or not (by jit). All comparisons against single-char literals of certain symbols of CJK languages are affected so we probably want to land the fix it as soon as possible.

Testing

We had a pretty big test coverage for all sorts of inputs for that optimizations, unfortunately, for the non-ASCII one we didn't use any of CJK symbols. Added CJK and did a fuzzy testing locally via a hand-written test generator for this API.

Risk

Low

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Nov 11, 2022

@jakobbotsch PTAL

@EgorBo EgorBo added the Servicing-consider Issue for next servicing release review label Nov 14, 2022
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 7.0.x

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 14, 2022
@carlossanlop
Copy link
Member

Approved by Tactics via email.
Signed off by area owners.
CI is green.
No OOB package authoring changes needed.
Ready to merge. :shipit:

@carlossanlop carlossanlop added this to the 7.0.1 milestone Nov 14, 2022
@carlossanlop carlossanlop merged commit 89161f4 into dotnet:release/7.0 Nov 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants