Updating to a version of Roslyn that contains the NumericIntPtr feature #69627

tannergooding · 2022-05-20T21:30:43Z

For .NET 7/C# 11, IntPtr and nint are now aliases of each other in the same way that Int32 and int are.

This updates to Roslyn v4.3.0-2.22270.2 which includes the relevant changes and updates IntPtr/UIntPtr to take advantage of the feature accordingly. For the IntPtr/UIntPtr tests, they now use reflection to ensure that the user-defined operators are still called and validated where relevant. They have likewise been updated to otherwise take advantage of the feature.

There were a handful of Span/ROSpan tests that likewise required updating due to the constant being "too large" and now trigger the relevant warning that it may fail on 32-bit platforms. Otherwise, no other code is being touched in this PR. A separate PR can be put up that switches the BCL over to use the keyword but that is "less important" since it isn't required to ship nor will it otherwise impact codegen now.

…intptr feature

dotnet-issue-labeler · 2022-05-20T21:30:47Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-issue-labeler · 2022-05-20T21:30:48Z

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.

ghost · 2022-05-20T21:31:47Z

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

Issue Details

For .NET 7/C# 11, IntPtr and nint are now aliases of each other in the same way that Int32 and int are.

This updates to Roslyn v4.3.0-2.22270.2 which includes the relevant changes and updates IntPtr/UIntPtr to take advantage of the feature accordingly. For the IntPtr/UIntPtr tests, they now use reflection to ensure that the user-defined operators are still called and validated where relevant. They have likewise been updated to otherwise take advantage of the feature.

There were a handful of Span/ROSpan tests that likewise required updating due to the constant being "too large" and now trigger the relevant warning that it may fail on 32-bit platforms. Otherwise, no other code is being touched in this PR. A separate PR can be put up that switches the BCL over to use the keyword but that is "less important" since it isn't required to ship nor will it otherwise impact codegen now.

Author:	tannergooding
Assignees:	tannergooding
Labels:	`area-System.Runtime`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2022-05-20T21:33:54Z

CC. @jcouv as an FYI

eerhardt · 2022-05-20T22:15:46Z

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs

