Fix bool.TryParse/Format on big-endian systems - #65078

Merged
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt
Feb 10, 2022
Merged

Fix bool.TryParse/Format on big-endian systems#65078
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt

Conversation

@uweigand

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label Feb 9, 2022

@EgorBoEgorBoFeb 9, 2022

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.

I don't understand, wasn't .WriteUInt64LittleEndian supposed to handle endianess via ReverseEndianness() for value?

/// <summary>/// Write a UInt64 into a span of bytes as little endian./// </summary>[CLSCompliant(false)][MethodImpl(MethodImplOptions.AggressiveInlining)]publicstaticvoidWriteUInt64LittleEndian(Span<byte>destination,ulongvalue){if(!BitConverter.IsLittleEndian){value=ReverseEndianness(value);}MemoryMarshal.Write(destination,refvalue);}

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.

if it wasn't then you can re-write code to just MemoryMarshal.Write(destination, 0x73006C00610046); I guess

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The problem is that you need to distinguish between swapping the order of the characters and the order of the bytes within one two-byte character. See #64782 (comment) for an example. There's no single store that does the correct thing for both of these simultaneously on both big- and little-endian platforms.

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.

I see, thanks for explanation! anyway maybe it's still better to use

MemoryMarshal.Write(destination, IsLittleEndian ? 0x73006C00610046 : ...)

?
less verbose IMO

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That would work as well. I can try this out if you prefer.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Patch updated accordingly.

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

CC @stephentoub

Author:uweigand
Assignees:-
Labels:

area-System.Runtime, community-contribution

Milestone:-

@jkotas
jkotas merged commit b2eba58 into dotnet:mainFeb 10, 2022

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing.

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

@uweigand
uweigand deleted the fix-boolfmt branch February 11, 2022 08:29
@uweigand

Copy link
Copy Markdown
ContributorAuthor

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

Do you mean these tests (part of libs.tests):

 System.Text.RegularExpressions.Generators.Tests Total: 164, Errors: 0, Failed: 0, Skipped: 0, Time: 6.729s
System.Text.RegularExpressions.Tests Total: 14829, Errors: 0, Failed: 0, Skipped: 0, Time: 59.637s

Those seem to be all good now ...

@stephentoub

Copy link
Copy Markdown
Member

Those seem to be all good now ...

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

Can you try compiling this program using a .NET 7 SDK on a little-endian machine:

usingSystem.Text.RegularExpressions;partialclassProgram{publicstaticvoidMain()=>Console.WriteLine(Example().IsMatch(@"1abcd"));[RegexGenerator(@"\dabcd")]publicstaticpartialRegexExample();}

and then running the resulting program on the big-endian one? It should print true, but I'm betting it's going to print false.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

I see.

It should print true, but I'm betting it's going to print false.

Yes, this is indeed what happens.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

@stephentoub

Copy link
Copy Markdown
Member

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

I repeated the test above using today's snapshot (dotnet-sdk-7.0.100-preview.2.22118.2-linux-x64.tar.gz) to build the regex test case on Intel, and can confirm that the resulting assembly now outputs "True" when run on s390x.

Thanks!

@stephentoub

Copy link
Copy Markdown
Member

Thanks for confirming, @uweigand.

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

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoteExecutor: String 'True' was not recognized as a valid Boolean.

5 participants

@uweigand@stephentoub@EgorBo@jkotas@marek-safar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix bool.TryParse/Format on big-endian systems - #65078

Merged
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt
Feb 10, 2022
Merged

Fix bool.TryParse/Format on big-endian systems#65078
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt

Conversation

@uweigand

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label Feb 9, 2022

@EgorBoEgorBoFeb 9, 2022

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.

I don't understand, wasn't .WriteUInt64LittleEndian supposed to handle endianess via ReverseEndianness() for value?

