ILLink: ArgumentOutOfRangeException in ILProcessor

vitek-karas commented 11 months ago

Repro:

dotnet new console

app.csproj:

<Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
        <PublishTrimmed>true</PublishTrimmed>
        <WarningsAsErrors>false</WarningsAsErrors>
        <NoWarn>NU1605;NU1603;NU1701</NoWarn>
    </PropertyGroup>

    <ItemGroup>
        <TrimmerRootAssembly Include="Kodeo.Doctum.Test" />

        <PackageReference Include="Kodeo.Doctum.Test" Version="6.0.0-beta008" />
    </ItemGroup>
</Project>

dotnet publish

Stack trace:

  Unhandled exception. System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Pa
  rameter 'target')
     at Mono.Cecil.Cil.ILProcessor.InsertAfter(Instruction target, Instruction instruction)
     at Mono.Linker.LinkerILProcessor.InsertAfter(Instruction target, Instruction instruction)
     at Mono.Linker.Steps.MarkStep.ProcessPendingTypeChecks()
     at Mono.Linker.Steps.MarkStep.Process()

/cc @LakshanF

ghost commented 11 months ago

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas See info in area-owners.md if you want to be subscribed.

Issue Details

Repro: `dotnet new console` `app.csproj`: ```xml Exe net8.0 enable enable true false NU1605;NU1603;NU1701 ``` `dotnet publish` Stack trace: ``` Unhandled exception. System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Pa rameter 'target') at Mono.Cecil.Cil.ILProcessor.InsertAfter(Instruction target, Instruction instruction) at Mono.Linker.LinkerILProcessor.InsertAfter(Instruction target, Instruction instruction) at Mono.Linker.Steps.MarkStep.ProcessPendingTypeChecks() at Mono.Linker.Steps.MarkStep.Process() ``` /cc @LakshanF

Author:	vitek-karas
Assignees:	-
Labels:	`area-Tools-ILLink`
Milestone:	-

sbomer commented 3 months ago

In Debug builds, this repro hits an assert:

Process terminated. Assertion failed.
   at Mono.Linker.Dataflow.CompilerGeneratedCallGraph.TrackCall(MethodDefinition fromMethod, TypeDefinition toType) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker.Dataflow/CompilerGeneratedCallGraph.cs:line 33
   at Mono.Linker.Dataflow.CompilerGeneratedState.<GetCompilerGeneratedStateForType>g__ProcessMethod|11_0(MethodDefinition method, <>c__DisplayClass11_0&) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker.Dataflow/CompilerGeneratedState.cs:line 212
   at Mono.Linker.Dataflow.CompilerGeneratedState.GetCompilerGeneratedStateForType(TypeDefinition type) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker.Dataflow/CompilerGeneratedState.cs:line 226
   at Mono.Linker.Dataflow.CompilerGeneratedState.TryGetCompilerGeneratedCalleesForUserMethod(MethodDefinition method, List`1& callees) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker.Dataflow/CompilerGeneratedState.cs:line 435
   at Mono.Linker.Steps.MarkStep.MarkReflectionLikeDependencies(MethodIL methodIL, Boolean requiresReflectionMethodBodyScanner, MessageOrigin origin) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs:line 3786
   at Mono.Linker.Steps.MarkStep.MarkMethodBody(MethodBody body, MessageOrigin origin) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs:line 3549
   at Mono.Linker.Steps.MarkStep.ProcessMethod(MethodDefinition method, DependencyInfo& reason) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs:line 3221
   at Mono.Linker.Steps.MarkStep.MethodDefinitionNode.GetStaticDependencies(NodeFactory context) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker.Steps/MarkStep.MethodDefinitionNode.cs:line 34
   at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.GetStaticDependenciesImpl(DependencyNodeCore`1 node) in /home/svbomer/src/runtime/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/DependencyAnalyzer.cs:line 182
   at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.GetStaticDependencies(DependencyNodeCore`1 node) in /home/svbomer/src/runtime/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/DependencyAnalyzer.cs:line 222
   at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ProcessMarkStack() in /home/svbomer/src/runtime/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/DependencyAnalyzer.cs:line 257
   at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeMarkedNodes() in /home/svbomer/src/runtime/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/DependencyAnalyzer.cs:line 308
   at Mono.Linker.Steps.MarkStep.Process() in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs:line 369
   at Mono.Linker.Steps.MarkStep.Process(LinkContext context) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs:line 243
   at Mono.Linker.Pipeline.ProcessStep(LinkContext context, IStep step) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker/Pipeline.cs:line 178
   at Mono.Linker.Pipeline.Process(LinkContext context) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker/Pipeline.cs:line 167
   at Mono.Linker.Driver.Run(ILogger customLogger) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker/Driver.cs:line 843
   at Mono.Linker.Driver.Main(String[] args) in /home/svbomer/src/runtime/src/tools/illink/src/linker/Linker/Driver.cs:line 68

It's asserting because there's an async state machine type that doesn't match the expected compiler-generated names. The method

System.Void Kodeo.Doctum.CodeCheck::Requires(System.Threading.Tasks.Task`1<System.Boolean>,System.String)

has an AsyncStateMachineAttribute that points to the type Kodeo.Doctum.CodeCheck/c.

sbomer commented 3 months ago

Found the problem:

There's a compiler-generated state machine method System.Void Kodeo.Doctum.EntityFramework.Extensions/g::a(), which is the MoveNext of an async state machine.
By design, MarkAndCheckRequiresReflectionMethodBodyScanner may be called multiple times for compiler-generated methods: https://github.com/dotnet/runtime/blob/2b5c5422b8359716fc0d5766487ea1a6527d7472/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs#L3597-L3601 (In this case once when it's analyzed as part of user code, and once as an override of IAsyncStateMachine::MoveNext.)
We normally cache the result to make MarkAndCheckRequiresReflectionMethodBodyScanner idempotent. The caching only happens for compiler-generated methods: https://github.com/dotnet/runtime/blob/2b5c5422b8359716fc0d5766487ea1a6527d7472/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs#L3618-L3619
Since g::a() doesn't match the compiler-generated names we look for, it's not cached: https://github.com/dotnet/runtime/blob/2b5c5422b8359716fc0d5766487ea1a6527d7472/src/tools/illink/src/linker/Linker.Dataflow/CompilerGeneratedNames.cs#L23-L35
As a result, we process instructions of this method multiple times. We double-track some isinst instructions that we can potentially remove, and later try to remove them twice from the same method, causing the crash.

The fix will be to somehow make MarkAndCheckRequiresReflectionMethodBodyScanner idempotent for the method in question, despite it not matching the expected name mangling scheme.

vitek-karas commented 3 months ago

That looks like obfuscated code - if that's the case we should probably close it. None of our compiler generated code logic will work when it's renamed/obfuscated.

dotnet / runtime

ILLink: ArgumentOutOfRangeException in ILProcessor #93802