From 5582c767b9b43706f381b03cc7fe11834baff652 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 18 Dec 2024 22:35:04 +0100 Subject: [PATCH 1/5] Remove unsafe code from IPAddress --- .../src/System/Net/IPAddress.cs | 66 +++++-------------- 1 file changed, 17 insertions(+), 49 deletions(-) diff --git a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs index 6d2d6b75a8235..54c723ab05dad 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs @@ -10,8 +10,6 @@ using System.Runtime.InteropServices; using System.Runtime.Intrinsics; -#pragma warning disable SA1648 // TODO: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3595 - namespace System.Net { /// @@ -201,15 +199,12 @@ public IPAddress(ReadOnlySpan address) private static ushort[] ReadUInt16NumbersFromBytes(ReadOnlySpan address) { ushort[] numbers = new ushort[NumberOfLabels]; - if (Vector128.IsHardwareAccelerated) + if (Vector128.IsHardwareAccelerated && BitConverter.IsLittleEndian) { - Vector128 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 ushorts = Vector128.Create(address).AsUInt16(); + // Reverse endianness of each ushort + ushorts = (ushorts << 8) | (ushorts >> 8); + ushorts.StoreUnsafe(ref numbers[0]); } else { @@ -347,23 +342,13 @@ public bool TryWriteBytes(Span destination, out int bytesWritten) private void WriteIPv6Bytes(Span destination) { ushort[]? numbers = _numbers; - Debug.Assert(numbers != null && numbers.Length == NumberOfLabels); + Debug.Assert(numbers is { Length: NumberOfLabels }); - if (BitConverter.IsLittleEndian) + if (BitConverter.IsLittleEndian && Vector128.IsHardwareAccelerated) { - if (Vector128.IsHardwareAccelerated) - { - Vector128 ushorts = Vector128.LoadUnsafe(ref MemoryMarshal.GetArrayDataReference(numbers)); - ushorts = Vector128.ShiftLeft(ushorts, 8) | Vector128.ShiftRightLogical(ushorts, 8); - ushorts.AsByte().StoreUnsafe(ref MemoryMarshal.GetReference(destination)); - } - else - { - for (int i = 0; i < numbers.Length; i++) - { - BinaryPrimitives.WriteUInt16BigEndian(destination.Slice(i * 2), numbers[i]); - } - } + Vector128 ushorts = Vector128.Create(numbers).AsUInt16(); + ushorts = (ushorts << 8) | (ushorts >> 8); + ushorts.AsByte().StoreUnsafe(ref destination[0]); } else { @@ -387,7 +372,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; @@ -633,15 +618,7 @@ public bool IsIPv4MappedToIPv6 { get { - if (IsIPv4) - { - return false; - } - - ReadOnlySpan numbers = MemoryMarshal.AsBytes(new ReadOnlySpan(_numbers)); - return - MemoryMarshal.Read(numbers) == 0 && - BinaryPrimitives.ReadUInt32LittleEndian(numbers.Slice(8)) == 0xFFFF0000; + return !IsIPv4 && _numbers.AsSpan(0, 6).SequenceEqual((ReadOnlySpan)[0, 0, 0, 0, 0, 0xFFFF]); } } @@ -695,18 +672,12 @@ internal bool Equals(IPAddress comparand) if (IsIPv6) { // For IPv6 addresses, we must compare the full 128-bit representation and the scope IDs. - ReadOnlySpan thisNumbers = MemoryMarshal.AsBytes(_numbers); - ReadOnlySpan comparandNumbers = MemoryMarshal.AsBytes(comparand._numbers); - return - MemoryMarshal.Read(thisNumbers) == MemoryMarshal.Read(comparandNumbers) && - MemoryMarshal.Read(thisNumbers.Slice(sizeof(ulong))) == MemoryMarshal.Read(comparandNumbers.Slice(sizeof(ulong))) && + return _numbers.AsSpan(0, 16).SequenceEqual(comparand._numbers.AsSpan(0, 16)) && PrivateScopeId == comparand.PrivateScopeId; } - else - { - // For IPv4 addresses, compare the integer representation. - return comparand.PrivateAddress == PrivateAddress; - } + + // For IPv4 addresses, compare the integer representation. + return comparand.PrivateAddress == PrivateAddress; } public override int GetHashCode() @@ -743,10 +714,7 @@ public IPAddress MapToIPv6() } uint address = (uint)NetworkToHostOrder(unchecked((int)PrivateAddress)); - ushort[] labels = new ushort[NumberOfLabels]; - labels[5] = 0xFFFF; - labels[6] = (ushort)(address >> 16); - labels[7] = (ushort)address; + ReadOnlySpan labels = [0, 0, 0, 0, 0, 0xFFFF, (ushort)(address >> 16), (ushort)address]; return new IPAddress(labels, 0); } From 771499c031d45e3afaed88d0ee47c2270b2c960c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 18 Dec 2024 22:39:25 +0100 Subject: [PATCH 2/5] revert pragma --- src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs index 54c723ab05dad..963ed47965511 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs @@ -10,6 +10,8 @@ using System.Runtime.InteropServices; using System.Runtime.Intrinsics; +#pragma warning disable SA1648 // TODO: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3595 + namespace System.Net { /// From 61447b132098a47764018695d1eec4dd9765298d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 18 Dec 2024 23:34:40 +0100 Subject: [PATCH 3/5] fix bug --- src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs index 963ed47965511..bde115ad61179 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs @@ -674,7 +674,7 @@ internal bool Equals(IPAddress comparand) if (IsIPv6) { // For IPv6 addresses, we must compare the full 128-bit representation and the scope IDs. - return _numbers.AsSpan(0, 16).SequenceEqual(comparand._numbers.AsSpan(0, 16)) && + return _numbers.AsSpan(0, 8).SequenceEqual(comparand._numbers.AsSpan(0, 8)) && PrivateScopeId == comparand.PrivateScopeId; } From a36701970f4467dea3558a45d390d76174d26a73 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 18 Dec 2024 23:49:45 +0100 Subject: [PATCH 4/5] address feedback --- .../src/System/Net/IPAddress.cs | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs index bde115ad61179..752a9d2bb7636 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs @@ -206,7 +206,7 @@ private static ushort[] ReadUInt16NumbersFromBytes(ReadOnlySpan address) Vector128 ushorts = Vector128.Create(address).AsUInt16(); // Reverse endianness of each ushort ushorts = (ushorts << 8) | (ushorts >> 8); - ushorts.StoreUnsafe(ref numbers[0]); + ushorts.CopyTo(numbers); } else { @@ -346,11 +346,21 @@ private void WriteIPv6Bytes(Span destination) ushort[]? numbers = _numbers; Debug.Assert(numbers is { Length: NumberOfLabels }); - if (BitConverter.IsLittleEndian && Vector128.IsHardwareAccelerated) + if (BitConverter.IsLittleEndian) { - Vector128 ushorts = Vector128.Create(numbers).AsUInt16(); - ushorts = (ushorts << 8) | (ushorts >> 8); - ushorts.AsByte().StoreUnsafe(ref destination[0]); + if (Vector128.IsHardwareAccelerated) + { + Vector128 ushorts = Vector128.Create(numbers).AsUInt16(); + ushorts = (ushorts << 8) | (ushorts >> 8); + ushorts.AsByte().CopyTo(destination); + } + else + { + for (int i = 0; i < numbers.Length; i++) + { + BinaryPrimitives.WriteUInt16BigEndian(destination.Slice(i * 2), numbers[i]); + } + } } else { From 8dab38f1e0ca054db728c8608a9c4cdccdbfeb84 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 19 Dec 2024 03:17:43 +0100 Subject: [PATCH 5/5] Add comments --- .../src/System/Net/IPAddress.cs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs index 752a9d2bb7636..cd7e281e541c5 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs @@ -684,8 +684,15 @@ internal bool Equals(IPAddress comparand) if (IsIPv6) { // For IPv6 addresses, we must compare the full 128-bit representation and the scope IDs. - return _numbers.AsSpan(0, 8).SequenceEqual(comparand._numbers.AsSpan(0, 8)) && - PrivateScopeId == comparand.PrivateScopeId; + // We give JIT a hint that the arrays are always of length IPv6AddressShorts, so it + // 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 left = _numbers.AsSpan(0, IPAddressParserStatics.IPv6AddressShorts); + ReadOnlySpan right = comparand._numbers.AsSpan(0, IPAddressParserStatics.IPv6AddressShorts); + return left.SequenceEqual(right) && PrivateScopeId == comparand.PrivateScopeId; } // For IPv4 addresses, compare the integer representation. @@ -726,7 +733,10 @@ public IPAddress MapToIPv6() } uint address = (uint)NetworkToHostOrder(unchecked((int)PrivateAddress)); - ReadOnlySpan labels = [0, 0, 0, 0, 0, 0xFFFF, (ushort)(address >> 16), (ushort)address]; + ushort[] labels = new ushort[NumberOfLabels]; + labels[5] = 0xFFFF; + labels[6] = (ushort)(address >> 16); + labels[7] = (ushort)address; return new IPAddress(labels, 0); }