Skip to content
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

Remove unsafe code from IPAddress #110824

Merged
merged 5 commits into from
Dec 19, 2024
Merged

Remove unsafe code from IPAddress #110824

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5582c76
Remove unsafe code from IPAddress
EgorBo Dec 18, 2024
771499c
revert pragma
EgorBo Dec 18, 2024
61447b1
fix bug
EgorBo Dec 18, 2024
a367019
address feedback
EgorBo Dec 18, 2024
8dab38f
Add comments
EgorBo Dec 19, 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
56 changes: 23 additions & 33 deletions src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,15 +201,12 @@ public IPAddress(ReadOnlySpan<byte> address)
private static ushort[] ReadUInt16NumbersFromBytes(ReadOnlySpan<byte> address)
{
ushort[] numbers = new ushort[NumberOfLabels];
if (Vector128.IsHardwareAccelerated)
if (Vector128.IsHardwareAccelerated && BitConverter.IsLittleEndian)
{
Vector128<ushort> ushorts = Vector128.LoadUnsafe(ref MemoryMarshal.GetReference(address)).AsUInt16();
if (BitConverter.IsLittleEndian)
{
// Reverse endianness of each ushort
ushorts = Vector128.ShiftLeft(ushorts, 8) | Vector128.ShiftRightLogical(ushorts, 8);
}
ushorts.StoreUnsafe(ref MemoryMarshal.GetArrayDataReference(numbers));
Vector128<ushort> ushorts = Vector128.Create(address).AsUInt16();
// Reverse endianness of each ushort
ushorts = (ushorts << 8) | (ushorts >> 8);
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
ushorts.CopyTo(numbers);
}
else
{
Expand Down Expand Up @@ -347,15 +344,15 @@ public bool TryWriteBytes(Span<byte> destination, out int bytesWritten)
private void WriteIPv6Bytes(Span<byte> destination)
{
ushort[]? numbers = _numbers;
Debug.Assert(numbers != null && numbers.Length == NumberOfLabels);
Debug.Assert(numbers is { Length: NumberOfLabels });

if (BitConverter.IsLittleEndian)
{
if (Vector128.IsHardwareAccelerated)
{
Vector128<ushort> ushorts = Vector128.LoadUnsafe(ref MemoryMarshal.GetArrayDataReference(numbers));
ushorts = Vector128.ShiftLeft(ushorts, 8) | Vector128.ShiftRightLogical(ushorts, 8);
ushorts.AsByte().StoreUnsafe(ref MemoryMarshal.GetReference(destination));
Vector128<ushort> ushorts = Vector128.Create(numbers).AsUInt16();
ushorts = (ushorts << 8) | (ushorts >> 8);
ushorts.AsByte().CopyTo(destination);
}
else
{
Expand Down Expand Up @@ -387,7 +384,7 @@ public byte[] GetAddressBytes()
{
if (IsIPv6)
{
Debug.Assert(_numbers != null && _numbers.Length == NumberOfLabels);
Debug.Assert(_numbers is { Length: NumberOfLabels });
byte[] bytes = new byte[IPAddressParserStatics.IPv6AddressBytes];
WriteIPv6Bytes(bytes);
return bytes;
Expand Down Expand Up @@ -633,15 +630,7 @@ public bool IsIPv4MappedToIPv6
{
get
{
if (IsIPv4)
{
return false;
}

ReadOnlySpan<byte> numbers = MemoryMarshal.AsBytes(new ReadOnlySpan<ushort>(_numbers));
return
MemoryMarshal.Read<ulong>(numbers) == 0 &&
BinaryPrimitives.ReadUInt32LittleEndian(numbers.Slice(8)) == 0xFFFF0000;
return !IsIPv4 && _numbers.AsSpan(0, 6).SequenceEqual((ReadOnlySpan<ushort>)[0, 0, 0, 0, 0, 0xFFFF]);
}
}

Expand Down Expand Up @@ -695,18 +684,19 @@ internal bool Equals(IPAddress comparand)
if (IsIPv6)
{
// For IPv6 addresses, we must compare the full 128-bit representation and the scope IDs.
ReadOnlySpan<byte> thisNumbers = MemoryMarshal.AsBytes<ushort>(_numbers);
ReadOnlySpan<byte> comparandNumbers = MemoryMarshal.AsBytes<ushort>(comparand._numbers);
return
MemoryMarshal.Read<ulong>(thisNumbers) == MemoryMarshal.Read<ulong>(comparandNumbers) &&
MemoryMarshal.Read<ulong>(thisNumbers.Slice(sizeof(ulong))) == MemoryMarshal.Read<ulong>(comparandNumbers.Slice(sizeof(ulong))) &&
PrivateScopeId == comparand.PrivateScopeId;
}
else
{
// For IPv4 addresses, compare the integer representation.
return comparand.PrivateAddress == PrivateAddress;
// We give JIT a hint that the arrays are always of length IPv6AddressShorts, so it
Copy link

Choose a reason for hiding this comment

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

Does Debug.Assert have a side effect on JIT decisions? Even in Release builds? Is it mentioned somewhere or is it not guaranteed? What if the assertion no longer holds in Release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does Debug.Assert have a side effect on JIT decisions? Even in Release builds? Is it mentioned somewhere or is it not guaranteed? What if the assertion no longer holds in Release?

Debug.Assert doesn't exist in Release code, it's Debug only

Copy link

Choose a reason for hiding this comment

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

Thank you for clarification. I misunderstood your code comment, and thus the question arose.

// can unroll the comparison. JIT probably could do it on its own, but that requires
// complex inter-procedural optimizations.
Debug.Assert(_numbers.Length == IPAddressParserStatics.IPv6AddressShorts);
Debug.Assert(comparand._numbers!.Length == IPAddressParserStatics.IPv6AddressShorts);

ReadOnlySpan<ushort> left = _numbers.AsSpan(0, IPAddressParserStatics.IPv6AddressShorts);
ReadOnlySpan<ushort> right = comparand._numbers.AsSpan(0, IPAddressParserStatics.IPv6AddressShorts);
return left.SequenceEqual(right) && PrivateScopeId == comparand.PrivateScopeId;
}

// For IPv4 addresses, compare the integer representation.
return comparand.PrivateAddress == PrivateAddress;
}

public override int GetHashCode()
Expand Down
Loading