Skip to content

ILLink: Stop giving special treatment to icalls #117419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mrvoorhe wants to merge 4 commits into from
Closed

ILLink: Stop giving special treatment to icalls #117419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3b15cd
ILLink: Stop giving special treatment to icalls
mrvoorhe Mar 19, 2025
abf5387
Add missing dependencies in corelib binder
jkotas Mar 20, 2025
c83b6fa
Attempted fixes
mrvoorhe Mar 21, 2025
5687eeb
Formatting fix
mrvoorhe Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/coreclr/tools/aot/ILCompiler/ILCompiler_inbuild.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
<RuntimeIdentifier>$(NETCoreSdkRuntimeIdentifier)</RuntimeIdentifier>
<PublishDir>$(RuntimeBinDir)$(BuildArchitecture)/ilc/</PublishDir>
<SelfContained>true</SelfContained>
<PublishTrimmed>true</PublishTrimmed>
<!-- Re-enable once we pickup toolchain with https://github.com/dotnet/runtime/pull/113704 -->
<!-- <PublishTrimmed>true</PublishTrimmed> -->
<!--
Publish with AOT on Windows as Single file doesn't work well when an app has to include DiaSymReader.Native.
Publish single-file elsewhere so we don't need to adjust our "AOT in build" logic to handle targeting the SDK RID.
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/tools/aot/crossgen2/crossgen2_inbuild.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
<RuntimeIdentifier>$(NETCoreSdkRuntimeIdentifier)</RuntimeIdentifier>
<PublishDir>$(RuntimeBinDir)$(BuildArchitecture)/crossgen2/</PublishDir>
<SelfContained>true</SelfContained>
<PublishTrimmed>true</PublishTrimmed>
<!-- Re-enable once we pickup toolchain with https://github.com/dotnet/runtime/pull/113704 -->
<!-- <PublishTrimmed>true</PublishTrimmed> -->
<!--
Publish with AOT on Windows as Single file doesn't work well when an app has to include DiaSymReader.Native.
Publish single-file elsewhere so we don't need to adjust our "AOT in build" logic to handle targeting the SDK RID.
Expand Down
9 changes: 9 additions & 0 deletions src/coreclr/vm/corelib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,9 @@ DEFINE_FIELD_U(m_value, ReflectMethodObject, m_pMD)
DEFINE_CLASS(STUBMETHODINFO, System, RuntimeMethodInfoStub)
DEFINE_FIELD(STUBMETHODINFO, HANDLE, m_value)
DEFINE_METHOD(STUBMETHODINFO, FROMPTR, FromPtr, SM_IntPtr_RetObj)
#ifdef FOR_ILLINK
DEFINE_METHOD(STUBMETHODINFO, CTOR, .ctor, IM_RetVoid)
#endif // FOR_ILLINK

DEFINE_CLASS(CONSTRUCTOR_INFO, Reflection, ConstructorInfo)

Expand Down Expand Up @@ -603,6 +606,9 @@ DEFINE_FIELD_U(m_runtimeType, ReflectModuleBaseObject, m_runtim
DEFINE_FIELD_U(m_runtimeAssembly, ReflectModuleBaseObject, m_runtimeAssembly)
DEFINE_FIELD_U(m_pData, ReflectModuleBaseObject, m_pData)
DEFINE_CLASS(MODULE, Reflection, RuntimeModule)
#ifdef FOR_ILLINK
DEFINE_METHOD(MODULE, CTOR, .ctor, IM_RetVoid)
#endif // FOR_ILLINK

DEFINE_CLASS(TYPE_BUILDER, ReflectionEmit, TypeBuilder)
DEFINE_CLASS(ENUM_BUILDER, ReflectionEmit, EnumBuilder)
Expand Down Expand Up @@ -668,6 +674,9 @@ DEFINE_CLASS(PARAMETER, Reflection, ParameterInfo)
DEFINE_CLASS(PARAMETER_MODIFIER, Reflection, ParameterModifier)

DEFINE_CLASS(POINTER, Reflection, Pointer)
#ifdef FOR_ILLINK
DEFINE_METHOD(POINTER, CTOR, .ctor, IM_RetVoid)
#endif // FOR_ILLINK

DEFINE_CLASS_U(Reflection, Pointer, ReflectionPointer)
DEFINE_FIELD_U(_ptr, ReflectionPointer, _ptr)
Expand Down
4 changes: 0 additions & 4 deletions src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeEnumBuilder.Mono.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,6 @@ public override Type GetEnumUnderlyingType()
return _underlyingType;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2110:ReflectionToDynamicallyAccessedMembers",
Justification = "For instance members with MethodImplOptions.InternalCall, the linker preserves all fields of the declaring type. " +
"The _tb field has DynamicallyAccessedMembersAttribute requirements, but the field access is safe because " +
"Reflection.Emit is not subject to trimming.")]
[MethodImplAttribute(MethodImplOptions.InternalCall)]
private extern void setup_enum_type(Type t);