@@ -34,136 +33,125 @@ namespace System
          IMinMaxValue<nint>,
          ISignedNumber<nint>
    {
-        private readonly unsafe void* _value; // Do not rename (binary serialization)


(nit) Do we want to keep the comment here, since we still support binary serialization (for now)?

@GrabYourPitchforks indicated the comment is out of date since we have an explicit serializer method.

To expand on this a little bit, the ISerializable interface tells reflection-based serializers, "Stop! There's something special about my layout and you shouldn't perform your normal scrape-the-fields logic when serializing me. Instead, call GetObjectData and my special ctor, and I'll tell you how to serialize my own instances."

So as long as the type has ISerializable on it (or we add it), we have considerable leeway to change the internal layout of the type with regard to serialization. Debuggers (SOS) may need to be updated separately, though.

eerhardt · 2022-05-20T22:17:39Z

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs


        [NonVersionable]
-        public static unsafe explicit operator IntPtr(long value) =>
-            new IntPtr(value);
+        public static explicit operator nint(long value) => checked((nint)value);


For my education, how doesn't this result in an infinite loop?

The same as for Int32. Basically the C# compiler ignores all "user-defined operators" for the primitive types and uses its own "built-in" operators instead.

So the user-defined method basically only exists for "back-compat" and can only be invoked indirectly such as via reflection with the C# compiler just emitting conv.ovf.i for checked((nint)value)

dakersnar

I don't have a technical grasp on the logic behind all of these updates, but at a high level this change makes sense, and I trust that this is mostly mechanical. I recommend someone else take a look at some point, but I can approve for now to unblock you.

dakersnar · 2022-05-20T22:12:29Z

src/libraries/System.Memory/tests/Span/Overflow.cs

@@ -78,7 +78,7 @@ public static void SliceStartInt32Overflow()
            {
                unsafe
                {
-                    if (!AllocationHelper.TryAllocNative((IntPtr)ThreeGiB, out IntPtr memory))
+                    if (!AllocationHelper.TryAllocNative(unchecked((nint)ThreeGiB), out IntPtr memory))


Did IntPtr used to behave as an unchecked nint, hence this update being needed now that IntPtr has different behavior?

IntPtr had no language support while nint did. This meant that (IntPtr)TooLargeConstant would issue no error while (nint)TooLargeConstant would issue a error.

Now that IntPtr/nint are aliases and have all the same functionality, IntPtr issues an error requiring you to acknowledge the potential overflow that existed here and use unchecked.

tannergooding · 2022-05-20T22:34:09Z

GenAPI changes are here: dotnet/arcade#9421

jkotas · 2022-05-20T22:40:44Z

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs

        {
            if (destination.Length >= sizeof(nint_t))
            {
-                nint_t value = BitConverter.IsLittleEndian ? (nint_t)(_value) : BinaryPrimitives.ReverseEndianness((nint_t)(_value));
+                nint_t value = (nint_t)_value;


Incorrectly resolved merge conflict? This seems to be undoing #69063

No. #69063 didn't touch these files, only System.Buffers.Binary.BinaryPrimitives

For writing the pattern looks to be the below, where-as the ternary expression is for reads.

if (!BitConverter.IsLittleEndian) { nint_t value = ReverseEndianness((nint_t)_value); return MemoryMarshal.Write(destination, ref value); } return MemoryMarshal.Write(destination, ref _value);

I'd need to check how this plays into Unsafe.WriteUnaligned, but more generally this should probably just use #if BIGENDIAN and avoid the larger IL and branches altogether.

I'd suspect the "ideal" here is the following and I can log an issue to track validating this and doing the "optimal" thing:

#if BIGENDIAN return Unsafe.WriteUnaligned(ref MemoryMarshal.GetReference(destination), BinaryPrimitives.ReverseEndianness((nint_t)_value)); #else return Unsafe.WriteUnaligned(ref MemoryMarshal.GetReference(destination), _value); #endif

… check the overflow exception

tannergooding · 2022-05-20T23:13:11Z

There are microsoft.dotnet.compatibility failures that I'm looking at. It seems the analyzer isn't happy with the compiler changes around IntPtr

….22252.4

tannergooding · 2022-05-20T23:29:56Z

Updated the analyzer and the CompatibilitySuppressions files. Logged dotnet/sdk#25566 to ensure the analyzer correctly handles this scenario so we can remove the unnecessary suppressions.

I did manually validate that the APIs do still exist in the packages.

…ected call on Mono

tannergooding · 2022-05-21T00:18:13Z

src/libraries/System.Runtime/tests/System/IntPtrTests.cs

+            if (ex is TargetInvocationException)
+            {
+                // RyuJIT throws TargetInvocationException wrapping an OverflowException
+                // while Mono directly throws the OverflowException
+                ex = ex.InnerException;
+            }
+            Assert.IsType<OverflowException>(ex);


Turns out Mono and RyuJIT have different behavior for how the exception is thrown here.

GrabYourPitchforks · 2022-05-21T00:58:21Z

src/libraries/System.Private.CoreLib/src/System/IntPtr.cs


-            if (Size == 4 && (l > int.MaxValue || l < int.MinValue))
+#if TARGET_32BIT
+            if ((value > int.MaxValue) || (value < int.MinValue))


A good example of where #46259 (comment) and #54439 would come in handy. Would also allow you to get rid of the ifdef.

I think I'd prefer the ifdef here anyways. This isn't exactly perf critical, but it is one of the "core types" in the BCL and we have the infrastructure to minimize the IL here.

For libraries that aren't corelib and aren't getting built uniquely for each OS/Architecture, 100% agree

GrabYourPitchforks · 2022-05-21T01:13:32Z

src/libraries/System.Runtime/tests/System/UIntPtrTests.cs

-            VerifyPointer(new UIntPtr(i), i);
-            VerifyPointer((UIntPtr)i, i);
+            VerifyPointer(new nuint(i), i);
+            VerifyPointer(i, i);


I don't think this is testing the correct thing now. (And in fact it may have been broken during the earlier nint / IntPtr consolidation.) The C# compiler in netcore emits a conv.u instruction, whereas the compiler in netfx would've emitted a call (op explicit) instruction. This line presumably intends to test our implementation of the explicit operator. We might even need to drop down to reflection to call it now, similar to what you've done in other test methods here. Alternatively, just kill it.

This line presumably intends to test our implementation of the explicit operator. We might even need to drop down to reflection to call it now, similar to what you've done in other test methods here. Alternatively, just kill it.

Given that this test is for Ctor_UInt and there is a separate test covering explicit conversions to/from, I took this as just wanting to separately validate that the constructor returns the same value as a cast and not as explicitly wanting to compare new nuint vs the user-defined nuint op_Explicit(uint).

It's definitely possible that it could have been trying to validate the latter instead, but the TestExplicitCast test is already covering this exact value anyways so it doesn't buy much to do it.

I'd be amenable to removing it if you'd prefer.

tannergooding added 2 commits May 20, 2022 11:11

Update MicrosoftNetCompilersToolsetVersion to 4.3.0-2.22270.2

335fa7a

Updating IntPtr/UIntPtr and a handful of tests to handle the numeric-…

8b1603b

…intptr feature

ghost assigned tannergooding May 20, 2022

dotnet-issue-labeler bot added the new-api-needs-documentation label May 20, 2022

tannergooding added the area-System.Runtime label May 20, 2022

Merge remote-tracking branch 'dotnet/main' into numeric-intptr

d4ebd53

eerhardt reviewed May 20, 2022

View reviewed changes

dakersnar approved these changes May 20, 2022

View reviewed changes

tannergooding mentioned this pull request May 20, 2022

Updating GenAPI to properly handle nint for C# 9 and later dotnet/arcade#9421

Merged

jkotas reviewed May 20, 2022

View reviewed changes

Ensure IntPtr.TestExplicitCast and UIntPtr.TestExplicitCast correctly…

d4eef79

… check the overflow exception

tannergooding added 2 commits May 20, 2022 16:16

Update the Microsoft.Dotnet.Compatibility analyzer to 2.0.0-preview.4…

a69c58c

….22252.4

Updating the CompatibilitySuppressions for the NumericIntPtr feature

80f4558

tannergooding mentioned this pull request May 20, 2022

Microsoft.Dotnet.Compatibility analyzer doesn't work with the NumericIntPtr feature dotnet/sdk#25566

Closed

Handle a difference in how the OverflowException is thrown for a refl…

05f39c8

…ected call on Mono

tannergooding commented May 21, 2022

View reviewed changes

Use Assert.ThrowsAny as that includes derived types for xUnit

975279f

GrabYourPitchforks reviewed May 21, 2022

View reviewed changes

GrabYourPitchforks approved these changes May 21, 2022

View reviewed changes

tannergooding merged commit 7626c5d into dotnet:main May 21, 2022

kunalspathak mentioned this pull request May 24, 2022

Regressions in multiple benchmarks after char.Is* update #69727

Closed

kunalspathak mentioned this pull request Jun 2, 2022

[Perf] Changes at 5/20/2022 10:18:01 PM dotnet/perf-autofiling-issues#5494

Closed

This was referenced Jun 2, 2022

[Perf] Changes at 5/21/2022 4:54:11 AM dotnet/perf-autofiling-issues#5478

Closed

[Perf] Changes at 5/21/2022 4:54:11 AM dotnet/perf-autofiling-issues#5461

Closed

dakersnar mentioned this pull request Jun 10, 2022

.NET 7.0 Preview 5 Microbenchmarks Performance Study Report #70590

Closed

14 tasks

ghost locked as resolved and limited conversation to collaborators Jun 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating to a version of Roslyn that contains the NumericIntPtr feature #69627

Updating to a version of Roslyn that contains the NumericIntPtr feature #69627

tannergooding commented May 20, 2022

dotnet-issue-labeler bot commented May 20, 2022

dotnet-issue-labeler bot commented May 20, 2022

ghost commented May 20, 2022

tannergooding commented May 20, 2022

eerhardt May 20, 2022 •

edited

Loading

tannergooding May 20, 2022

GrabYourPitchforks May 21, 2022

eerhardt May 20, 2022

tannergooding May 20, 2022

dakersnar left a comment

dakersnar May 20, 2022

tannergooding May 20, 2022

tannergooding commented May 20, 2022

jkotas May 20, 2022

tannergooding May 20, 2022 •

edited

Loading

tannergooding commented May 20, 2022

tannergooding commented May 20, 2022

tannergooding May 21, 2022

GrabYourPitchforks May 21, 2022 •

edited

Loading

tannergooding May 21, 2022

tannergooding May 21, 2022

GrabYourPitchforks May 21, 2022 •

edited

Loading

tannergooding May 21, 2022 •

edited

Loading

Updating to a version of Roslyn that contains the NumericIntPtr feature #69627

Updating to a version of Roslyn that contains the NumericIntPtr feature #69627

Conversation

tannergooding commented May 20, 2022

dotnet-issue-labeler bot commented May 20, 2022

dotnet-issue-labeler bot commented May 20, 2022

ghost commented May 20, 2022

tannergooding commented May 20, 2022

eerhardt May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dakersnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented May 20, 2022

Choose a reason for hiding this comment

tannergooding May 20, 2022 • edited Loading

Choose a reason for hiding this comment

tannergooding commented May 20, 2022

tannergooding commented May 20, 2022

Choose a reason for hiding this comment

GrabYourPitchforks May 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrabYourPitchforks May 21, 2022 • edited Loading

Choose a reason for hiding this comment

tannergooding May 21, 2022 • edited Loading

Choose a reason for hiding this comment

eerhardt May 20, 2022 •

edited

Loading

tannergooding May 20, 2022 •

edited

Loading

GrabYourPitchforks May 21, 2022 •

edited

Loading

GrabYourPitchforks May 21, 2022 •

edited

Loading

tannergooding May 21, 2022 •

edited

Loading