Fix bool.TryParse/Format on big-endian systems#65078
Fix bool.TryParse/Format on big-endian systems#65078jkotas merged 1 commit intodotnet:mainfrom uweigand:fix-boolfmt
Conversation
|
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. |
There was a problem hiding this comment.
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)]
public static void WriteUInt64LittleEndian(Span<byte> destination, ulong value)
{
if (!BitConverter.IsLittleEndian)
{
value = ReverseEndianness(value);
}
MemoryMarshal.Write(destination, ref value);
}There was a problem hiding this comment.
if it wasn't then you can re-write code to just MemoryMarshal.Write(destination, 0x73006C00610046); I guess
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see, thanks for explanation! anyway maybe it's still better to use
MemoryMarshal.Write(destination, IsLittleEndian ? 0x73006C00610046 : ...)
?
less verbose IMO
There was a problem hiding this comment.
That would work as well. I can try this out if you prefer.
There was a problem hiding this comment.
Patch updated accordingly.
|
Tagging subscribers to this area: @dotnet/area-system-runtime Issue Details
CC @stephentoub
|
* Fix regression after #64782
stephentoub
left a comment
There was a problem hiding this comment.
Thanks for fixing.
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 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: using System.Text.RegularExpressions;
partial class Program
{
public static void Main() => Console.WriteLine(Example().IsMatch(@"1abcd"));
[RegexGenerator(@"\dabcd")]
public static partial Regex Example();
}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. |
I see.
Yes, this is indeed what happens.
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! |
|
Thanks for confirming, @uweigand. |
CC @stephentoub