/// <summary>/// Write a UInt64 into a span of bytes as little endian./// </summary>[CLSCompliant(false)][MethodImpl(MethodImplOptions.AggressiveInlining)]publicstaticvoidWriteUInt64LittleEndian(Span<byte>destination,ulongvalue){if(!BitConverter.IsLittleEndian){value=ReverseEndianness(value);}MemoryMarshal.Write(destination,refvalue);}

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.

if it wasn't then you can re-write code to just MemoryMarshal.Write(destination, 0x73006C00610046); I guess

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The problem is that you need to distinguish between swapping the order of the characters and the order of the bytes within one two-byte character. See #64782 (comment) for an example. There's no single store that does the correct thing for both of these simultaneously on both big- and little-endian platforms.

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.

I see, thanks for explanation! anyway maybe it's still better to use

MemoryMarshal.Write(destination, IsLittleEndian ? 0x73006C00610046 : ...)

?
less verbose IMO

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That would work as well. I can try this out if you prefer.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Patch updated accordingly.

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

CC @stephentoub

Author:uweigand
Assignees:-
Labels:

area-System.Runtime, community-contribution

Milestone:-

@jkotas
jkotas merged commit b2eba58 into dotnet:mainFeb 10, 2022

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing.

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

@uweigand
uweigand deleted the fix-boolfmt branch February 11, 2022 08:29
@uweigand

Copy link
Copy Markdown
ContributorAuthor

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

Do you mean these tests (part of libs.tests):

 System.Text.RegularExpressions.Generators.Tests Total: 164, Errors: 0, Failed: 0, Skipped: 0, Time: 6.729s
System.Text.RegularExpressions.Tests Total: 14829, Errors: 0, Failed: 0, Skipped: 0, Time: 59.637s

Those seem to be all good now ...

@stephentoub

Copy link
Copy Markdown
Member

Those seem to be all good now ...

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

Can you try compiling this program using a .NET 7 SDK on a little-endian machine:

usingSystem.Text.RegularExpressions;partialclassProgram{publicstaticvoidMain()=>Console.WriteLine(Example().IsMatch(@"1abcd"));[RegexGenerator(@"\dabcd")]publicstaticpartialRegexExample();}

and then running the resulting program on the big-endian one? It should print true, but I'm betting it's going to print false.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

I see.

It should print true, but I'm betting it's going to print false.

Yes, this is indeed what happens.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

@stephentoub

Copy link
Copy Markdown
Member

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

I repeated the test above using today's snapshot (dotnet-sdk-7.0.100-preview.2.22118.2-linux-x64.tar.gz) to build the regex test case on Intel, and can confirm that the resulting assembly now outputs "True" when run on s390x.

Thanks!

@stephentoub

Copy link
Copy Markdown
Member

Thanks for confirming, @uweigand.

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

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoteExecutor: String 'True' was not recognized as a valid Boolean.

5 participants

@uweigand@stephentoub@EgorBo@jkotas@marek-safar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix bool.TryParse/Format on big-endian systems - #65078

Merged
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt
Feb 10, 2022
Merged

Fix bool.TryParse/Format on big-endian systems#65078
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt

Conversation

@uweigand

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label Feb 9, 2022

@EgorBoEgorBoFeb 9, 2022

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.

I don't understand, wasn't .WriteUInt64LittleEndian supposed to handle endianess via ReverseEndianness() for value?

/// <summary>/// Write a UInt64 into a span of bytes as little endian./// </summary>[CLSCompliant(false)][MethodImpl(MethodImplOptions.AggressiveInlining)]publicstaticvoidWriteUInt64LittleEndian(Span<byte>destination,ulongvalue){if(!BitConverter.IsLittleEndian){value=ReverseEndianness(value);}MemoryMarshal.Write(destination,refvalue);}

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.

if it wasn't then you can re-write code to just MemoryMarshal.Write(destination, 0x73006C00610046); I guess

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The problem is that you need to distinguish between swapping the order of the characters and the order of the bytes within one two-byte character. See #64782 (comment) for an example. There's no single store that does the correct thing for both of these simultaneously on both big- and little-endian platforms.

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.

