Skip to content

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

Merged
carlossanlop merged 1 commit into
dotnet:release/7.0from
EgorBo:fix-equals-unroll-bug-7
Nov 14, 2022
Merged

[release/7.0] Fix bug in String.Equals unrolling for CJK single-char strings#78234
carlossanlop merged 1 commit into
dotnet:release/7.0from
EgorBo:fix-equals-unroll-bug-7

Conversation

@EgorBo

@EgorBoEgorBo commented Nov 11, 2022

Copy link
Copy Markdown
Member

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.

constStringv1="鉄";varv2=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

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 11, 2022
@carlossanlop

carlossanlop commented Nov 11, 2022

Copy link
Copy Markdown
Contributor

@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.

@ghostghost assigned EgorBoNov 11, 2022
@ghost

Copy link
Copy Markdown

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.

constStringv1="鉄";varv2=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

EgorBo commented Nov 11, 2022

Copy link
Copy Markdown
MemberAuthor

@jakobbotsch PTAL

@EgorBoEgorBo added the Servicing-consider Issue for next servicing release review label Nov 14, 2022

@jeffschwMSFTjeffschwMSFT left a comment

Copy link
Copy Markdown
Member

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

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

Copy link
Copy Markdown
Contributor

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

@carlossanlopcarlossanlop added this to the 7.0.1 milestone Nov 14, 2022
@carlossanlop
carlossanlop merged commit 89161f4 into dotnet:release/7.0Nov 14, 2022
@ghostghost locked as resolved and limited conversation to collaborators Dec 15, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@EgorBo@carlossanlop@jakobbotsch@jeffschwMSFT