Skip to content

Prep CompareInfo for spanification - #32385

Merged
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom
GrabYourPitchforks:spanify_compareinfo
Mar 14, 2020
Merged

Prep CompareInfo for spanification#32385
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom
GrabYourPitchforks:spanify_compareinfo

Conversation

@GrabYourPitchforks

@GrabYourPitchforksGrabYourPitchforks commented Feb 16, 2020•
edited
Loading

Copy link
Copy Markdown
Member

This removes many (not all, see #8890) of the string-based p/invoke wrappers in CompareInfo and replaces them with span-based wrappers.

There's a little more error checking around the actual p/invokes themselves, but otherwise the intent of this PR is that there's no behavioral change to the existing surface area, and there's no new API being exposed.

This PR serves as the shared foundation for upcoming work such as #25428, #27912, and #27935.

This PR also reverts a small optimization we did to SortKey in #31779, since I was already touching SortKey-related code paths here.

jkotas
jkotas reviewed Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs Outdated
jkotas
jkotas reviewed Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
tarekgh
tarekgh reviewed Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs Outdated
@GrabYourPitchforks

GrabYourPitchforks commented Feb 21, 2020

Copy link
Copy Markdown
MemberAuthor

Over the next few days I'll get the perf measurements for the change with SetLastError = true. Been heads-down with other work.

I don't expect this to have significant impact on typical API calls to string since we already special-case the common code paths of ordinal comparisons and all-ASCII data. These changes would primarily impact non-ASCII and culture-aware text processing code paths.

Would it be a strange behavioral difference to check last error only in debug builds? If we went this route I imagine we'd want to Debug.Assert rather than throw an exception, since that would give the smallest behavioral difference between debug and release. Do we follow this pattern elsewhere in the framework?

@jkotas

jkotas commented Feb 21, 2020

Copy link
Copy Markdown
Member

If we went this route I imagine we'd want to Debug.Assert rather than throw an exception

Yep.

@GrabYourPitchforksGrabYourPitchforks mentioned this pull request Mar 13, 2020
Merged
@GrabYourPitchforks

GrabYourPitchforks commented Mar 13, 2020

Copy link
Copy Markdown
MemberAuthor

Aside from the GetLastError in debug part is there any other feedback for this?

tarekgh
tarekgh reviewed Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs Outdated
@tarekgh

tarekgh commented Mar 13, 2020

Copy link
Copy Markdown
Member

I added minor comments. LGTM otherwise. Thanks for your effort with it.

@GrabYourPitchforks

GrabYourPitchforks commented Mar 13, 2020

Copy link
Copy Markdown
MemberAuthor

I believe all the feedback is now addressed. Waiting for CI to go green.

jkotas
jkotas reviewed Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs Outdated
char* lpStringValue,
int cchValue,
int bIgnoreCase);
bool bIgnoreCase);

@jkotasjkotasMar 13, 2020

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.

This will add marshaling frame. Does it have effect on performance for small strings?

You can use upper-case BOOL to get bool that is both zero-overhead and self-describing.

@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

You mean apply [MarshalAs(BOOL)] to avoid the marshaling frame?

@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Oh, we have an actual internal BOOL enum. Got it.

@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I almost want to define BOOL.FromBoolean(bool value) => Unsafe.As<bool, BOOL>(ref value) to avoid the branching at the call sites. But that's probably an issue for a different PR.

@jkotasjkotasMar 13, 2020

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.

And it does not work. bool and BOOL have different sizes.

@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sorry, Unsafe.As<bool, byte>(ref value). The same thing we tried back in dotnet/coreclr#16138 but was killed.

jkotas
jkotas reviewed Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
string? localeName = _sortHandle != IntPtr.Zero ? null : _sortName;

// CompareStringEx may try to dereference the first character of its input, even if an explicit
// length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs

@jkotasjkotasMar 13, 2020

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.

Really? What is the input that it happens for?

@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Create a new C++ application and drop in this code. It will AV the process.

auto result = CompareStringEx(
LOCALE_NAME_INVARIANT,
NORM_LINGUISTIC_CASING,
L"x",
1,
(LPCWCH)0xdeadbeef,
0,
nullptr,
nullptr,
0);

Edit: I reported it to the Windows team and it's being fixed in a future release. But we pessimistically have to work around it for now.

jkotas
jkotas reviewed Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs

// CompareStringEx may try to dereference the first character of its input, even if an explicit
// length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
// are normalized to a null-terminated empty string.

@jkotasjkotasMar 13, 2020

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.

Do we have test coverage for all these empty / null string corner cases?

@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes. The unit tests that are part of #1514 (from which this PR was broken out as a more manageable chunk) cover this scenario. In fact, those unit tests are where the problem was originally discovered. :)

Edit: Looks like https://github.com/dotnet/runtime/blob/master/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs also would cover this scenario when the larger "don't early-exit inappropriately" PR comes online.

@GrabYourPitchforks

GrabYourPitchforks commented Mar 13, 2020

Copy link
Copy Markdown
MemberAuthor

I'm a little confused as to why CI is failing here. There are some things that look suspicious. From the Helix logs:

/private/tmp/helix/working/A96E0923/w/AB700940/e /private/tmp/helix/working/A96E0923/w/AB700940/e
Discovering: System.Memory.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Memory.Tests (found 2523 of 2547 test cases)
Starting: System.Memory.Tests (parallel test collections = on, max threads = 4)
Process terminated. Assertion failed.
at System.Globalization.CompareInfo.IndexOfOrdinalCore(ReadOnlySpan`1 source, ReadOnlySpan`1 value, Boolean ignoreCase, Boolean fromBeginning) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs:line 48
at System.Globalization.CompareInfo.IndexOfOrdinal(String source, String value, Int32 startIndex, Int32 count, Boolean ignoreCase) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs:line 1115
at System.Tests.StringTests.IndexOf_AllSubstrings(String s, String value, Int32 startIndex, StringComparison comparison) in /_/src/libraries/Common/tests/Tests/System/StringTests.cs:line 2801

Since the failure is at CompareInfo.Unix.cs, line 48, the assertion that's failing is the !GlobalizationMode.Invariant check below.

https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs#L46-L49