I see, thanks for explanation! anyway maybe it's still better to use

MemoryMarshal.Write(destination, IsLittleEndian ? 0x73006C00610046 : ...)

?
less verbose IMO

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That would work as well. I can try this out if you prefer.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Patch updated accordingly.

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

CC @stephentoub

Author:uweigand
Assignees:-
Labels:

area-System.Runtime, community-contribution

Milestone:-

@jkotas
jkotas merged commit b2eba58 into dotnet:mainFeb 10, 2022

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing.

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

@uweigand
uweigand deleted the fix-boolfmt branch February 11, 2022 08:29
@uweigand

Copy link
Copy Markdown
ContributorAuthor

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

Do you mean these tests (part of libs.tests):

 System.Text.RegularExpressions.Generators.Tests Total: 164, Errors: 0, Failed: 0, Skipped: 0, Time: 6.729s
System.Text.RegularExpressions.Tests Total: 14829, Errors: 0, Failed: 0, Skipped: 0, Time: 59.637s

Those seem to be all good now ...

@stephentoub

Copy link
Copy Markdown
Member

Those seem to be all good now ...

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

Can you try compiling this program using a .NET 7 SDK on a little-endian machine:

usingSystem.Text.RegularExpressions;partialclassProgram{publicstaticvoidMain()=>Console.WriteLine(Example().IsMatch(@"1abcd"));[RegexGenerator(@"\dabcd")]publicstaticpartialRegexExample();}

and then running the resulting program on the big-endian one? It should print true, but I'm betting it's going to print false.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

I see.

It should print true, but I'm betting it's going to print false.

Yes, this is indeed what happens.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

@stephentoub

Copy link
Copy Markdown
Member

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

I repeated the test above using today's snapshot (dotnet-sdk-7.0.100-preview.2.22118.2-linux-x64.tar.gz) to build the regex test case on Intel, and can confirm that the resulting assembly now outputs "True" when run on s390x.

Thanks!

@stephentoub

Copy link
Copy Markdown
Member

Thanks for confirming, @uweigand.

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

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoteExecutor: String 'True' was not recognized as a valid Boolean.

5 participants

@uweigand@stephentoub@EgorBo@jkotas@marek-safar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix bool.TryParse/Format on big-endian systems - #65078

Merged
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt
Feb 10, 2022
Merged

Fix bool.TryParse/Format on big-endian systems#65078
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt

Conversation

@uweigand

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label Feb 9, 2022

@EgorBoEgorBoFeb 9, 2022

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.

I don't understand, wasn't .WriteUInt64LittleEndian supposed to handle endianess via ReverseEndianness() for value?

/// <summary>/// Write a UInt64 into a span of bytes as little endian./// </summary>[CLSCompliant(false)][MethodImpl(MethodImplOptions.AggressiveInlining)]publicstaticvoidWriteUInt64LittleEndian(Span<byte>destination,ulongvalue){if(!BitConverter.IsLittleEndian){value=ReverseEndianness(value);}MemoryMarshal.Write(destination,refvalue);}

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.

if it wasn't then you can re-write code to just MemoryMarshal.Write(destination, 0x73006C00610046); I guess

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The problem is that you need to distinguish between swapping the order of the characters and the order of the bytes within one two-byte character. See #64782 (comment) for an example. There's no single store that does the correct thing for both of these simultaneously on both big- and little-endian platforms.

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.

I see, thanks for explanation! anyway maybe it's still better to use

MemoryMarshal.Write(destination, IsLittleEndian ? 0x73006C00610046 : ...)

?
less verbose IMO

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That would work as well. I can try this out if you prefer.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Patch updated accordingly.

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

CC @stephentoub

Author:uweigand
Assignees:-
Labels:

area-System.Runtime, community-contribution

Milestone:-

@jkotas
jkotas merged commit b2eba58 into dotnet:mainFeb 10, 2022

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing.

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

@uweigand
uweigand deleted the fix-boolfmt branch February 11, 2022 08:29
@uweigand