Expand Down
4 changes: 0 additions & 4 deletions src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.Mono.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,10 +638,6 @@ protected override ConstructorBuilder DefineTypeInitializerCore()
null);
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2110:ReflectionToDynamicallyAccessedMembers",
Justification = "For instance member internal calls, the linker preserves all fields of the declaring type. " +
"The parent and created fields have DynamicallyAccessedMembersAttribute requirements, but creating the runtime class is safe " +
"because the annotations fully preserve the parent type, and the type created via Reflection.Emit is not subject to trimming.")]
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
[MethodImplAttribute(MethodImplOptions.InternalCall)]
private extern TypeInfo create_runtime_class();
Expand Down
2 changes: 1 addition & 1 deletion src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3406,7 +3406,7 @@ protected virtual void ProcessMethod(MethodDefinition method, in DependencyInfo
MarkCustomAttributes(method.MethodReturnType, new DependencyInfo(DependencyKind.ReturnTypeAttribute, method), methodOrigin);
MarkMarshalSpec(method.MethodReturnType, new DependencyInfo(DependencyKind.ReturnTypeMarshalSpec, method), methodOrigin);

if (method.IsPInvokeImpl || method.IsInternalCall)
if (method.IsPInvokeImpl)
Copy link
Member

@jtschuster jtschuster Jul 8, 2025

Choose a reason for hiding this comment

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

Won't we miss marking the return type (and ref and out parameters) as instantiated if we don't call ProcessInteropMethod? We might still need a call to MarkRequirementsForInstantiatedTypes, even if we don't need all the other parts of ProcessInteropMethod.

Or it might make more sense to replace the calls to MarkDefaultConstructor in ProcessInteropMethod with MarkRequirementsForInstantiatedTypes.

Copy link
Contributor Author

@mrvoorhe mrvoorhe Jul 9, 2025

Choose a reason for hiding this comment

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

That was the route I originally wanted to go #113437 but I got push back. The consensus was that ILLink shouldn't speculate about dependencies and therefore the current speculation should actually be removed. I agree not speculating is more consistent with the rest of ILLink, but this is a long standing speculation that ILLink has been making for code and removing it didn't go smoothly. Which is why this PR is still hanging around.

Copy link
Member

@jtschuster jtschuster Jul 9, 2025

Choose a reason for hiding this comment

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

So if my understanding is correct (based on #113437 (comment)), the goal is to avoid special handling for internal calls and expect DynamicDependencyAttributes or xml descriptors to keep what's necessary?

Copy link
Contributor Author

@mrvoorhe mrvoorhe Jul 9, 2025

Choose a reason for hiding this comment

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

So if my understanding is correct (based on #113437 (comment)), the goal is to avoid special handling for internal calls and expect DynamicDependencyAttributes or xml descriptors to keep what's necessary?

Yes.

{
ProcessInteropMethod(method, methodOrigin);
}
Expand Down
36 changes: 3 additions & 33 deletions ...tools/illink/test/ILLink.RoslynAnalyzer.Tests/Interop/InternalCalls/InternalCallsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,39 +11,9 @@ public sealed class InternalCallsTests : LinkerTestBase
protected override string TestSuiteName => "Interop/InternalCalls";

[Fact]
public Task UnusedDefaultConstructorIsRemoved()
public Task NoSpecialMarking ()
{
return RunTest();
}

[Fact]
public Task UnusedFieldsOfTypesAreNotRemoved()
{
return RunTest();
}

[Fact]
public Task UnusedFieldsOfTypesWhenHasThisAreNotRemoved()
{
return RunTest();
}

[Fact]
public Task DefaultConstructorOfReturnTypeIsNotRemoved()
{
return RunTest();
}

[Fact]
public Task UnusedDefaultConstructorOfTypePassedByRefIsNotRemoved()
{
return RunTest();
}

[Fact]
public Task UnusedFieldsOfTypesPassedByRefAreNotRemoved()
{
return RunTest();
return RunTest ();
}
}
}
}
25 changes: 0 additions & 25 deletions ...no.Linker.Tests.Cases/Interop/InternalCalls/DefaultConstructorOfReturnTypeIsNotRemoved.cs
View file
Open in desktop

This file was deleted.

36 changes: 36 additions & 0 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/Interop/InternalCalls/NoSpecialMarking.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Runtime.CompilerServices;
using Mono.Linker.Tests.Cases.Expectations.Assertions;

namespace Mono.Linker.Tests.Cases.Interop.InternalCalls;

public class NoSpecialMarking
{
public static void Main ()
{
A a = null;
SomeMethod (ref a, null, null);
}

[Kept]
class A;

[Kept]
class B;

[Kept]
class C;

[Kept]
class D
{
int field1;
int field2;
}

[Kept]
[MethodImpl (MethodImplOptions.InternalCall)]
static extern C SomeMethod (ref A a, B b, D d);
}
30 changes: 0 additions & 30 deletions ...k/test/Mono.Linker.Tests.Cases/Interop/InternalCalls/UnusedDefaultConstructorIsRemoved.cs
View file
Open in desktop

This file was deleted.

31 changes: 0 additions & 31 deletions ...ests.Cases/Interop/InternalCalls/UnusedDefaultConstructorOfTypePassedByRefIsNotRemoved.cs
View file
Open in desktop

This file was deleted.

26 changes: 0 additions & 26 deletions ...nk/test/Mono.Linker.Tests.Cases/Interop/InternalCalls/UnusedFieldsOfTypesAreNotRemoved.cs
View file
Open in desktop

This file was deleted.

26 changes: 0 additions & 26 deletions ...o.Linker.Tests.Cases/Interop/InternalCalls/UnusedFieldsOfTypesPassedByRefAreNotRemoved.cs
View file
Open in desktop

This file was deleted.

24 changes: 0 additions & 24 deletions ...o.Linker.Tests.Cases/Interop/InternalCalls/UnusedFieldsOfTypesWhenHasThisAreNotRemoved.cs
View file
Open in desktop

This file was deleted.

Loading