Remove S.R.CS.Unsafe and Intrinsify Unsafe

[Wraith2]

Closes #45475

• Moves Unsafe into corelib
• Implements all Unsafe members as vm intrinsics in coreclr and nativeaot (mono changes required are detailed in the issue but are not implemented in this PR)
• Removes the System.Runtime.CompilerServices.Unsafe il assembly and replaces it with type forwards

[dotnet-issue-labeler]

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.

[ghost]

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

Issue Details

---

Closes #45475

• Moves Unsafe into corelib
• Implements all Unsafe members as vm intrinsics in coreclr and nativeaot (mono changes required are detailed in the issue but are not implemented in this PR)
• Removes the System.Runtime.CompilerServices.Unsafe il assembly and replaces it with type forwards

Author:	Wraith2
Assignees:	-
Labels:	area-Meta, new-api-needs-documentation
Milestone:	-

[ghost]

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

Issue Details

---

Closes #45475

• Moves Unsafe into corelib
• Implements all Unsafe members as vm intrinsics in coreclr and nativeaot (mono changes required are detailed in the issue but are not implemented in this PR)
• Removes the System.Runtime.CompilerServices.Unsafe il assembly and replaces it with type forwards

Author:	Wraith2
Assignees:	-
Labels:	area-Meta, area-System.Runtime.CompilerServices, new-api-needs-documentation, community-contribution
Milestone:	-

[Wraith2]

I don't understand how the main errors that i'm seeing in the CI are being caused. they are:

src/libraries/Common/src/Interop/Interop.Odbc.cs(157,34): error DLLIMPORTGEN001: (NETCORE_ENGINEERING_TELEMETRY=Build) The type 'byte[]' is not supported by source-generated P/Invokes. The generated source will not handle marshalling of parameter 'Value'

I changed the dll import generator to remove the Internal vs System choice but that shouldn't cause any functional change to how it works. Does anyone have any ideas?

[jkotas]

I changed the dll import generator to remove the Internal vs System choice but that shouldn't cause any functional change to how it works. Does anyone have any ideas?

@jkoritzinsky ?

[Wraith2]

I think that the problem with generated imports is coming from this:

// TODO: Remove once helper types (like ArrayMarshaller) are part of the runtime
// This check is to help with enabling the source generator for runtime libraries without making each
// library directly reference System.Memory and System.Runtime.CompilerServices.Unsafe unless it needs to
if (p.MarshallingAttributeInfo is MissingSupportMarshallingInfo
    && (environment.TargetFramework == TargetFramework.Net && environment.TargetFrameworkVersion.Major >= 7))
{
    throw new MarshallingNotSupportedException(p, this);
}

[jkotas]

This does not look right. Why is it needed?

[Wraith2]

See: #64861 (comment)

[Wraith2]

While debugging the trimming test failures i realised that there was a possible problem in trimmed scenarios. When resolving any unsafe method in the jit it walks along a list of if-else conditions one for each intrinsic and until it finds a match it will fetch the method description for the intrinsic to check against, if the intrinsic it's checking against has been trimmed out then the method description won't be found and it'll fail with an AV/segfault. Essentially any trimmed unsafe method could cause resolution of one which is still present to fail depending on their order of checks in the jit code.

There are two ways to fix this that I can see. The simple one is to add an ILLInk preservation element so that unsafe is never trimmed. The second would be to change the intrinsic lookup to use a function which can early exit if the lookup doesn't produce a result and then use that everywhere. For now I've gone with the illink route but I would consider the other way cleaner.

[jkotas]

All types and methods in corelib.h are preserved via auto-generated ILLink descriptor. You do not need to pin them manually.

The auto-generated ILLink desctriptor is only used for CoreCLR w/JIT. It is not used for NativeAOT and Mono since they are able to deal with the type completely missing just fine.

If you add this type to the shared ILLink descriptor, the type will be always preserved on all runtime, and so it will produce unnecessary waste on NativeAOT and Mono.

[Wraith2]

Ok. Removed from the commit.

[Wraith2]

Libraries build failures are net6.0 target libraries, e.g.:

They'll need changing so they know to pick up the correct location for Unsafe, I'm guessing @ericstj is the person to ask on this one?

[jkotas]

Libraries build failures are net6.0 target libraries, e.g.

Take a look how System.Memory is referenced in System.Collections.Immutable. S.R.CS.Unsafe needs to use the same pattern.

[jkotas]

SystemRuntimeCompilerServicesUnsafeVersion

Could you please also change SystemRuntimeCompilerServicesUnsafeVersion in eng\Versions.props back to 6.0.0?

[Wraith2]