Copy link
Copy Markdown
ContributorAuthor

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

Do you mean these tests (part of libs.tests):

 System.Text.RegularExpressions.Generators.Tests Total: 164, Errors: 0, Failed: 0, Skipped: 0, Time: 6.729s
System.Text.RegularExpressions.Tests Total: 14829, Errors: 0, Failed: 0, Skipped: 0, Time: 59.637s

Those seem to be all good now ...

@stephentoub

Copy link
Copy Markdown
Member

Those seem to be all good now ...

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

Can you try compiling this program using a .NET 7 SDK on a little-endian machine:

usingSystem.Text.RegularExpressions;partialclassProgram{publicstaticvoidMain()=>Console.WriteLine(Example().IsMatch(@"1abcd"));[RegexGenerator(@"\dabcd")]publicstaticpartialRegexExample();}

and then running the resulting program on the big-endian one? It should print true, but I'm betting it's going to print false.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

I see.

It should print true, but I'm betting it's going to print false.

Yes, this is indeed what happens.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

@stephentoub

Copy link
Copy Markdown
Member

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

I repeated the test above using today's snapshot (dotnet-sdk-7.0.100-preview.2.22118.2-linux-x64.tar.gz) to build the regex test case on Intel, and can confirm that the resulting assembly now outputs "True" when run on s390x.

Thanks!

@stephentoub

Copy link
Copy Markdown
Member

Thanks for confirming, @uweigand.

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

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoteExecutor: String 'True' was not recognized as a valid Boolean.

5 participants

@uweigand@stephentoub@EgorBo@jkotas@marek-safar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix bool.TryParse/Format on big-endian systems - #65078

Merged
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt
Feb 10, 2022
Merged

Fix bool.TryParse/Format on big-endian systems#65078
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt

Conversation

@uweigand

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label Feb 9, 2022

@EgorBoEgorBoFeb 9, 2022

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.

I don't understand, wasn't .WriteUInt64LittleEndian supposed to handle endianess via ReverseEndianness() for value?

/// <summary>/// Write a UInt64 into a span of bytes as little endian./// </summary>[CLSCompliant(false)][MethodImpl(MethodImplOptions.AggressiveInlining)]publicstaticvoidWriteUInt64LittleEndian(Span<byte>destination,ulongvalue){if(!BitConverter.IsLittleEndian){value=ReverseEndianness(value);}MemoryMarshal.Write(destination,refvalue);}

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.

if it wasn't then you can re-write code to just MemoryMarshal.Write(destination, 0x73006C00610046); I guess

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The problem is that you need to distinguish between swapping the order of the characters and the order of the bytes within one two-byte character. See #64782 (comment) for an example. There's no single store that does the correct thing for both of these simultaneously on both big- and little-endian platforms.

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.

I see, thanks for explanation! anyway maybe it's still better to use

MemoryMarshal.Write(destination, IsLittleEndian ? 0x73006C00610046 : ...)

?
less verbose IMO

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That would work as well. I can try this out if you prefer.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Patch updated accordingly.

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

CC @stephentoub

Author:uweigand
Assignees:-
Labels:

area-System.Runtime, community-contribution

Milestone:-

@jkotas
jkotas merged commit b2eba58 into dotnet:mainFeb 10, 2022

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing.

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

@uweigand
uweigand deleted the fix-boolfmt branch February 11, 2022 08:29
@uweigand

Copy link
Copy Markdown
ContributorAuthor

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

Do you mean these tests (part of libs.tests):

 System.Text.RegularExpressions.Generators.Tests Total: 164, Errors: 0, Failed: 0, Skipped: 0, Time: 6.729s
System.Text.RegularExpressions.Tests Total: 14829, Errors: 0, Failed: 0, Skipped: 0, Time: 59.637s

Those seem to be all good now ...

@stephentoub

Copy link
Copy Markdown
Member

Those seem to be all good now ...

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

