Update runtime repo to use Preview 7 SDK #72145

agocke · 2022-07-13T23:48:05Z

No description provided.

dotnet-issue-labeler · 2022-07-13T23:48:13Z

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.

ViktorHofer

LGTM. Of course, before merging we need to go through the usual process of communicating the change.

agocke · 2022-07-14T23:42:38Z

@MichalStrehovsky @LakshanF Looks like some NativeAOT libraries tests are failing after an SDK update. Ideas?

MichalStrehovsky · 2022-07-15T02:12:42Z

@MichalStrehovsky @LakshanF Looks like some NativeAOT libraries tests are failing after an SDK update. Ideas?

Looking... one thing that's missing is adding handling of the new TrimMode to NativeAOT targets. But the failure mode doesn't look like that's the problem.

MichalStrehovsky · 2022-07-15T02:40:24Z

@MichalStrehovsky @LakshanF Looks like some NativeAOT libraries tests are failing after an SDK update. Ideas?

Looking... one thing that's missing is adding handling of the new TrimMode to NativeAOT targets. But the failure mode doesn't look like that's the problem.

It's from the new aggresse trimming default in Preview 6. xUnit is calling PropertyInfo.GetValue on CollectionBehaviorAttribute.DisableTestParallelization but we trimmed the getter (the setter was left because the attribute is applied on something and we need the setter to materialize the Attribute instance at runtime - the getter is fair game).

This is actually a very lucky catch. Only a couple of the tests fail. The rest look like this:

Running assembly:System.Runtime.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51
Finished System.Runtime.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51

Tests run: 0, Errors: 0, Failures: 0, Skipped: 0. Time: 0.0130819s

The fix will be along the lines of adding <TrimMode>partial</TrimMode> to eng\testing\tests.singlefile.targets.

MichalStrehovsky · 2022-07-15T02:53:19Z

src/tests/nativeaot/Directory.Build.props

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
-    <TrimmerDefaultAction>link</TrimmerDefaultAction>
+    <TrimMode>full</TrimMode>


We'll probably need to do the opposite here - nativeaot tests expect trimming and require the (now default) TrimMode=full. So this line is technically not needed. But the rest of the coreclr test tree will want to keep the partial TrimMode.

MichalStrehovsky · 2022-07-15T02:53:57Z

src/coreclr/tools/aot/crossgen2/crossgen2.props

@@ -14,7 +14,7 @@
    <Configurations>Debug;Release;Checked</Configurations>
    <ServerGarbageCollection>true</ServerGarbageCollection>
    <!-- Trim all dependent assemblies as though they were all marked IsTrimmable. -->
-    <TrimmerDefaultAction>link</TrimmerDefaultAction>
+    <TrimMode>full</TrimMode>


Do we need this line if this is now the default?

No, we don't, I was just being explicit.

agocke · 2022-07-15T04:41:33Z

Does NativeAOT understand TrimMode=partial or does that need to be added?

agocke · 2022-07-15T04:51:16Z

/azp run runtime-extra-platforms,runtime-wasm

azure-pipelines · 2022-07-15T04:51:35Z

Azure Pipelines successfully started running 2 pipeline(s).

MichalStrehovsky · 2022-07-15T07:08:11Z

Does NativeAOT understand TrimMode=partial or does that need to be added?

I think #71117 is what's needed. Could be folded into this PR to avoid adding the TODO.

agocke · 2022-07-15T08:10:32Z

/azp run runtime-extra-platforms,runtime-wasm

azure-pipelines · 2022-07-15T08:10:52Z

Azure Pipelines successfully started running 2 pipeline(s).

agocke · 2022-07-15T22:18:10Z

/azp run runtime-extra-platforms,runtime-wasm

azure-pipelines · 2022-07-15T22:18:26Z

Azure Pipelines successfully started running 2 pipeline(s).

agocke · 2022-07-16T07:02:17Z

It looks like the installer tests are broken by dotnet/sdk#26462. The test execution happens through Arcade, so we can't pass DOTNET_ROOT as is.

I see two options for workaround:

Add a capability to Arcade to set environment variables for the test process
Update to an SDK daily build that contains the fix.

I don't see any way to use the Preview 6+ SDK otherwise.

AaronRobinsonMSFT · 2022-08-13T16:12:18Z

@MichalStrehovsky or @vitek-karas Any thoughts on the NativeAOT failures?