Could you please also change SystemRuntimeCompilerServicesUnsafeVersion in eng\Versions.props back to 6.0.0

Done. Also rebased to clear the error caused by number formatting's new use of unsafe.

[Wraith2]

Detected package downgrade: System.Runtime.CompilerServices.Unsafe from 7.0.0-preview.2.22103.2 to 6.0.0. Reference the package directly from the project to select a different version. 
 Microsoft.NET.HostModel -> System.Text.Json 7.0.0-preview.2.22103.2 -> System.Runtime.CompilerServices.Unsafe (>= 7.0.0-preview.2.22103.2) 
 Microsoft.NET.HostModel -> System.Runtime.CompilerServices.Unsafe (>= 6.0.0)	Microsoft.NET.HostModel	E:\Programming\csharp7\runtime\src\installer\managed\Microsoft.NET.HostModel\Microsoft.NET.HostModel.csproj	

Not sure what to do with this one. The S.R.CS.Unsafe package is already being referenced directly from the projects that are getting the error. I considered changing System.Text.Json to force the 6.0.0 version but that would prevent them using any newer unsafe methods and they are a likely place to need them for perf.

[Wraith2]

Thanks for all the help with this, especially @BrzVlad for supplying the mono intrinsics that I wasn't able to write.

[kunalspathak]

Some improvements dotnet/perf-autofiling-issues#3642, dotnet/perf-autofiling-issues#3652, dotnet/perf-autofiling-issues#3657, dotnet/perf-autofiling-issues#3664, dotnet/perf-autofiling-issues#3666, dotnet/perf-autofiling-issues#3673

[tannergooding]

My expectation is that most of these are due to PGO data becoming stale due to the move of these types into S.P.Corelib.

There wasn't really any major changes to the IL used here and if those minor changes are impacting perf then that's likely a separate issue to look at.

[kunalspathak]

Arm64 regression in dotnet/perf-autofiling-issues#3693

[ghost]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

[ViktorHofer]

I just added the breaking-change label to this PR to make sure that a breaking-change / announcement issue is posted when .NET 7 ships. For .NET 6, an announcement was posted that listed all the packages that were dropped because they were moved inbox only: dotnet/announcements#190 (the list at the bottom before the Timelines section).

[stephentoub]

What is the breaking change?

[ViktorHofer]

The relevant section from the announcement issue that was posted with .NET 6:

These packages will no longer be updated because their implementation is now part of the .NET 6 platform:

Apparently removing a shipping package which shipped in a previous .NET version is considered announcement worthy. In the past the breaking-change label was used to track such removals even though the change itself isn't breaking.

#36707 removed a package and @eerhardt added the breaking-change label and also posted a breaking change doc: dotnet/docs#18438 even though the change itself wasn't breaking.

[mmitche]

@Wraith2 There is a dependency on this package in aspnetcore that needs removing from dependency tracking: https://github.com/dotnet/aspnetcore/blob/main/eng/Version.Details.xml#L216-L219 and https://github.com/dotnet/aspnetcore/blob/main/eng/Versions.props#L116

[jkotas]

There is a dependency on this package in aspnetcore that needs removing from dependency tracking:

Done: dotnet/aspnetcore#40471

[dreddy-work]

Seems this change is causing multiple issues in Winforms with 7.0 P3.
dotnet/winforms#6794
#65664

Bug 1495179: Intellitrace PDQ's cause hang in VS on .NET 7

Bug 1495188: VIL Does not Hook System.Runtime.CompilerServices.Unsafe.* on NET 7

[Wraith2]

Seems this change is causing multiple issues in Winforms with 7.0 P3.
dotnet/winforms#6794
#65664

I can't see the bugs on the internal tracker. What is the causal link between these bugs and moving of unsafe inbox?

[jkotas]

causing multiple issues in Winforms with 7.0 P3.

All issues you have linked look like dups of the same underlying issue in the VS VIL interpreter. @tommcdon Anything we can do to prioritize the fix?

I can't see the bugs on the internal tracker. What is the causal link between these bugs and moving of unsafe inbox?

The VS debugger has hardcoded assumptions about System.Private.CoreLib internal implementation details. It sometimes breaks when the CoreLib internal implementation details change in a substantial way.

[tommcdon]

All issues you have linked look like dups of the same underlying issue in the VS VIL interpreter. @tommcdon Anything we can do to prioritize the fix?

I'll work with the appropriate folks to prioritize the fix.
Disabling Intellitrace should workaround the Visual Studio hang issue.

[ericstj]

@dotnet/area-system-runtime-compilerservices can you please help by filing a breaking change document?

[joperezr]

[Triage] I'll take care of the breaking change document for this.