Can you try compiling this program using a .NET 7 SDK on a little-endian machine:

usingSystem.Text.RegularExpressions;partialclassProgram{publicstaticvoidMain()=>Console.WriteLine(Example().IsMatch(@"1abcd"));[RegexGenerator(@"\dabcd")]publicstaticpartialRegexExample();}

and then running the resulting program on the big-endian one? It should print true, but I'm betting it's going to print false.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

I see.

It should print true, but I'm betting it's going to print false.

Yes, this is indeed what happens.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

@stephentoub

Copy link
Copy Markdown
Member

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

I repeated the test above using today's snapshot (dotnet-sdk-7.0.100-preview.2.22118.2-linux-x64.tar.gz) to build the regex test case on Intel, and can confirm that the resulting assembly now outputs "True" when run on s390x.

Thanks!

@stephentoub

Copy link
Copy Markdown
Member

Thanks for confirming, @uweigand.

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

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoteExecutor: String 'True' was not recognized as a valid Boolean.

5 participants

@uweigand@stephentoub@EgorBo@jkotas@marek-safar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix bool.TryParse/Format on big-endian systems - #65078

Merged
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt
Feb 10, 2022
Merged

Fix bool.TryParse/Format on big-endian systems#65078
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt

Conversation

@uweigand

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label Feb 9, 2022

@EgorBoEgorBoFeb 9, 2022

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.

I don't understand, wasn't .WriteUInt64LittleEndian supposed to handle endianess via ReverseEndianness() for value?

/// <summary>/// Write a UInt64 into a span of bytes as little endian./// </summary>[CLSCompliant(false)][MethodImpl(MethodImplOptions.AggressiveInlining)]publicstaticvoidWriteUInt64LittleEndian(Span<byte>destination,ulongvalue){if(!BitConverter.IsLittleEndian){value=ReverseEndianness(value);}MemoryMarshal.Write(destination,refvalue);}

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.

if it wasn't then you can re-write code to just MemoryMarshal.Write(destination, 0x73006C00610046); I guess

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The problem is that you need to distinguish between swapping the order of the characters and the order of the bytes within one two-byte character. See #64782 (comment) for an example. There's no single store that does the correct thing for both of these simultaneously on both big- and little-endian platforms.

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.

I see, thanks for explanation! anyway maybe it's still better to use

MemoryMarshal.Write(destination, IsLittleEndian ? 0x73006C00610046 : ...)

?
less verbose IMO

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That would work as well. I can try this out if you prefer.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Patch updated accordingly.

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

CC @stephentoub

Author:uweigand
Assignees:-
Labels:

area-System.Runtime, community-contribution

Milestone:-

@jkotas
jkotas merged commit b2eba58 into dotnet:mainFeb 10, 2022

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing.

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

@uweigand
uweigand deleted the fix-boolfmt branch February 11, 2022 08:29
@uweigand

Copy link
Copy Markdown
ContributorAuthor

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

Do you mean these tests (part of libs.tests):

 System.Text.RegularExpressions.Generators.Tests Total: 164, Errors: 0, Failed: 0, Skipped: 0, Time: 6.729s
System.Text.RegularExpressions.Tests Total: 14829, Errors: 0, Failed: 0, Skipped: 0, Time: 59.637s

Those seem to be all good now ...

@stephentoub

Copy link
Copy Markdown
Member

Those seem to be all good now ...

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

Can you try compiling this program using a .NET 7 SDK on a little-endian machine:

usingSystem.Text.RegularExpressions;partialclassProgram{publicstaticvoidMain()=>Console.WriteLine(Example().IsMatch(@"1abcd"));[RegexGenerator(@"\dabcd")]publicstaticpartialRegexExample();}

and then running the resulting program on the big-endian one? It should print true, but I'm betting it's going to print false.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

I see.

It should print true, but I'm betting it's going to print false.

Yes, this is indeed what happens.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

@stephentoub

Copy link
Copy Markdown
Member

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