/_/src/xunit.execution/Extensions/ReflectionAbstractionExtensions.cs(92): Trim analysis error IL2070: ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Reflection.TypeInfo.DeclaredMethods.get'. The parameter 'type' of method 'ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [/__w/1/s/src/libraries/System.Runtime.Intrinsics/tests/System.Runtime.Intrinsics.Tests.csproj]
##[error]/_/src/xunit.execution/Extensions/ReflectionAbstractionExtensions.cs(92,0): error IL2070: (NETCORE_ENGINEERING_TELEMETRY=Build) ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Reflection.TypeInfo.DeclaredMethods.get'. The parameter 'type' of method 'ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/src/xunit.execution/Extensions/ReflectionAbstractionExtensions.cs(92): Trim analysis error IL2070: ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Reflection.TypeInfo.DeclaredMethods.get'. The parameter 'type' of method 'ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [/__w/1/s/src/libraries/System.Runtime.Intrinsics/tests/System.Runtime.Intrinsics.Tests.csproj]
##[error]/_/src/xunit.execution/Extensions/ReflectionAbstractionExtensions.cs(92,0): error IL2070: (NETCORE_ENGINEERING_TELEMETRY=Build) ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Reflection.TypeInfo.DeclaredMethods.get'. The parameter 'type' of method 'ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/src/xunit.execution/Extensions/ReflectionAbstractionExtensions.cs(92): Trim analysis error IL2067: ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Reflection.RuntimeReflectionExtensions.GetRuntimeMethods(Type)'. The parameter 'type' of method 'ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [/__w/1/s/src/libraries/System.Runtime.Intrinsics/tests/System.Runtime.Intrinsics.Tests.csproj]
##[error]/_/src/xunit.execution/Extensions/ReflectionAbstractionExtensions.cs(92,0): error IL2067: (NETCORE_ENGINEERING_TELEMETRY=Build) ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Reflection.RuntimeReflectionExtensions.GetRuntimeMethods(Type)'. The parameter 'type' of method 'ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
/_/src/xunit.execution/Extensions/ReflectionAbstractionExtensions.cs(92): Trim analysis error IL2067: ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Reflection.RuntimeReflectionExtensions.GetRuntimeMethods(Type)'. The parameter 'type' of method 'ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. [/__w/1/s/src/libraries/System.Runtime.Intrinsics/tests/System.Runtime.Intrinsics.Tests.csproj]
##[error]/_/src/xunit.execution/Extensions/ReflectionAbstractionExtensions.cs(92,0): error IL2067: (NETCORE_ENGINEERING_TELEMETRY=Build) ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo): 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Reflection.RuntimeReflectionExtensions.GetRuntimeMethods(Type)'. The parameter 'type' of method 'ReflectionAbstractionExtensions.GetMethodInfoFromIMethodInfo(Type,IMethodInfo)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

radical · 2022-08-13T16:46:13Z

/azp run runtime-extra-platforms,runtime-wasm

azure-pipelines · 2022-08-13T16:46:29Z

Azure Pipelines successfully started running 2 pipeline(s).

MichalStrehovsky · 2022-08-15T03:12:05Z

@MichalStrehovsky or @vitek-karas Any thoughts on the NativeAOT failures?

This is WarnAsError in action.

Apparently we deleted what it means to SuppressTrimAnalysisWarnings in dotnet/linker#2930 and NativeAOT no longer suppresses these. I've inlined what that means into the targets to work around and filed a 7.0 issue to fix it because this is customer-visible (#73926).

I'm puzzled why we're seeing it here because the break merged into linker repo 10 days ago and there's no way that made it into Preview 7 (in fact, troubleshooting this took longer than I'm willing to admit because the .targets in P7 SDK still have this suppression but we're picking it up from a NuGet instead).

cc @sbomer

MichalStrehovsky · 2022-08-15T06:52:18Z

The Linux_musl x64 Debug failure is #73930 (a test touched 4 hours ago, so I'm going with the "unrelated to what we're doing here" theory).

We're good to merge once llvmfullaot is done with its thing.

teo-tsirpanis · 2022-08-15T09:19:11Z

src/coreclr/tools/dotnet-pgo/SPGO/LbrEntry.cs

