[illink] Assert when running analysis on code produced by custom step for iOS, collision on IL offsets

[vitek-karas]

This method in a custom step: https://github.com/xamarin/xamarin-macios/blob/84cb6e794e315f2b037c35fc96efe36fc9d84f6b/tools/dotnet-linker/Steps/ManagedRegistrarStep.cs#L1333
adds code into the assembly via IL emitter. Later on we will run data flow analysis on the method body.
Whenever there's a call instruction we record it via TrimAnalysisMethodCallPattern. These patterns are stored in a dictionary in https://github.com/xamarin/xamarin-macios/blob/84cb6e794e315f2b037c35fc96efe36fc9d84f6b/tools/dotnet-linker/Steps/ManagedRegistrarStep.cs#L1333. The key to the dictionary is the MessageOrigin. The MessageOrigin identity is defined as Provider, ILOffset, ... (in this order). So for two calls from the same method the two origins will be identical except for the ILOffset.

The problem is, that IL emitter doesn't assign IL offsets to the generated instructions, so in the custom step generated body all instructions has offset of 0. Thus we end up in the situation where two different method calls have identical MessageOrigin.

This will eventually produce an assert in

runtime/src/tools/illink/src/linker/Linker.Dataflow/TrimAnalysisMethodCallPattern.cs

Line 47 in 373f048

Debug.Assert (Operation == other.Operation);

because the two patterns will be in the same dictionary slot, thus we will try to merge them, but they're for a different operation (method calls to different methods).

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/illink
See info in area-owners.md if you want to be subscribed.

[vitek-karas]

/cc @simonrozsival

I don't know what the best fix would be (other than removing custom steps :-)).
We could try to change MessageOrigin to store the actual instruction instance, not just offset, but I don't know how difficult it would be (and what would be the memory consumption). Alternatively we could assign some unique IL offsets when we generate the method in the custom step (this is fragile as it's easy to forget).

[vitek-karas]

This is ultimately also the cause of the exception in #110714 (we ignore the asserts in release and will try to merge argument lists with different number of arguments).

[vitek-karas]

xamarin/xamarin-macios@main...vitek-karas:xamarin-macios:ILOffsets contains the workaround for iOS custom steps. It generates unique IL offsets for all instructions in methods generated by the custom step.
I validated that it fixes the problem reported in #110714.

I'm curious what you think should be the fix: @simonrozsival , @sbomer

[vitek-karas]

Note that Android custom steps might have similar problem, but from a quick look they don't do so much code generation in the custom steps (but there is some of it happening).

[sbomer]

I'm curious what you think should be the fix: @simonrozsival , @sbomer

I think we should take your fix to the custom step. Generating IL with correct IL offsets seems like the safest way to ensure things don't go wrong downstream (short of removing/replacing the custom step).