I repeated the test above using today's snapshot (dotnet-sdk-7.0.100-preview.2.22118.2-linux-x64.tar.gz) to build the regex test case on Intel, and can confirm that the resulting assembly now outputs "True" when run on s390x.

Thanks!

@stephentoub

Copy link
Copy Markdown
Member

Thanks for confirming, @uweigand.

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

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoteExecutor: String 'True' was not recognized as a valid Boolean.

5 participants

@uweigand@stephentoub@EgorBo@jkotas@marek-safar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix bool.TryParse/Format on big-endian systems - #65078

Merged
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt
Feb 10, 2022
Merged

Fix bool.TryParse/Format on big-endian systems#65078
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt

Conversation

@uweigand

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label Feb 9, 2022

@EgorBoEgorBoFeb 9, 2022

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.

I don't understand, wasn't .WriteUInt64LittleEndian supposed to handle endianess via ReverseEndianness() for value?

/// <summary>/// Write a UInt64 into a span of bytes as little endian./// </summary>[CLSCompliant(false)][MethodImpl(MethodImplOptions.AggressiveInlining)]publicstaticvoidWriteUInt64LittleEndian(Span<byte>destination,ulongvalue){if(!BitConverter.IsLittleEndian){value=ReverseEndianness(value);}MemoryMarshal.Write(destination,refvalue);}

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.

if it wasn't then you can re-write code to just MemoryMarshal.Write(destination, 0x73006C00610046); I guess

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The problem is that you need to distinguish between swapping the order of the characters and the order of the bytes within one two-byte character. See #64782 (comment) for an example. There's no single store that does the correct thing for both of these simultaneously on both big- and little-endian platforms.

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.

I see, thanks for explanation! anyway maybe it's still better to use

MemoryMarshal.Write(destination, IsLittleEndian ? 0x73006C00610046 : ...)

?
less verbose IMO

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That would work as well. I can try this out if you prefer.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Patch updated accordingly.

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

CC @stephentoub

Author:uweigand
Assignees:-
Labels:

area-System.Runtime, community-contribution

Milestone:-

@jkotas
jkotas merged commit b2eba58 into dotnet:mainFeb 10, 2022

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing.

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

@uweigand
uweigand deleted the fix-boolfmt branch February 11, 2022 08:29
@uweigand

Copy link
Copy Markdown
ContributorAuthor

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

Do you mean these tests (part of libs.tests):

 System.Text.RegularExpressions.Generators.Tests Total: 164, Errors: 0, Failed: 0, Skipped: 0, Time: 6.729s
System.Text.RegularExpressions.Tests Total: 14829, Errors: 0, Failed: 0, Skipped: 0, Time: 59.637s

Those seem to be all good now ...

@stephentoub

Copy link
Copy Markdown
Member

Those seem to be all good now ...

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

Can you try compiling this program using a .NET 7 SDK on a little-endian machine:

usingSystem.Text.RegularExpressions;partialclassProgram{publicstaticvoidMain()=>Console.WriteLine(Example().IsMatch(@"1abcd"));[RegexGenerator(@"\dabcd")]publicstaticpartialRegexExample();}

and then running the resulting program on the big-endian one? It should print true, but I'm betting it's going to print false.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

I see.

It should print true, but I'm betting it's going to print false.

Yes, this is indeed what happens.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

@stephentoub

Copy link
Copy Markdown
Member

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

I repeated the test above using today's snapshot (dotnet-sdk-7.0.100-preview.2.22118.2-linux-x64.tar.gz) to build the regex test case on Intel, and can confirm that the resulting assembly now outputs "True" when run on s390x.

Thanks!

@stephentoub

Copy link
Copy Markdown
Member

Thanks for confirming, @uweigand.

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

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoteExecutor: String 'True' was not recognized as a valid Boolean.

5 participants

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

Fix bool.TryParse/Format on big-endian systems - #65078

Merged
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt
Feb 10, 2022
Merged

