Skip to content

Do not ignore MemoryMarshal.TryWrite result in Guid #104728

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Do not ignore MemoryMarshal.TryWrite result in Guid #104728

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
01675ac
Do not ignore `MemoryMarshal.TryWrite` result in `Guid`
xtqqczze Jul 11, 2024
811533c
Revert "Do not ignore `MemoryMarshal.TryWrite` result in `Guid`"
xtqqczze Jul 23, 2024
c29f826
Minimal fix
xtqqczze Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions src/libraries/System.Private.CoreLib/src/System/Guid.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,75 +909,75 @@ private static bool IsHexPrefix(ReadOnlySpan<char> str, int i) =>
// Returns an unsigned byte array containing the GUID.
public byte[] ToByteArray()
{
var g = new byte[16];
var bytes = new byte[16];
if (BitConverter.IsLittleEndian)
{
MemoryMarshal.TryWrite(g, in this);
MemoryMarshal.Write(bytes, in this);
}
else
{
// slower path for BigEndian
Guid guid = new Guid(MemoryMarshal.AsBytes(new ReadOnlySpan<Guid>(in this)), false);
MemoryMarshal.TryWrite(g, in guid);
MemoryMarshal.Write(bytes, in guid);
}
return g;
return bytes;
}


// Returns an unsigned byte array containing the GUID.
public byte[] ToByteArray(bool bigEndian)
{
var g = new byte[16];
var bytes = new byte[16];
if (BitConverter.IsLittleEndian != bigEndian)
{
MemoryMarshal.TryWrite(g, in this);
MemoryMarshal.Write(bytes, in this);
}
else
{
// slower path for Reverse
Guid guid = new Guid(MemoryMarshal.AsBytes(new ReadOnlySpan<Guid>(in this)), bigEndian);
MemoryMarshal.TryWrite(g, in guid);
MemoryMarshal.Write(bytes, in guid);
}
return g;
return bytes;
}

// Returns whether bytes are successfully written to given span.
public bool TryWriteBytes(Span<byte> destination)
{
if (destination.Length < 16)
if ((uint)destination.Length < 16)
Copy link
Member

@EgorBo EgorBo Jul 23, 2024

Choose a reason for hiding this comment

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

Jit definitely shouldn't need this, it should be aware that Span.Length is never negative

Copy link
Contributor Author

@xtqqczze xtqqczze Jul 23, 2024

Choose a reason for hiding this comment

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

There is an unsigned length check in MemoryMarshal.Write:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MemoryMarshal.cs

Line 514 in 0378936

if ((uint)sizeof(T) > (uint)destination.Length)

The JIT won't elide the check in the case where one length check is signed, the other unsigned:

https://csharp.godbolt.org/z/WTj89j5Mn

Copy link
Contributor Author

@xtqqczze xtqqczze Jul 23, 2024

Choose a reason for hiding this comment

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

The check in MemoryMarshal.Write could be made signed, but then there could be a problem calling Write with a sliced span, due to the unsigned conditionals in Span.Slice.

Copy link
Member

@EgorBo EgorBo Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see bound checks in your godbolt link, I do see two similar conditions that jit didn't remove - it's a different issue that we should also fix. Like I said previously, if JIT fails to fix an obvious case - we should file an issue instead of applying hacks in the codebase. These hacks still make sense for cases where JIT likely has no ability to know that certain external argument are never negative.

Copy link
Member

@EgorBo EgorBo Jul 23, 2024

Choose a reason for hiding this comment

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

I don't see bound checks

What I mean is that we currently have two different algorithms for bound checks specifically and a general redundant branch removal

Copy link
Member

@EgorBo EgorBo Jul 23, 2024

Choose a reason for hiding this comment

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

@AndyAyersMS you might be interested in the godbolt link above (https://csharp.godbolt.org/z/WTj89j5Mn) for RBO/JumpThreading missing opportunity

Copy link
Contributor Author

@xtqqczze xtqqczze Jul 23, 2024

Choose a reason for hiding this comment

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

There are similar conditions in Guid.TryWriteBytes and MemoryMarshal.Write, but no bounds checks per se .

Copy link
Member

@EgorBo EgorBo Jul 23, 2024

Choose a reason for hiding this comment

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

Btw, we have an issue to remove existing (uint) hacks - #67044 so we shouldn't do the opposite

Copy link
Contributor Author

@xtqqczze xtqqczze Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Btw, we have an issue to remove existing (uint) hacks - #67044 so we shouldn't do the opposite

xtqqczze@1520741

return false;

if (BitConverter.IsLittleEndian)
{
MemoryMarshal.TryWrite(destination, in this);
MemoryMarshal.Write(destination, in this);
}
else
{
// slower path for BigEndian
Guid guid = new Guid(MemoryMarshal.AsBytes(new ReadOnlySpan<Guid>(in this)), false);
MemoryMarshal.TryWrite(destination, in guid);
MemoryMarshal.Write(destination, in guid);
}
return true;
}

// Returns whether bytes are successfully written to given span.
public bool TryWriteBytes(Span<byte> destination, bool bigEndian, out int bytesWritten)
{
if (destination.Length < 16)
if ((uint)destination.Length < 16)
{
bytesWritten = 0;
return false;
}

if (BitConverter.IsLittleEndian != bigEndian)
{
MemoryMarshal.TryWrite(destination, in this);
MemoryMarshal.Write(destination, in this);
}
else
{
// slower path for Reverse
Guid guid = new Guid(MemoryMarshal.AsBytes(new ReadOnlySpan<Guid>(in this)), bigEndian);
MemoryMarshal.TryWrite(destination, in guid);
MemoryMarshal.Write(destination, in guid);
}
bytesWritten = 16;
return true;
Expand Down