ILLink: Stop giving special treatment to icalls #117419
Conversation
mrvoorhe
requested review from
marek-safar,
steveisok and
vitek-karas
as code owners
dotnet-policy-service
bot
added
linkable-framework
github-actions
bot
added
the
area-Tools-ILLink
|
Tagging subscribers to this area: @dotnet/illink |
There was a problem hiding this comment.
Pull Request Overview
This PR removes special handling for internal calls in the ILLink marking step, consolidates multiple specific interop tests into a single generic test, and updates related core library definitions and build settings to align with the new behavior.
- Drop
method.IsInternalCallbranch inMarkStep.csso only P/Invoke methods are treated as interop. - Remove several focused internal-call tests and introduce one
NoSpecialMarkingtest. - Add
FOR_ILLINKconstructors incorelib.h, remove unnecessary suppressions, and comment outPublishTrimmedin AOT project files until downstream toolchain updates.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/tools/illink/src/linker/Linker.Steps/MarkStep.cs | Only treat PInvokeImpl as interop, remove IsInternalCall case. |
| src/tools/illink/test/Mono.Linker.Tests.Cases/Interop/InternalCalls/*.cs | Delete specific internal‐call test cases. |
| src/tools/illink/test/Mono.Linker.Tests.Cases/Interop/InternalCalls/NoSpecialMarking.cs | Add a single generic test for internal‐call behavior. |
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/Interop/InternalCalls/InternalCallsTests.cs | Remove all specific [Fact] tests, leave only NoSpecialMarking. |
| src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.Mono.cs | Drop UnconditionalSuppressMessage on internal calls. |
| src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeEnumBuilder.Mono.cs | Drop UnconditionalSuppressMessage on internal calls. |
| src/coreclr/vm/corelib.h | Define .ctor methods under FOR_ILLINK for certain classes. |
| src/coreclr/tools/aot/crossgen2/crossgen2_inbuild.csproj | Comment out <PublishTrimmed> until toolchain update. |
| src/coreclr/tools/aot/ILCompiler/ILCompiler_inbuild.csproj | Comment out <PublishTrimmed> until toolchain update. |
Comments suppressed due to low confidence (1)
src/tools/illink/test/Mono.Linker.Tests.Cases/Interop/InternalCalls/NoSpecialMarking.cs:9
- [nitpick] Replacing multiple focused internal-call tests with a single generic one may reduce clarity and coverage of specific scenarios (e.g., default constructors, field retention, return-type constructors). Consider reintroducing targeted tests or expanding this test to validate each previously covered case.
public class NoSpecialMarking
| return RunTest(); | ||
| } | ||
| [Fact] | ||
| public Task NoSpecialMarking () |
There was a problem hiding this comment.
The test method NoSpecialMarking is missing the [Fact] attribute, so it won't be picked up by the test runner. Add [Fact] above the method to ensure it's executed.
|
@sbomer I cherry picked the tip commit from your comment #113704 (comment) but that led to a bunch of changed files that were unrelated. I closed the other PR because all the modified files added a bunch of reviewers. Here is a new branch rebased on the latest Sorry for the radio silence on this PR. The original issue was something I was trying to tackle on the side. Then this suggestion to change the icall behavior turned into a much larger distraction than I intended. I'm still pre-occupied with other priorities. I will get back to this at some point but if this is something you guys want cleaned up sooner than later I'd suggest taking over the effort. |
| MarkMarshalSpec(method.MethodReturnType, new DependencyInfo(DependencyKind.ReturnTypeMarshalSpec, method), methodOrigin); | ||
|
|
||
| if (method.IsPInvokeImpl || method.IsInternalCall) | ||
| if (method.IsPInvokeImpl) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
No problem. In that case, I might just convert this to draft. |
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
4 participants
As discussed #113437 (comment)
Note this is a reattempt of #113704