Fix bool.TryParse/Format on big-endian systems#65078
jkotas merged 1 commit into
dotnet:mainfrom
uweigand:fix-boolfmt

Conversation

@uweigand

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label Feb 9, 2022

@EgorBoEgorBoFeb 9, 2022

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.

I don't understand, wasn't .WriteUInt64LittleEndian supposed to handle endianess via ReverseEndianness() for value?

/// <summary>/// Write a UInt64 into a span of bytes as little endian./// </summary>[CLSCompliant(false)][MethodImpl(MethodImplOptions.AggressiveInlining)]publicstaticvoidWriteUInt64LittleEndian(Span<byte>destination,ulongvalue){if(!BitConverter.IsLittleEndian){value=ReverseEndianness(value);}MemoryMarshal.Write(destination,refvalue);}

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.

if it wasn't then you can re-write code to just MemoryMarshal.Write(destination, 0x73006C00610046); I guess

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The problem is that you need to distinguish between swapping the order of the characters and the order of the bytes within one two-byte character. See #64782 (comment) for an example. There's no single store that does the correct thing for both of these simultaneously on both big- and little-endian platforms.

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.

I see, thanks for explanation! anyway maybe it's still better to use

MemoryMarshal.Write(destination, IsLittleEndian ? 0x73006C00610046 : ...)

?
less verbose IMO

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That would work as well. I can try this out if you prefer.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Patch updated accordingly.

@ghost

ghost commented Feb 9, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

CC @stephentoub

Author:uweigand
Assignees:-
Labels:

area-System.Runtime, community-contribution

Milestone:-

@jkotas
jkotas merged commit b2eba58 into dotnet:mainFeb 10, 2022

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for fixing.

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

@uweigand
uweigand deleted the fix-boolfmt branch February 11, 2022 08:29
@uweigand

Copy link
Copy Markdown
ContributorAuthor

Are we running the regex tests on the s390x? I would expect it's going to have similar problems.

Do you mean these tests (part of libs.tests):

 System.Text.RegularExpressions.Generators.Tests Total: 164, Errors: 0, Failed: 0, Skipped: 0, Time: 6.729s
System.Text.RegularExpressions.Tests Total: 14829, Errors: 0, Failed: 0, Skipped: 0, Time: 59.637s

Those seem to be all good now ...

@stephentoub

Copy link
Copy Markdown
Member

Those seem to be all good now ...

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

Can you try compiling this program using a .NET 7 SDK on a little-endian machine:

usingSystem.Text.RegularExpressions;partialclassProgram{publicstaticvoidMain()=>Console.WriteLine(Example().IsMatch(@"1abcd"));[RegexGenerator(@"\dabcd")]publicstaticpartialRegexExample();}

and then running the resulting program on the big-endian one? It should print true, but I'm betting it's going to print false.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

Thanks for looking. I think there's actually still an issue there, but it's masked by limitations in our test infrastructure. .NET 7 includes a source generator for regex, where the generator will emit into the .dll the code for the regex implementation. I expect if you were to take a .dll compiled on a little-endian machine and run it on the s390x, or vice versa, you'd see operations fail.

I see.

It should print true, but I'm betting it's going to print false.

Yes, this is indeed what happens.

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

@stephentoub

Copy link
Copy Markdown
Member

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

@uweigand

Copy link
Copy Markdown
ContributorAuthor

I'll prepare a fix for it, but I'm going to need your help in validating its correctness.

Sure, happy to help! Thanks for thinking of this.

I ended up just deleting the offending code instead.

I repeated the test above using today's snapshot (dotnet-sdk-7.0.100-preview.2.22118.2-linux-x64.tar.gz) to build the regex test case on Intel, and can confirm that the resulting assembly now outputs "True" when run on s390x.

Thanks!

@stephentoub

Copy link
Copy Markdown
Member

Thanks for confirming, @uweigand.

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

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RemoteExecutor: String 'True' was not recognized as a valid Boolean.

5 participants

@uweigand@stephentoub@EgorBo@jkotas@marek-safar