@@ -74,7 +74,7 @@ public unsafe struct LbrTraceEventData64
        public Span<LbrEntry64> Entries(int totalSize)
        {
            IntPtr entriesOffset = Unsafe.ByteOffset(ref Unsafe.As<LbrTraceEventData64, byte>(ref this), ref Unsafe.As<LbrEntry64, byte>(ref _entries));
-            return MemoryMarshal.CreateSpan(ref _entries, (totalSize - (int)entriesOffset) / sizeof(LbrEntry64));
+            return MemoryMarshal.CreateSpan(ref Unsafe.AsRef<LbrEntry64>(Unsafe.AsPointer(ref _entries)), (totalSize - (int)entriesOffset) / sizeof(LbrEntry64));


Won't that cause a GC hole? Same with above.

Good catch. We either need to make this a ref struct with a big "DO NOT CLEAN UP THE REF PART" comment, or use fixed to grab the pointer instead.

Yes it will. I'll push a fix.

We either need to make this a ref struct with a big "DO NOT CLEAN UP THE REF PART" comment, or use fixed to grab the pointer instead.

I think we can make these static functions that take the owning struct by ref.

Can't we make it

Suggested change

return MemoryMarshal.CreateSpan(ref Unsafe.AsRef<LbrEntry64>(Unsafe.AsPointer(ref _entries)), (totalSize - (int)entriesOffset) / sizeof(LbrEntry64));

return MemoryMarshal.CreateSpan(ref _entries, (totalSize - (int)entriesOffset) / sizeof(LbrEntry64));

and add [UnscopedRef] to the method?

Since I've also got the owner here, can we get this moved to build against the live build? Building against the repo's SDK is the root issue here.

I can look into it, I'm not totally sure on the constraints about that (we are publishing dotnet-pgo as a nuget package, would that be affected?). I probably need to involve @davidwrighton here.
I assume it can be done separately?

I assume it can be done separately?

Absolutely. My only concern is this project is the only one I found that seems to use the repo's SDK and could be heavily impacted by complex language changes that force updates to the public API surface. This is a longer-term concern and just something to keep in mind as the language evolves to have more of these low-level friendly features.

My only concern is this project is the only one I found that seems to use the repo's SDK and could be heavily impacted by complex language changes that force updates to the public API surface

Everything under src/coreclr/tools builds against the repo's SDK. Building against live bits has build sequencing issues where one wouldn't be able to build crossgen2 without building libs first, or run crossgen2 without building the host.

If we want to switch to live, I would want to know what the VS debug experience for live built things looks like. I would still prefer we don't do that because I kind of like I don't need to build coreclr VM, host, or other stuff for NativeAOT development.

Building against live bits has build sequencing issues

Ah. Thanks @MichalStrehovsky. This is an important consideration to note. Perhaps this wouldn't justify the resulting dev-loop pain.

There was no GC hole here in practice since the users have unmanaged pointers. However, the better fix is to just mark it as UnscopedRef.

radical · 2022-08-15T09:45:01Z

/azp run runtime-wasm

azure-pipelines · 2022-08-15T09:45:10Z

Azure Pipelines successfully started running 1 pipeline(s).

radical · 2022-08-15T10:19:24Z

@jakobbotsch build failed

jakobbotsch · 2022-08-15T10:25:15Z

@jakobbotsch build failed

Hmm strange, it builds fine from within VS locally. I suppose we cannot use this attribute yet in the project. Let me go back to my other fix.

radical · 2022-08-15T10:45:43Z

/azp run runtime-wasm

azure-pipelines · 2022-08-15T10:45:53Z

Azure Pipelines successfully started running 1 pipeline(s).

sbomer · 2022-08-15T12:48:17Z

I'm puzzled why we're seeing it here because the break merged into linker repo 10 days ago and there's no way that made it into Preview 7 (in fact, troubleshooting this took longer than I'm willing to admit because the .targets in P7 SDK still have this suppression but we're picking it up from a NuGet instead)

dotnet/linker#2837 moved the linker targets to the nuget package, and we are only now picking up the SDK change that imports them from the package. Now the recent targets changes are lighting up because runtime depends on a more recent nuget than the one that was shipped in-box with the P7 SDK.

radical · 2022-08-15T19:56:18Z

The wasm - AOT/EAT failures are present on main too, and are - #73961 .
The wasm debugger test failure is - #73957

jkoritzinsky · 2022-08-15T21:13:18Z

Linux x64 Debug leg failure is #73930

jkoritzinsky · 2022-08-15T21:13:34Z

All failures are accounted for. Merging this in.

ViktorHofer · 2022-08-15T21:18:11Z

Thanks for getting this in everyone. @jkoritzinsky as usual, can you please send out the notice to the team including instructions on how to obtain the new SDK (for global installs)?

jkoritzinsky · 2022-08-15T21:20:44Z

Will do!

Update runtime repo to use Preview 6 SDK

d27729f

ghost assigned agocke Jul 13, 2022

agocke added 3 commits July 13, 2022 18:28

Fix json encoding

f276db3

Use TrimMode=full instead of TrimmerDefaultAction

0c66891

Also set TrimMode=full in TrimmingTests props file

556bb8a

agocke marked this pull request as ready for review July 14, 2022 21:48

agocke requested a review from a team July 14, 2022 23:02

ViktorHofer approved these changes Jul 14, 2022

View reviewed changes

MichalStrehovsky reviewed Jul 15, 2022

View reviewed changes

agocke added 2 commits July 14, 2022 21:43

Add TrimmerDefaultAction for NativeAOT library testing

b40a841

Remove usage of default trim mode

5af1afa

Replace with TrimMode partial

e1b012a

agocke closed this Jul 15, 2022

agocke reopened this Jul 15, 2022

runfoapp bot mentioned this pull request Jul 15, 2022

System.Net.Http.Function.Tests and System.Net.Websockets.Client.Tests work items failing on runtime-extra-platforms Mono_Release runs #72209

Closed

Merge remote-tracking branch 'upstream/main' into runtime-p6

890efef

agocke force-pushed the runtime-p6 branch from 61a16fb to 890efef Compare July 15, 2022 22:15

AaronRobinsonMSFT added 2 commits August 13, 2022 08:04

Merge remote-tracking branch 'upstream/main' into runtime-p6

9d0508a

Merge in main and update pgo project

459a67a

Add ByRefFields feature flag.

79ff97d

jaredpar approved these changes Aug 13, 2022

View reviewed changes

MichalStrehovsky added 2 commits August 15, 2022 12:05

Inline deleted ILLink.targets logic

99af1bd

Cleanups

6fa5900

teo-tsirpanis reviewed Aug 15, 2022

View reviewed changes

Ensure no GC hole for LbrTraceEventData

cedada9

There was no GC hole here in practice since the users have unmanaged pointers. However, the better fix is to just mark it as UnscopedRef.

Use static workaround since UnscopedRef is not yet available

17a4e22

runfoapp bot mentioned this pull request Aug 15, 2022

Long Running Test: Interop/MonoAPI/MonoMono/PInvokeDetach/PInvokeDetach.sh #73040

Closed

jkoritzinsky merged commit e3b2af3 into dotnet:main Aug 15, 2022

carlossanlop mentioned this pull request Aug 17, 2022

Packages need to stabilize for shipping .NET 7 #72111

Closed

ghost locked as resolved and limited conversation to collaborators Sep 15, 2022

	return MemoryMarshal.CreateSpan(ref Unsafe.AsRef<LbrEntry64>(Unsafe.AsPointer(ref _entries)), (totalSize - (int)entriesOffset) / sizeof(LbrEntry64));
	return MemoryMarshal.CreateSpan(ref _entries, (totalSize - (int)entriesOffset) / sizeof(LbrEntry64));

Update runtime repo to use Preview 7 SDK #72145

Update runtime repo to use Preview 7 SDK #72145

Conversation

agocke commented Jul 13, 2022

dotnet-issue-labeler bot commented Jul 13, 2022

ViktorHofer left a comment

Choose a reason for hiding this comment

agocke commented Jul 14, 2022

MichalStrehovsky commented Jul 15, 2022

MichalStrehovsky commented Jul 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented Jul 15, 2022

agocke commented Jul 15, 2022

azure-pipelines bot commented Jul 15, 2022

MichalStrehovsky commented Jul 15, 2022

agocke commented Jul 15, 2022

azure-pipelines bot commented Jul 15, 2022

agocke commented Jul 15, 2022

azure-pipelines bot commented Jul 15, 2022

agocke commented Jul 16, 2022 • edited Loading

AaronRobinsonMSFT commented Aug 13, 2022

radical commented Aug 13, 2022

azure-pipelines bot commented Aug 13, 2022

MichalStrehovsky commented Aug 15, 2022 • edited Loading

MichalStrehovsky commented Aug 15, 2022

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

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

radical commented Aug 15, 2022

azure-pipelines bot commented Aug 15, 2022

radical commented Aug 15, 2022

jakobbotsch commented Aug 15, 2022

radical commented Aug 15, 2022

azure-pipelines bot commented Aug 15, 2022

sbomer commented Aug 15, 2022

radical commented Aug 15, 2022

jkoritzinsky commented Aug 15, 2022

jkoritzinsky commented Aug 15, 2022

ViktorHofer commented Aug 15, 2022

jkoritzinsky commented Aug 15, 2022

agocke commented Jul 16, 2022 •

edited

Loading

MichalStrehovsky commented Aug 15, 2022 •

edited

Loading