According to the stack trace the caller is CompareInfo.cs, line 1115, but there's no relevant code at that line. It's just another Debug.Assert statement. (Presumably that assertion succeeded, else control flow wouldn't have continued.)

https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1113-L1116

There are only three places where we call IndexOfOrdinalCore. All three are guarded with either the same Debug.Assert(!GlobalizationMode.Invariant) (which clearly didn't fail) or with an explicit if check to route control flow appropriately.

https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1130-L1137

https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L969-L984

@jkotas, @tarekgh, any thoughts as to why CI might report such an error? And why the error would occur only on OSX and not any other Unix platform?

@jkotas

jkotas commented Mar 13, 2020

Copy link
Copy Markdown
Member

I do not see anything obvious. It can be a JIT bug or data corruption - lots of unsafe code in globalization.

@tarekgh

tarekgh commented Mar 13, 2020

Copy link
Copy Markdown
Member

maybe try to add some messages to the asserts in the involved files which may help give some clue.

@tarekgh

tarekgh commented Mar 13, 2020

Copy link
Copy Markdown
Member

The other idea could be trying repro it on OSX machine

@GrabYourPitchforks

GrabYourPitchforks commented Mar 13, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

I'll investigate a little more with the assumption that the stack trace is lying. :)
Edit: Yes, turned out that the stack trace was indeed reporting incorrect line numbers. I've put a new commit to work around the edge cases that were hitting this assert.

@GrabYourPitchforks
GrabYourPitchforks merged commit faf08f7 into dotnet:masterMar 14, 2020
@GrabYourPitchforks
GrabYourPitchforks deleted the spanify_compareinfo branch March 14, 2020 02:43
@GrabYourPitchforksGrabYourPitchforks mentioned this pull request Mar 14, 2020
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas approved these changes
@tarekghtarekghtarekgh approved these changes

Assignees

No one assigned

Labels

area-System.Globalization

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@jkotas@tarekgh
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Prep CompareInfo for spanification by GrabYourPitchforks · Pull Request #32385 · dotnet/runtime · GitHub
Skip to content
Prep CompareInfo for spanification - #32385
Merged
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Mar 14, 2020
Merged
Prep CompareInfo for spanification#32385
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Feb 16, 2020•

edited

Loading

Copy link


Copy Markdown

Member
This removes many (not all, see #8890) of the string-based p/invoke wrappers in CompareInfo and replaces them with span-based wrappers.
There's a little more error checking around the actual p/invokes themselves, but otherwise the intent of this PR is that there's no behavioral change to the existing surface area, and there's no new API being exposed.
This PR serves as the shared foundation for upcoming work such as #25428, #27912, and #27935.
This PR also reverts a small optimization we did to SortKey in #31779, since I was already touching SortKey-related code paths here.
jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
tarekgh
tarekgh
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
Outdated

@GrabYourPitchforks
GrabYourPitchforks
commented
Feb 21, 2020

Copy link


Copy Markdown

MemberAuthor
Over the next few days I'll get the perf measurements for the change with SetLastError = true. Been heads-down with other work.
I don't expect this to have significant impact on typical API calls to string since we already special-case the common code paths of ordinal comparisons and all-ASCII data. These changes would primarily impact non-ASCII and culture-aware text processing code paths.
Would it be a strange behavioral difference to check last error only in debug builds? If we went this route I imagine we'd want to Debug.Assert rather than throw an exception, since that would give the smallest behavioral difference between debug and release. Do we follow this pattern elsewhere in the framework?
@jkotas
jkotas
commented
Feb 21, 2020

Copy link


Copy Markdown

Member
If we went this route I imagine we'd want to Debug.Assert rather than throw an exception
Yep.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 13, 2020
 Merged

@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Aside from the GetLastError in debug part is there any other feedback for this?
tarekgh
tarekgh
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I added minor comments. LGTM otherwise. Thanks for your effort with it.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I believe all the feedback is now addressed. Waiting for CI to go green.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

 char* lpStringValue,
 int cchValue,
int bIgnoreCase);
bool bIgnoreCase);
@jkotasjkotasMar 13, 2020

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.

This will add marshaling frame. Does it have effect on performance for small strings?
You can use upper-case BOOL to get bool that is both zero-overhead and self-describing.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

You mean apply [MarshalAs(BOOL)] to avoid the marshaling frame?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Oh, we have an actual internal BOOL enum. Got it.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I almost want to define BOOL.FromBoolean(bool value) => Unsafe.As<bool, BOOL>(ref value) to avoid the branching at the call sites. But that's probably an issue for a different PR.
@jkotasjkotasMar 13, 2020

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.

And it does not work. bool and BOOL have different sizes.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, Unsafe.As<bool, byte>(ref value). The same thing we tried back in dotnet/coreclr#16138 but was killed.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
 string? localeName = _sortHandle != IntPtr.Zero ? null : _sortName;

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
@jkotasjkotasMar 13, 2020

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.

Really? What is the input that it happens for?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Create a new C++ application and drop in this code. It will AV the process.
auto result = CompareStringEx(
LOCALE_NAME_INVARIANT,
NORM_LINGUISTIC_CASING,
L"x",
1,
(LPCWCH)0xdeadbeef,
0,
nullptr,
nullptr,
0);
Edit: I reported it to the Windows team and it's being fixed in a future release. But we pessimistically have to work around it for now.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
 // are normalized to a null-terminated empty string.
@jkotasjkotasMar 13, 2020

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.

Do we have test coverage for all these empty / null string corner cases?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. The unit tests that are part of #1514 (from which this PR was broken out as a more manageable chunk) cover this scenario. In fact, those unit tests are where the problem was originally discovered. :)
Edit: Looks like https://github.com/dotnet/runtime/blob/master/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs also would cover this scenario when the larger "don't early-exit inappropriately" PR comes online.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I'm a little confused as to why CI is failing here. There are some things that look suspicious. From the Helix logs:
/private/tmp/helix/working/A96E0923/w/AB700940/e /private/tmp/helix/working/A96E0923/w/AB700940/e
Discovering: System.Memory.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Memory.Tests (found 2523 of 2547 test cases)
Starting: System.Memory.Tests (parallel test collections = on, max threads = 4)
Process terminated. Assertion failed.
at System.Globalization.CompareInfo.IndexOfOrdinalCore(ReadOnlySpan`1 source, ReadOnlySpan`1 value, Boolean ignoreCase, Boolean fromBeginning) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs:line 48
at System.Globalization.CompareInfo.IndexOfOrdinal(String source, String value, Int32 startIndex, Int32 count, Boolean ignoreCase) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs:line 1115
at System.Tests.StringTests.IndexOf_AllSubstrings(String s, String value, Int32 startIndex, StringComparison comparison) in /_/src/libraries/Common/tests/Tests/System/StringTests.cs:line 2801
Since the failure is at CompareInfo.Unix.cs, line 48, the assertion that's failing is the !GlobalizationMode.Invariant check below.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs#L46-L49
According to the stack trace the caller is CompareInfo.cs, line 1115, but there's no relevant code at that line. It's just another Debug.Assert statement. (Presumably that assertion succeeded, else control flow wouldn't have continued.)
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1113-L1116
There are only three places where we call IndexOfOrdinalCore. All three are guarded with either the same Debug.Assert(!GlobalizationMode.Invariant) (which clearly didn't fail) or with an explicit if check to route control flow appropriately.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1130-L1137
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L969-L984
@jkotas, @tarekgh, any thoughts as to why CI might report such an error? And why the error would occur only on OSX and not any other Unix platform?
@jkotas
jkotas
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I do not see anything obvious. It can be a JIT bug or data corruption - lots of unsafe code in globalization.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
maybe try to add some messages to the asserts in the involved files which may help give some clue.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
The other idea could be trying repro it on OSX machine
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll investigate a little more with the assumption that the stack trace is lying. :)

Edit: Yes, turned out that the stack trace was indeed reporting incorrect line numbers. I've put a new commit to work around the edge cases that were hitting this assert.
@GrabYourPitchforks
GrabYourPitchforks
merged commit faf08f7
into
dotnet:masterMar 14, 2020
@GrabYourPitchforks
GrabYourPitchforks
deleted the

spanify_compareinfo

branch
March 14, 2020 02:43
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 14, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 10, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@tarekghtarekghtarekgh approved these changes

Assignees


No one assigned


Labels

area-System.Globalization

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@jkotas@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Prep CompareInfo for spanification by GrabYourPitchforks · Pull Request #32385 · dotnet/runtime · GitHub
Skip to content
Prep CompareInfo for spanification - #32385
Merged
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Mar 14, 2020
Merged
Prep CompareInfo for spanification#32385
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Feb 16, 2020•

edited

Loading

Copy link


Copy Markdown

Member
This removes many (not all, see #8890) of the string-based p/invoke wrappers in CompareInfo and replaces them with span-based wrappers.
There's a little more error checking around the actual p/invokes themselves, but otherwise the intent of this PR is that there's no behavioral change to the existing surface area, and there's no new API being exposed.
This PR serves as the shared foundation for upcoming work such as #25428, #27912, and #27935.
This PR also reverts a small optimization we did to SortKey in #31779, since I was already touching SortKey-related code paths here.
jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
tarekgh
tarekgh
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
Outdated

@GrabYourPitchforks
GrabYourPitchforks
commented
Feb 21, 2020

Copy link


Copy Markdown

MemberAuthor
Over the next few days I'll get the perf measurements for the change with SetLastError = true. Been heads-down with other work.
I don't expect this to have significant impact on typical API calls to string since we already special-case the common code paths of ordinal comparisons and all-ASCII data. These changes would primarily impact non-ASCII and culture-aware text processing code paths.
Would it be a strange behavioral difference to check last error only in debug builds? If we went this route I imagine we'd want to Debug.Assert rather than throw an exception, since that would give the smallest behavioral difference between debug and release. Do we follow this pattern elsewhere in the framework?
@jkotas
jkotas
commented
Feb 21, 2020

Copy link


Copy Markdown

Member
If we went this route I imagine we'd want to Debug.Assert rather than throw an exception
Yep.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 13, 2020
 Merged

@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Aside from the GetLastError in debug part is there any other feedback for this?
tarekgh
tarekgh
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I added minor comments. LGTM otherwise. Thanks for your effort with it.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I believe all the feedback is now addressed. Waiting for CI to go green.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

 char* lpStringValue,
 int cchValue,
int bIgnoreCase);
bool bIgnoreCase);
@jkotasjkotasMar 13, 2020

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.

This will add marshaling frame. Does it have effect on performance for small strings?
You can use upper-case BOOL to get bool that is both zero-overhead and self-describing.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

You mean apply [MarshalAs(BOOL)] to avoid the marshaling frame?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Oh, we have an actual internal BOOL enum. Got it.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I almost want to define BOOL.FromBoolean(bool value) => Unsafe.As<bool, BOOL>(ref value) to avoid the branching at the call sites. But that's probably an issue for a different PR.
@jkotasjkotasMar 13, 2020

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.

And it does not work. bool and BOOL have different sizes.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, Unsafe.As<bool, byte>(ref value). The same thing we tried back in dotnet/coreclr#16138 but was killed.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
 string? localeName = _sortHandle != IntPtr.Zero ? null : _sortName;

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
@jkotasjkotasMar 13, 2020

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.

Really? What is the input that it happens for?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Create a new C++ application and drop in this code. It will AV the process.
auto result = CompareStringEx(
LOCALE_NAME_INVARIANT,
NORM_LINGUISTIC_CASING,
L"x",
1,
(LPCWCH)0xdeadbeef,
0,
nullptr,
nullptr,
0);
Edit: I reported it to the Windows team and it's being fixed in a future release. But we pessimistically have to work around it for now.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
 // are normalized to a null-terminated empty string.
@jkotasjkotasMar 13, 2020

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.

Do we have test coverage for all these empty / null string corner cases?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. The unit tests that are part of #1514 (from which this PR was broken out as a more manageable chunk) cover this scenario. In fact, those unit tests are where the problem was originally discovered. :)
Edit: Looks like https://github.com/dotnet/runtime/blob/master/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs also would cover this scenario when the larger "don't early-exit inappropriately" PR comes online.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I'm a little confused as to why CI is failing here. There are some things that look suspicious. From the Helix logs:
/private/tmp/helix/working/A96E0923/w/AB700940/e /private/tmp/helix/working/A96E0923/w/AB700940/e
Discovering: System.Memory.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Memory.Tests (found 2523 of 2547 test cases)
Starting: System.Memory.Tests (parallel test collections = on, max threads = 4)
Process terminated. Assertion failed.
at System.Globalization.CompareInfo.IndexOfOrdinalCore(ReadOnlySpan`1 source, ReadOnlySpan`1 value, Boolean ignoreCase, Boolean fromBeginning) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs:line 48
at System.Globalization.CompareInfo.IndexOfOrdinal(String source, String value, Int32 startIndex, Int32 count, Boolean ignoreCase) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs:line 1115
at System.Tests.StringTests.IndexOf_AllSubstrings(String s, String value, Int32 startIndex, StringComparison comparison) in /_/src/libraries/Common/tests/Tests/System/StringTests.cs:line 2801
Since the failure is at CompareInfo.Unix.cs, line 48, the assertion that's failing is the !GlobalizationMode.Invariant check below.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs#L46-L49
According to the stack trace the caller is CompareInfo.cs, line 1115, but there's no relevant code at that line. It's just another Debug.Assert statement. (Presumably that assertion succeeded, else control flow wouldn't have continued.)
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1113-L1116
There are only three places where we call IndexOfOrdinalCore. All three are guarded with either the same Debug.Assert(!GlobalizationMode.Invariant) (which clearly didn't fail) or with an explicit if check to route control flow appropriately.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1130-L1137
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L969-L984
@jkotas, @tarekgh, any thoughts as to why CI might report such an error? And why the error would occur only on OSX and not any other Unix platform?
@jkotas
jkotas
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I do not see anything obvious. It can be a JIT bug or data corruption - lots of unsafe code in globalization.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
maybe try to add some messages to the asserts in the involved files which may help give some clue.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
The other idea could be trying repro it on OSX machine
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll investigate a little more with the assumption that the stack trace is lying. :)

Edit: Yes, turned out that the stack trace was indeed reporting incorrect line numbers. I've put a new commit to work around the edge cases that were hitting this assert.
@GrabYourPitchforks
GrabYourPitchforks
merged commit faf08f7
into
dotnet:masterMar 14, 2020
@GrabYourPitchforks
GrabYourPitchforks
deleted the

spanify_compareinfo

branch
March 14, 2020 02:43
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 14, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 10, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@tarekghtarekghtarekgh approved these changes

Assignees


No one assigned


Labels

area-System.Globalization

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@jkotas@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Prep CompareInfo for spanification by GrabYourPitchforks · Pull Request #32385 · dotnet/runtime · GitHub
Skip to content
Prep CompareInfo for spanification - #32385
Merged
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Mar 14, 2020
Merged
Prep CompareInfo for spanification#32385
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Feb 16, 2020•

edited

Loading

Copy link


Copy Markdown

Member
This removes many (not all, see #8890) of the string-based p/invoke wrappers in CompareInfo and replaces them with span-based wrappers.
There's a little more error checking around the actual p/invokes themselves, but otherwise the intent of this PR is that there's no behavioral change to the existing surface area, and there's no new API being exposed.
This PR serves as the shared foundation for upcoming work such as #25428, #27912, and #27935.
This PR also reverts a small optimization we did to SortKey in #31779, since I was already touching SortKey-related code paths here.
jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
tarekgh
tarekgh
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
Outdated

@GrabYourPitchforks
GrabYourPitchforks
commented
Feb 21, 2020

Copy link


Copy Markdown

MemberAuthor
Over the next few days I'll get the perf measurements for the change with SetLastError = true. Been heads-down with other work.
I don't expect this to have significant impact on typical API calls to string since we already special-case the common code paths of ordinal comparisons and all-ASCII data. These changes would primarily impact non-ASCII and culture-aware text processing code paths.
Would it be a strange behavioral difference to check last error only in debug builds? If we went this route I imagine we'd want to Debug.Assert rather than throw an exception, since that would give the smallest behavioral difference between debug and release. Do we follow this pattern elsewhere in the framework?
@jkotas
jkotas
commented
Feb 21, 2020

Copy link


Copy Markdown

Member
If we went this route I imagine we'd want to Debug.Assert rather than throw an exception
Yep.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 13, 2020
 Merged

@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Aside from the GetLastError in debug part is there any other feedback for this?
tarekgh
tarekgh
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I added minor comments. LGTM otherwise. Thanks for your effort with it.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I believe all the feedback is now addressed. Waiting for CI to go green.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

 char* lpStringValue,
 int cchValue,
int bIgnoreCase);
bool bIgnoreCase);
@jkotasjkotasMar 13, 2020

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.

This will add marshaling frame. Does it have effect on performance for small strings?
You can use upper-case BOOL to get bool that is both zero-overhead and self-describing.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

You mean apply [MarshalAs(BOOL)] to avoid the marshaling frame?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Oh, we have an actual internal BOOL enum. Got it.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I almost want to define BOOL.FromBoolean(bool value) => Unsafe.As<bool, BOOL>(ref value) to avoid the branching at the call sites. But that's probably an issue for a different PR.
@jkotasjkotasMar 13, 2020

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.

And it does not work. bool and BOOL have different sizes.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, Unsafe.As<bool, byte>(ref value). The same thing we tried back in dotnet/coreclr#16138 but was killed.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
 string? localeName = _sortHandle != IntPtr.Zero ? null : _sortName;

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
@jkotasjkotasMar 13, 2020

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.

Really? What is the input that it happens for?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Create a new C++ application and drop in this code. It will AV the process.
auto result = CompareStringEx(
LOCALE_NAME_INVARIANT,
NORM_LINGUISTIC_CASING,
L"x",
1,
(LPCWCH)0xdeadbeef,
0,
nullptr,
nullptr,
0);
Edit: I reported it to the Windows team and it's being fixed in a future release. But we pessimistically have to work around it for now.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
 // are normalized to a null-terminated empty string.
@jkotasjkotasMar 13, 2020

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.

Do we have test coverage for all these empty / null string corner cases?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. The unit tests that are part of #1514 (from which this PR was broken out as a more manageable chunk) cover this scenario. In fact, those unit tests are where the problem was originally discovered. :)
Edit: Looks like https://github.com/dotnet/runtime/blob/master/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs also would cover this scenario when the larger "don't early-exit inappropriately" PR comes online.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I'm a little confused as to why CI is failing here. There are some things that look suspicious. From the Helix logs:
/private/tmp/helix/working/A96E0923/w/AB700940/e /private/tmp/helix/working/A96E0923/w/AB700940/e
Discovering: System.Memory.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Memory.Tests (found 2523 of 2547 test cases)
Starting: System.Memory.Tests (parallel test collections = on, max threads = 4)
Process terminated. Assertion failed.
at System.Globalization.CompareInfo.IndexOfOrdinalCore(ReadOnlySpan`1 source, ReadOnlySpan`1 value, Boolean ignoreCase, Boolean fromBeginning) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs:line 48
at System.Globalization.CompareInfo.IndexOfOrdinal(String source, String value, Int32 startIndex, Int32 count, Boolean ignoreCase) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs:line 1115
at System.Tests.StringTests.IndexOf_AllSubstrings(String s, String value, Int32 startIndex, StringComparison comparison) in /_/src/libraries/Common/tests/Tests/System/StringTests.cs:line 2801
Since the failure is at CompareInfo.Unix.cs, line 48, the assertion that's failing is the !GlobalizationMode.Invariant check below.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs#L46-L49
According to the stack trace the caller is CompareInfo.cs, line 1115, but there's no relevant code at that line. It's just another Debug.Assert statement. (Presumably that assertion succeeded, else control flow wouldn't have continued.)
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1113-L1116
There are only three places where we call IndexOfOrdinalCore. All three are guarded with either the same Debug.Assert(!GlobalizationMode.Invariant) (which clearly didn't fail) or with an explicit if check to route control flow appropriately.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1130-L1137
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L969-L984
@jkotas, @tarekgh, any thoughts as to why CI might report such an error? And why the error would occur only on OSX and not any other Unix platform?
@jkotas
jkotas
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I do not see anything obvious. It can be a JIT bug or data corruption - lots of unsafe code in globalization.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
maybe try to add some messages to the asserts in the involved files which may help give some clue.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
The other idea could be trying repro it on OSX machine
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll investigate a little more with the assumption that the stack trace is lying. :)

Edit: Yes, turned out that the stack trace was indeed reporting incorrect line numbers. I've put a new commit to work around the edge cases that were hitting this assert.
@GrabYourPitchforks
GrabYourPitchforks
merged commit faf08f7
into
dotnet:masterMar 14, 2020
@GrabYourPitchforks
GrabYourPitchforks
deleted the

spanify_compareinfo

branch
March 14, 2020 02:43
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 14, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 10, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@tarekghtarekghtarekgh approved these changes

Assignees


No one assigned


Labels

area-System.Globalization

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@jkotas@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Prep CompareInfo for spanification by GrabYourPitchforks · Pull Request #32385 · dotnet/runtime · GitHub
Skip to content
Prep CompareInfo for spanification - #32385
Merged
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Mar 14, 2020
Merged
Prep CompareInfo for spanification#32385
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Feb 16, 2020•

edited

Loading

Copy link


Copy Markdown

Member
This removes many (not all, see #8890) of the string-based p/invoke wrappers in CompareInfo and replaces them with span-based wrappers.
There's a little more error checking around the actual p/invokes themselves, but otherwise the intent of this PR is that there's no behavioral change to the existing surface area, and there's no new API being exposed.
This PR serves as the shared foundation for upcoming work such as #25428, #27912, and #27935.
This PR also reverts a small optimization we did to SortKey in #31779, since I was already touching SortKey-related code paths here.
jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
tarekgh
tarekgh
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
Outdated

@GrabYourPitchforks
GrabYourPitchforks
commented
Feb 21, 2020

Copy link


Copy Markdown

MemberAuthor
Over the next few days I'll get the perf measurements for the change with SetLastError = true. Been heads-down with other work.
I don't expect this to have significant impact on typical API calls to string since we already special-case the common code paths of ordinal comparisons and all-ASCII data. These changes would primarily impact non-ASCII and culture-aware text processing code paths.
Would it be a strange behavioral difference to check last error only in debug builds? If we went this route I imagine we'd want to Debug.Assert rather than throw an exception, since that would give the smallest behavioral difference between debug and release. Do we follow this pattern elsewhere in the framework?
@jkotas
jkotas
commented
Feb 21, 2020

Copy link


Copy Markdown

Member
If we went this route I imagine we'd want to Debug.Assert rather than throw an exception
Yep.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 13, 2020
 Merged

@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Aside from the GetLastError in debug part is there any other feedback for this?
tarekgh
tarekgh
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I added minor comments. LGTM otherwise. Thanks for your effort with it.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I believe all the feedback is now addressed. Waiting for CI to go green.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

 char* lpStringValue,
 int cchValue,
int bIgnoreCase);
bool bIgnoreCase);
@jkotasjkotasMar 13, 2020

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.

This will add marshaling frame. Does it have effect on performance for small strings?
You can use upper-case BOOL to get bool that is both zero-overhead and self-describing.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

You mean apply [MarshalAs(BOOL)] to avoid the marshaling frame?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Oh, we have an actual internal BOOL enum. Got it.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I almost want to define BOOL.FromBoolean(bool value) => Unsafe.As<bool, BOOL>(ref value) to avoid the branching at the call sites. But that's probably an issue for a different PR.
@jkotasjkotasMar 13, 2020

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.

And it does not work. bool and BOOL have different sizes.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, Unsafe.As<bool, byte>(ref value). The same thing we tried back in dotnet/coreclr#16138 but was killed.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
 string? localeName = _sortHandle != IntPtr.Zero ? null : _sortName;

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
@jkotasjkotasMar 13, 2020

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.

Really? What is the input that it happens for?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Create a new C++ application and drop in this code. It will AV the process.
auto result = CompareStringEx(
LOCALE_NAME_INVARIANT,
NORM_LINGUISTIC_CASING,
L"x",
1,
(LPCWCH)0xdeadbeef,
0,
nullptr,
nullptr,
0);
Edit: I reported it to the Windows team and it's being fixed in a future release. But we pessimistically have to work around it for now.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
 // are normalized to a null-terminated empty string.
@jkotasjkotasMar 13, 2020

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.

Do we have test coverage for all these empty / null string corner cases?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. The unit tests that are part of #1514 (from which this PR was broken out as a more manageable chunk) cover this scenario. In fact, those unit tests are where the problem was originally discovered. :)
Edit: Looks like https://github.com/dotnet/runtime/blob/master/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs also would cover this scenario when the larger "don't early-exit inappropriately" PR comes online.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I'm a little confused as to why CI is failing here. There are some things that look suspicious. From the Helix logs:
/private/tmp/helix/working/A96E0923/w/AB700940/e /private/tmp/helix/working/A96E0923/w/AB700940/e
Discovering: System.Memory.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Memory.Tests (found 2523 of 2547 test cases)
Starting: System.Memory.Tests (parallel test collections = on, max threads = 4)
Process terminated. Assertion failed.
at System.Globalization.CompareInfo.IndexOfOrdinalCore(ReadOnlySpan`1 source, ReadOnlySpan`1 value, Boolean ignoreCase, Boolean fromBeginning) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs:line 48
at System.Globalization.CompareInfo.IndexOfOrdinal(String source, String value, Int32 startIndex, Int32 count, Boolean ignoreCase) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs:line 1115
at System.Tests.StringTests.IndexOf_AllSubstrings(String s, String value, Int32 startIndex, StringComparison comparison) in /_/src/libraries/Common/tests/Tests/System/StringTests.cs:line 2801
Since the failure is at CompareInfo.Unix.cs, line 48, the assertion that's failing is the !GlobalizationMode.Invariant check below.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs#L46-L49
According to the stack trace the caller is CompareInfo.cs, line 1115, but there's no relevant code at that line. It's just another Debug.Assert statement. (Presumably that assertion succeeded, else control flow wouldn't have continued.)
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1113-L1116
There are only three places where we call IndexOfOrdinalCore. All three are guarded with either the same Debug.Assert(!GlobalizationMode.Invariant) (which clearly didn't fail) or with an explicit if check to route control flow appropriately.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1130-L1137
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L969-L984
@jkotas, @tarekgh, any thoughts as to why CI might report such an error? And why the error would occur only on OSX and not any other Unix platform?
@jkotas
jkotas
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I do not see anything obvious. It can be a JIT bug or data corruption - lots of unsafe code in globalization.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
maybe try to add some messages to the asserts in the involved files which may help give some clue.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
The other idea could be trying repro it on OSX machine
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll investigate a little more with the assumption that the stack trace is lying. :)

Edit: Yes, turned out that the stack trace was indeed reporting incorrect line numbers. I've put a new commit to work around the edge cases that were hitting this assert.
@GrabYourPitchforks
GrabYourPitchforks
merged commit faf08f7
into
dotnet:masterMar 14, 2020
@GrabYourPitchforks
GrabYourPitchforks
deleted the

spanify_compareinfo

branch
March 14, 2020 02:43
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 14, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 10, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@tarekghtarekghtarekgh approved these changes

Assignees


No one assigned


Labels

area-System.Globalization

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@jkotas@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Prep CompareInfo for spanification by GrabYourPitchforks · Pull Request #32385 · dotnet/runtime · GitHub
Skip to content
Prep CompareInfo for spanification - #32385
Merged
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Mar 14, 2020
Merged
Prep CompareInfo for spanification#32385
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Feb 16, 2020•

edited

Loading

Copy link


Copy Markdown

Member
This removes many (not all, see #8890) of the string-based p/invoke wrappers in CompareInfo and replaces them with span-based wrappers.
There's a little more error checking around the actual p/invokes themselves, but otherwise the intent of this PR is that there's no behavioral change to the existing surface area, and there's no new API being exposed.
This PR serves as the shared foundation for upcoming work such as #25428, #27912, and #27935.
This PR also reverts a small optimization we did to SortKey in #31779, since I was already touching SortKey-related code paths here.
jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
tarekgh
tarekgh
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
Outdated

@GrabYourPitchforks
GrabYourPitchforks
commented
Feb 21, 2020

Copy link


Copy Markdown

MemberAuthor
Over the next few days I'll get the perf measurements for the change with SetLastError = true. Been heads-down with other work.
I don't expect this to have significant impact on typical API calls to string since we already special-case the common code paths of ordinal comparisons and all-ASCII data. These changes would primarily impact non-ASCII and culture-aware text processing code paths.
Would it be a strange behavioral difference to check last error only in debug builds? If we went this route I imagine we'd want to Debug.Assert rather than throw an exception, since that would give the smallest behavioral difference between debug and release. Do we follow this pattern elsewhere in the framework?
@jkotas
jkotas
commented
Feb 21, 2020

Copy link


Copy Markdown

Member
If we went this route I imagine we'd want to Debug.Assert rather than throw an exception
Yep.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 13, 2020
 Merged

@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Aside from the GetLastError in debug part is there any other feedback for this?
tarekgh
tarekgh
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I added minor comments. LGTM otherwise. Thanks for your effort with it.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I believe all the feedback is now addressed. Waiting for CI to go green.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

 char* lpStringValue,
 int cchValue,
int bIgnoreCase);
bool bIgnoreCase);
@jkotasjkotasMar 13, 2020

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.

This will add marshaling frame. Does it have effect on performance for small strings?
You can use upper-case BOOL to get bool that is both zero-overhead and self-describing.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

You mean apply [MarshalAs(BOOL)] to avoid the marshaling frame?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Oh, we have an actual internal BOOL enum. Got it.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I almost want to define BOOL.FromBoolean(bool value) => Unsafe.As<bool, BOOL>(ref value) to avoid the branching at the call sites. But that's probably an issue for a different PR.
@jkotasjkotasMar 13, 2020

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.

And it does not work. bool and BOOL have different sizes.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, Unsafe.As<bool, byte>(ref value). The same thing we tried back in dotnet/coreclr#16138 but was killed.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
 string? localeName = _sortHandle != IntPtr.Zero ? null : _sortName;

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
@jkotasjkotasMar 13, 2020

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.

Really? What is the input that it happens for?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Create a new C++ application and drop in this code. It will AV the process.
auto result = CompareStringEx(
LOCALE_NAME_INVARIANT,
NORM_LINGUISTIC_CASING,
L"x",
1,
(LPCWCH)0xdeadbeef,
0,
nullptr,
nullptr,
0);
Edit: I reported it to the Windows team and it's being fixed in a future release. But we pessimistically have to work around it for now.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
 // are normalized to a null-terminated empty string.
@jkotasjkotasMar 13, 2020

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.

Do we have test coverage for all these empty / null string corner cases?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. The unit tests that are part of #1514 (from which this PR was broken out as a more manageable chunk) cover this scenario. In fact, those unit tests are where the problem was originally discovered. :)
Edit: Looks like https://github.com/dotnet/runtime/blob/master/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs also would cover this scenario when the larger "don't early-exit inappropriately" PR comes online.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I'm a little confused as to why CI is failing here. There are some things that look suspicious. From the Helix logs:
/private/tmp/helix/working/A96E0923/w/AB700940/e /private/tmp/helix/working/A96E0923/w/AB700940/e
Discovering: System.Memory.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Memory.Tests (found 2523 of 2547 test cases)
Starting: System.Memory.Tests (parallel test collections = on, max threads = 4)
Process terminated. Assertion failed.
at System.Globalization.CompareInfo.IndexOfOrdinalCore(ReadOnlySpan`1 source, ReadOnlySpan`1 value, Boolean ignoreCase, Boolean fromBeginning) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs:line 48
at System.Globalization.CompareInfo.IndexOfOrdinal(String source, String value, Int32 startIndex, Int32 count, Boolean ignoreCase) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs:line 1115
at System.Tests.StringTests.IndexOf_AllSubstrings(String s, String value, Int32 startIndex, StringComparison comparison) in /_/src/libraries/Common/tests/Tests/System/StringTests.cs:line 2801
Since the failure is at CompareInfo.Unix.cs, line 48, the assertion that's failing is the !GlobalizationMode.Invariant check below.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs#L46-L49
According to the stack trace the caller is CompareInfo.cs, line 1115, but there's no relevant code at that line. It's just another Debug.Assert statement. (Presumably that assertion succeeded, else control flow wouldn't have continued.)
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1113-L1116
There are only three places where we call IndexOfOrdinalCore. All three are guarded with either the same Debug.Assert(!GlobalizationMode.Invariant) (which clearly didn't fail) or with an explicit if check to route control flow appropriately.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1130-L1137
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L969-L984
@jkotas, @tarekgh, any thoughts as to why CI might report such an error? And why the error would occur only on OSX and not any other Unix platform?
@jkotas
jkotas
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I do not see anything obvious. It can be a JIT bug or data corruption - lots of unsafe code in globalization.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
maybe try to add some messages to the asserts in the involved files which may help give some clue.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
The other idea could be trying repro it on OSX machine
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll investigate a little more with the assumption that the stack trace is lying. :)

Edit: Yes, turned out that the stack trace was indeed reporting incorrect line numbers. I've put a new commit to work around the edge cases that were hitting this assert.
@GrabYourPitchforks
GrabYourPitchforks
merged commit faf08f7
into
dotnet:masterMar 14, 2020
@GrabYourPitchforks
GrabYourPitchforks
deleted the

spanify_compareinfo

branch
March 14, 2020 02:43
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 14, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 10, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@tarekghtarekghtarekgh approved these changes

Assignees


No one assigned


Labels

area-System.Globalization

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@jkotas@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Prep CompareInfo for spanification by GrabYourPitchforks · Pull Request #32385 · dotnet/runtime · GitHub
Skip to content
Prep CompareInfo for spanification - #32385
Merged
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Mar 14, 2020
Merged
Prep CompareInfo for spanification#32385
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Feb 16, 2020•

edited

Loading

Copy link


Copy Markdown

Member
This removes many (not all, see #8890) of the string-based p/invoke wrappers in CompareInfo and replaces them with span-based wrappers.
There's a little more error checking around the actual p/invokes themselves, but otherwise the intent of this PR is that there's no behavioral change to the existing surface area, and there's no new API being exposed.
This PR serves as the shared foundation for upcoming work such as #25428, #27912, and #27935.
This PR also reverts a small optimization we did to SortKey in #31779, since I was already touching SortKey-related code paths here.
jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
tarekgh
tarekgh
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
Outdated

@GrabYourPitchforks
GrabYourPitchforks
commented
Feb 21, 2020

Copy link


Copy Markdown

MemberAuthor
Over the next few days I'll get the perf measurements for the change with SetLastError = true. Been heads-down with other work.
I don't expect this to have significant impact on typical API calls to string since we already special-case the common code paths of ordinal comparisons and all-ASCII data. These changes would primarily impact non-ASCII and culture-aware text processing code paths.
Would it be a strange behavioral difference to check last error only in debug builds? If we went this route I imagine we'd want to Debug.Assert rather than throw an exception, since that would give the smallest behavioral difference between debug and release. Do we follow this pattern elsewhere in the framework?
@jkotas
jkotas
commented
Feb 21, 2020

Copy link


Copy Markdown

Member
If we went this route I imagine we'd want to Debug.Assert rather than throw an exception
Yep.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 13, 2020
 Merged

@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Aside from the GetLastError in debug part is there any other feedback for this?
tarekgh
tarekgh
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I added minor comments. LGTM otherwise. Thanks for your effort with it.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I believe all the feedback is now addressed. Waiting for CI to go green.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

 char* lpStringValue,
 int cchValue,
int bIgnoreCase);
bool bIgnoreCase);
@jkotasjkotasMar 13, 2020

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.

This will add marshaling frame. Does it have effect on performance for small strings?
You can use upper-case BOOL to get bool that is both zero-overhead and self-describing.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

You mean apply [MarshalAs(BOOL)] to avoid the marshaling frame?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Oh, we have an actual internal BOOL enum. Got it.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I almost want to define BOOL.FromBoolean(bool value) => Unsafe.As<bool, BOOL>(ref value) to avoid the branching at the call sites. But that's probably an issue for a different PR.
@jkotasjkotasMar 13, 2020

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.

And it does not work. bool and BOOL have different sizes.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, Unsafe.As<bool, byte>(ref value). The same thing we tried back in dotnet/coreclr#16138 but was killed.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
 string? localeName = _sortHandle != IntPtr.Zero ? null : _sortName;

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
@jkotasjkotasMar 13, 2020

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.

Really? What is the input that it happens for?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Create a new C++ application and drop in this code. It will AV the process.
auto result = CompareStringEx(
LOCALE_NAME_INVARIANT,
NORM_LINGUISTIC_CASING,
L"x",
1,
(LPCWCH)0xdeadbeef,
0,
nullptr,
nullptr,
0);
Edit: I reported it to the Windows team and it's being fixed in a future release. But we pessimistically have to work around it for now.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
 // are normalized to a null-terminated empty string.
@jkotasjkotasMar 13, 2020

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.

Do we have test coverage for all these empty / null string corner cases?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. The unit tests that are part of #1514 (from which this PR was broken out as a more manageable chunk) cover this scenario. In fact, those unit tests are where the problem was originally discovered. :)
Edit: Looks like https://github.com/dotnet/runtime/blob/master/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs also would cover this scenario when the larger "don't early-exit inappropriately" PR comes online.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I'm a little confused as to why CI is failing here. There are some things that look suspicious. From the Helix logs:
/private/tmp/helix/working/A96E0923/w/AB700940/e /private/tmp/helix/working/A96E0923/w/AB700940/e
Discovering: System.Memory.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Memory.Tests (found 2523 of 2547 test cases)
Starting: System.Memory.Tests (parallel test collections = on, max threads = 4)
Process terminated. Assertion failed.
at System.Globalization.CompareInfo.IndexOfOrdinalCore(ReadOnlySpan`1 source, ReadOnlySpan`1 value, Boolean ignoreCase, Boolean fromBeginning) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs:line 48
at System.Globalization.CompareInfo.IndexOfOrdinal(String source, String value, Int32 startIndex, Int32 count, Boolean ignoreCase) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs:line 1115
at System.Tests.StringTests.IndexOf_AllSubstrings(String s, String value, Int32 startIndex, StringComparison comparison) in /_/src/libraries/Common/tests/Tests/System/StringTests.cs:line 2801
Since the failure is at CompareInfo.Unix.cs, line 48, the assertion that's failing is the !GlobalizationMode.Invariant check below.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs#L46-L49
According to the stack trace the caller is CompareInfo.cs, line 1115, but there's no relevant code at that line. It's just another Debug.Assert statement. (Presumably that assertion succeeded, else control flow wouldn't have continued.)
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1113-L1116
There are only three places where we call IndexOfOrdinalCore. All three are guarded with either the same Debug.Assert(!GlobalizationMode.Invariant) (which clearly didn't fail) or with an explicit if check to route control flow appropriately.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1130-L1137
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L969-L984
@jkotas, @tarekgh, any thoughts as to why CI might report such an error? And why the error would occur only on OSX and not any other Unix platform?
@jkotas
jkotas
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I do not see anything obvious. It can be a JIT bug or data corruption - lots of unsafe code in globalization.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
maybe try to add some messages to the asserts in the involved files which may help give some clue.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
The other idea could be trying repro it on OSX machine
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll investigate a little more with the assumption that the stack trace is lying. :)

Edit: Yes, turned out that the stack trace was indeed reporting incorrect line numbers. I've put a new commit to work around the edge cases that were hitting this assert.
@GrabYourPitchforks
GrabYourPitchforks
merged commit faf08f7
into
dotnet:masterMar 14, 2020
@GrabYourPitchforks
GrabYourPitchforks
deleted the

spanify_compareinfo

branch
March 14, 2020 02:43
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 14, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 10, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@tarekghtarekghtarekgh approved these changes

Assignees


No one assigned


Labels

area-System.Globalization

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@jkotas@tarekgh

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Prep CompareInfo for spanification by GrabYourPitchforks · Pull Request #32385 · dotnet/runtime · GitHub
Skip to content
Prep CompareInfo for spanification - #32385
Merged
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Mar 14, 2020
Merged
Prep CompareInfo for spanification#32385
GrabYourPitchforks merged 6 commits into
dotnet:masterfrom 
GrabYourPitchforks:spanify_compareinfo
Conversation
@GrabYourPitchforks
@GrabYourPitchforksGrabYourPitchforks
commented
Feb 16, 2020•

edited

Loading

Copy link


Copy Markdown

Member
This removes many (not all, see #8890) of the string-based p/invoke wrappers in CompareInfo and replaces them with span-based wrappers.
There's a little more error checking around the actual p/invokes themselves, but otherwise the intent of this PR is that there's no behavioral change to the existing surface area, and there's no new API being exposed.
This PR serves as the shared foundation for upcoming work such as #25428, #27912, and #27935.
This PR also reverts a small optimization we did to SortKey in #31779, since I was already touching SortKey-related code paths here.
jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

jkotas
jkotas
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
tarekgh
tarekgh
reviewed
Feb 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
Outdated

@GrabYourPitchforks
GrabYourPitchforks
commented
Feb 21, 2020

Copy link


Copy Markdown

MemberAuthor
Over the next few days I'll get the perf measurements for the change with SetLastError = true. Been heads-down with other work.
I don't expect this to have significant impact on typical API calls to string since we already special-case the common code paths of ordinal comparisons and all-ASCII data. These changes would primarily impact non-ASCII and culture-aware text processing code paths.
Would it be a strange behavioral difference to check last error only in debug builds? If we went this route I imagine we'd want to Debug.Assert rather than throw an exception, since that would give the smallest behavioral difference between debug and release. Do we follow this pattern elsewhere in the framework?
@jkotas
jkotas
commented
Feb 21, 2020

Copy link


Copy Markdown

Member
If we went this route I imagine we'd want to Debug.Assert rather than throw an exception
Yep.
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 13, 2020
 Merged

@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Aside from the GetLastError in debug part is there any other feedback for this?
tarekgh
tarekgh
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I added minor comments. LGTM otherwise. Thanks for your effort with it.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I believe all the feedback is now addressed. Waiting for CI to go green.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/Common/src/Interop/Windows/Kernel32/Interop.Globalization.cs
Outdated

 char* lpStringValue,
 int cchValue,
int bIgnoreCase);
bool bIgnoreCase);
@jkotasjkotasMar 13, 2020

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.

This will add marshaling frame. Does it have effect on performance for small strings?
You can use upper-case BOOL to get bool that is both zero-overhead and self-describing.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

You mean apply [MarshalAs(BOOL)] to avoid the marshaling frame?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Oh, we have an actual internal BOOL enum. Got it.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I almost want to define BOOL.FromBoolean(bool value) => Unsafe.As<bool, BOOL>(ref value) to avoid the branching at the call sites. But that's probably an issue for a different PR.
@jkotasjkotasMar 13, 2020

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.

And it does not work. bool and BOOL have different sizes.
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sorry, Unsafe.As<bool, byte>(ref value). The same thing we tried back in dotnet/coreclr#16138 but was killed.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs
 string? localeName = _sortHandle != IntPtr.Zero ? null : _sortName;

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
@jkotasjkotasMar 13, 2020

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.

Really? What is the input that it happens for?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Create a new C++ application and drop in this code. It will AV the process.
auto result = CompareStringEx(
LOCALE_NAME_INVARIANT,
NORM_LINGUISTIC_CASING,
L"x",
1,
(LPCWCH)0xdeadbeef,
0,
nullptr,
nullptr,
0);
Edit: I reported it to the Windows team and it's being fixed in a future release. But we pessimistically have to work around it for now.
jkotas
jkotas
reviewed
Mar 13, 2020
View reviewed changes
Comment threadsrc/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs

 // CompareStringEx may try to dereference the first character of its input, even if an explicit
 // length of 0 is specified. To work around potential AVs we'll always ensure zero-length inputs
 // are normalized to a null-terminated empty string.
@jkotasjkotasMar 13, 2020

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.

Do we have test coverage for all these empty / null string corner cases?
@GrabYourPitchforksGrabYourPitchforksMar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Yes. The unit tests that are part of #1514 (from which this PR was broken out as a more manageable chunk) cover this scenario. In fact, those unit tests are where the problem was originally discovered. :)
Edit: Looks like https://github.com/dotnet/runtime/blob/master/src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.Compare.cs also would cover this scenario when the larger "don't early-exit inappropriately" PR comes online.
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020

Copy link


Copy Markdown

MemberAuthor
I'm a little confused as to why CI is failing here. There are some things that look suspicious. From the Helix logs:
/private/tmp/helix/working/A96E0923/w/AB700940/e /private/tmp/helix/working/A96E0923/w/AB700940/e
Discovering: System.Memory.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Memory.Tests (found 2523 of 2547 test cases)
Starting: System.Memory.Tests (parallel test collections = on, max threads = 4)
Process terminated. Assertion failed.
at System.Globalization.CompareInfo.IndexOfOrdinalCore(ReadOnlySpan`1 source, ReadOnlySpan`1 value, Boolean ignoreCase, Boolean fromBeginning) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs:line 48
at System.Globalization.CompareInfo.IndexOfOrdinal(String source, String value, Int32 startIndex, Int32 count, Boolean ignoreCase) in /_/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs:line 1115
at System.Tests.StringTests.IndexOf_AllSubstrings(String s, String value, Int32 startIndex, StringComparison comparison) in /_/src/libraries/Common/tests/Tests/System/StringTests.cs:line 2801
Since the failure is at CompareInfo.Unix.cs, line 48, the assertion that's failing is the !GlobalizationMode.Invariant check below.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs#L46-L49
According to the stack trace the caller is CompareInfo.cs, line 1115, but there's no relevant code at that line. It's just another Debug.Assert statement. (Presumably that assertion succeeded, else control flow wouldn't have continued.)
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1113-L1116
There are only three places where we call IndexOfOrdinalCore. All three are guarded with either the same Debug.Assert(!GlobalizationMode.Invariant) (which clearly didn't fail) or with an explicit if check to route control flow appropriately.
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L1130-L1137
https://github.com/dotnet/runtime/blob/da2eb1dbdcfab8ebfc37db726f23c2c0936d12c9/src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs#L969-L984
@jkotas, @tarekgh, any thoughts as to why CI might report such an error? And why the error would occur only on OSX and not any other Unix platform?
@jkotas
jkotas
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
I do not see anything obvious. It can be a JIT bug or data corruption - lots of unsafe code in globalization.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
maybe try to add some messages to the asserts in the involved files which may help give some clue.
@tarekgh
tarekgh
commented
Mar 13, 2020

Copy link


Copy Markdown

Member
The other idea could be trying repro it on OSX machine
@GrabYourPitchforks
GrabYourPitchforks
commented
Mar 13, 2020•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
I'll investigate a little more with the assumption that the stack trace is lying. :)

Edit: Yes, turned out that the stack trace was indeed reporting incorrect line numbers. I've put a new commit to work around the edge cases that were hitting this assert.
@GrabYourPitchforks
GrabYourPitchforks
merged commit faf08f7
into
dotnet:masterMar 14, 2020
@GrabYourPitchforks
GrabYourPitchforks
deleted the

spanify_compareinfo

branch
March 14, 2020 02:43
@GrabYourPitchforksGrabYourPitchforks
mentioned this pull request
Mar 14, 2020

Closed

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 10, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes
@tarekghtarekghtarekgh approved these changes

Assignees


No one assigned


Labels

area-System.Globalization

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

3 participants

@GrabYourPitchforks@jkotas@tarekgh