Skip to content

Conversation

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Nov 17, 2025

We're likely losing out on test coverage by not testing the recommended mainstream shipping configuration.

The test changes are all to make sure we don't trim what's needed.

Where I could, I avoided just putting TrimMode=full on the assembly (because that makes the test case RequiresProcessIsolation and that's not great). For some I had to give up because there was no clean way to do it.

Cc @dotnet/ilc-contrib

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 17, 2025
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky MichalStrehovsky added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 17, 2025
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Nov 17, 2025
Fixes the build crash in dotnet#121697.

Since with TrimMode=partial, we consider all static fields targets of reflection, we cannot allow inlining their contents. With full trimming allowed, the optimization is unlocked and we hit an assert when trying to nullcheck a constant.
MichalStrehovsky added a commit that referenced this pull request Nov 17, 2025
Fixes the build crash in #121697.

Since with TrimMode=partial, we consider all static fields targets of
reflection, we cannot allow inlining their contents. With full trimming
allowed, the optimization is unlocked and we hit an assert when trying
to nullcheck a constant.
@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

@copilot there are some tests that are failing due to trimming under src/tests/Loader/classloader/generics/Variance/IL. The failure is caused by removal of default constructor.

The failure in each of the tests is caused by:

	private static void UnboxUToTInternal<T,U>()
	{
		T t = (T) Activator.CreateInstance(typeof(U));
	}

This method is replicated in many tests.

To fix the failure the methods private static void UnboxUToTInternal<T,U>() need to be annotated like this: private static void UnboxUToTInternal<T, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] U>(). The namespace of the attribute is System.Diagnostics.CodeAnalysis.

The attribute may need to be propagated to each of the callsites too. For example:

	private static void CaseClassUToTWrapper<T,U>()
	{
		UnboxUToTInternal<T,U>();
	}

needs to be annotated as private static void CaseClassUToTWrapper<T, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] U>(). The annotation needs to be applied transitively until the context is no longer generic. Make this change to all tests under src/tests/Loader/classloader/generics/Variance/IL.

Copy link
Contributor

Copilot AI commented Nov 18, 2025

@MichalStrehovsky I've opened a new pull request, #121731, to work on those changes. Once the pull request is ready, I'll request review from you.

@MichalStrehovsky
Copy link
Member Author

@copilot for C# source files under src/tests/Loader/classloader/generics/Variance/IL, find the following method in each of the files: private static void UnboxUToTInternal<T,U>(). Add an attribute like this: private static void UnboxUToTInternal<T, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] U>(). The namespace of the attribute is System.Diagnostics.CodeAnalysis.

Copy link
Contributor

Copilot AI commented Nov 18, 2025

@MichalStrehovsky I've opened a new pull request, #121734, to work on those changes. Once the pull request is ready, I'll request review from you.

MichalStrehovsky added a commit that referenced this pull request Nov 19, 2025
* `dynamic` keyword is unsupported with AOT
* So are ALCs
* `GetInterfaceMap` API has known issues (the test actually passes right
now, but will fail with #121697 so just disabling now)
@MichalStrehovsky
Copy link
Member Author

@copilot for C# source files under src/tests/Loader/classloader/generics/Variance/IL, find the following method in each of the files: public static bool UIsInstT<T,U>(bool expected). Add an attribute like this: public static bool UIsInstT<T, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] U>(bool expected). The namespace of the attribute is System.Diagnostics.CodeAnalysis.

Copy link
Contributor

Copilot AI commented Nov 20, 2025

@MichalStrehovsky I've opened a new pull request, #121833, to work on those changes. Once the pull request is ready, I'll request review from you.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Nice!

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

/ba-g clang assert in a macos leg that doesn't exercise changes in this branch

@MichalStrehovsky MichalStrehovsky merged commit d01d30e into main Dec 3, 2025
94 of 102 checks passed
@MichalStrehovsky MichalStrehovsky deleted the MichalStrehovsky-patch-1 branch December 3, 2025 12:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enables testing with TrimMode=full to improve test coverage for the recommended production configuration. The changes ensure that code required by tests is not trimmed away by the compiler.

  • Adds trim-awareness attributes (DynamicallyAccessedMembers) to generic type parameters and method parameters that use reflection-based activation
  • Updates IL code to reference System.Runtime instead of mscorlib and use trimming-aware diagnostic APIs
  • Adjusts project files to use TrimMode=partial where necessary to work around issues while maintaining stricter trimming for the overall test tree

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/tests/Regressions/coreclr/20616/UnicodeBug.il Adds case-sensitive type lookups for trimming analysis compatibility
src/tests/Regressions/coreclr/15647/interfacestatics.ilproj Sets TrimMode=partial to work around runtime issue #122013
src/tests/Loader/classloader/regressions/dev10_720779/dev10_720779.il Updates assembly references and adds DynamicallyAccessedMembers attribute
src/tests/Loader/classloader/generics/Variance/IL/Unbox*.cs Adds DynamicallyAccessedMembers attributes for Activator.CreateInstance usage
src/tests/Loader/classloader/generics/Variance/IL/IsInst*.cs Adds DynamicallyAccessedMembers attributes for Activator.CreateInstance usage
src/tests/Loader/classloader/generics/Variance/IL/CastClass001.cs Adds DynamicallyAccessedMembers attributes for Activator.CreateInstance usage
src/tests/Loader/classloader/TypeForwarding/UnitTest/UnitTest.il Adds additional type instantiations for trimming preservation
src/tests/JIT/opt/Vectorization/UnrollEqualsStartsWith.csproj Sets TrimMode=partial for compatibility
src/tests/JIT/opt/Tailcall/TailcallVerifyWithPrefix.ilproj Sets TrimMode=partial for compatibility
src/tests/JIT/Regression/VS-ia64-JIT/V1.2-M01/b12425/b12425.il Adds ActiveIssue attribute and assembly references
src/tests/JIT/Regression/JitBlue/Runtime_61486/Runtime_61486.cs Uses typeof instead of GetType() to preserve type for trimming
src/tests/JIT/Generics/ConstrainedCall/class*.il Updates to System.Runtime references and adds DynamicallyAccessedMembers attributes
src/tests/JIT/Directed/forceinlining/*.il Updates to use DiagnosticMethodInfo API for trimming compatibility
src/tests/Directory.Build.targets Removes TrimMode=partial default, enabling TrimMode=full by default
src/tests/CoreMangLib/system/delegate/VSD/OpenDelegate.cs Adds DynamicallyAccessedMembers attribute for reflection usage

.method public hidebysig static string DelCall0(object inst, class [mscorlib]System.Type t, string mName) cil managed {
.method public hidebysig static string DelCall0(object inst, class [System.Runtime]System.Type t, string mName) cil managed {

.param [2]
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The magic number 08 00 00 00 in the custom attribute blob should be replaced with a named constant or commented to indicate it represents DynamicallyAccessedMemberTypes.PublicMethods.

Suggested change
.param [2]
.param [2]
// 08 00 00 00 == DynamicallyAccessedMemberTypes.PublicMethods

Copilot uses AI. Check for mistakes.
{
.param type T
.custom instance void [mscorlib]System.Runtime.CompilerServices.NewConstraintAttribute::.ctor() = ( 01 00 00 00 )
.custom instance void [System.Runtime]System.Runtime.CompilerServices.NewConstraintAttribute::.ctor() = ( 01 00 00 00 )
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The magic number 01 00 00 00 in the custom attribute blob should be replaced with a named constant or commented to indicate it represents DynamicallyAccessedMemberTypes.PublicParameterlessConstructor.

Suggested change
.custom instance void [System.Runtime]System.Runtime.CompilerServices.NewConstraintAttribute::.ctor() = ( 01 00 00 00 )
.custom instance void [System.Runtime]System.Runtime.CompilerServices.NewConstraintAttribute::.ctor() = ( 01 00 00 00 )
// 01 00 00 00 == DynamicallyAccessedMemberTypes.PublicParameterlessConstructor

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants