Feature switch with ILLink substitution does not always trim branches

[eiriktsarpalis]

@eerhardt @vitek-karas for context to https://github.com/dotnet/runtime/pull/90013/files#r1284314974 adding this method call resulted in rooting the reflection resolver.

_Originally posted by @eiriktsarpalis in https://github.com/dotnet/runtime/pull/90013#discussion_r1284316652_

See https://github.com/dotnet/runtime/commit/b2ba402683d34c58cd486814904b6391ed954703 for a reproducing test.

[ghost]

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

Issue Details

---

> @eerhardt @vitek-karas for context to https://github.com/dotnet/runtime/pull/90013/files#r1284314974 adding this method call resulted in rooting the reflection resolver. _Originally posted by @eiriktsarpalis in https://github.com/dotnet/runtime/pull/90013#discussion_r1284316652_ See https://github.com/dotnet/runtime/commit/b2ba402683d34c58cd486814904b6391ed954703 for a reproducing test.

Author:	eiriktsarpalis
Assignees:	-
Labels:	`untriaged`, `area-Tools-ILLink`, `needs-area-label`
Milestone:	-

[vitek-karas]

I was able to get a repro from the old branch. The problem is this IL pattern:

    IL_0016: call bool System.Text.Json.JsonSerializer::get_IsReflectionEnabledByDefault()
    IL_001b: brfalse.s IL_0028   // The trimmer correctly recognizes this as a constant branch and will remove the _typeInfoResolver check below

    IL_001d: ldarg.0
    IL_001e: ldfld class System.Text.Json.Serialization.Metadata.IJsonTypeInfoResolver System.Text.Json.JsonSerializerOptions::_typeInfoResolver
    IL_0023: ldnull
    IL_0024: ceq
    IL_0026: br.s IL_0029  // This is the problem - we scan all the code even if we decided to not include this branch

    IL_0028: ldc.i4.0

    IL_0029: stloc.1    // Target of the jump from IL_0026
    IL_002a: ldloc.1
    IL_002b: brfalse.s IL_0039  // This should also be recognized as constant branch, but the trimmer gives up

    IL_002d: nop
    IL_002e: ldarg.1
    IL_002f: ldarg.0
    IL_0030: ldc.i4.1
    IL_0031: call class System.Text.Json.Serialization.JsonConverter System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver::GetConverterForType(class [System.Runtime]System.Type, class System.Text.Json.JsonSerializerOptions, bool)
    IL_0036: stloc.2
    IL_0037: br.s IL_0043

    IL_0039: ldarg.0

The trimmer will recognize the ldc.i4.0; stloc.1; ldloc.1; pattern and will understand that it's a constant 0, but it sees that one of the instructions in it is a jump target (from IL_0026), so it decides to not rewrite the condition - since it's not smart enough to also rewrite the jump (which would be really tricky).

Honestly, I don't think we should try to fix all of these cases. Mainly because:

• It's never ending, there's always going to be a case which breaks - unless we implement full code flow analysis with full data flow (like JIT has) which is very expensive and doesn't feel necessary
• It's very hard to guarantee the same behavior across the 3 tools (analyzer, illink, ilc)

This is basically another example why we need to define a simple short set of patterns which are recognized and stick to that. Ideally, we would have a way to tell the developer that the code strayed away from such patterns, but that pretty difficult to do. And this is all just for feature switches which are pretty rare (I know it doesn't seem like it, because in the BCL we use them quite often, but we're the exception here).

[eiriktsarpalis]

And this is all just for feature switches which are pretty rare (I know it doesn't seem like it, because in the BCL we use them quite often, but we're the exception here).

That's true, however JsonSerializer.IsReflectionEnabledByDefault is public API that we're explicitly documenting as being substituted by the linker. The idea is that library authors (including aspnetcore already) rely on this flag to avoid accidentally rooting the substantial STJ reflection components.

Perhaps the solution is to be more deliberate in documenting what types of branching patterns are "safe" in that regard.

[vitek-karas]

Perhaps the solution is to be more deliberate in documenting what types of branching patterns are "safe" in that regard.

Exactly - we just need to define that set first. So far it's been a "best effort" type of solution... obviously now we need something more defined.

[sbomer]

We now have a document describing the supported patterns. It mentions the pattern that initially prompted this issue unsupported for now: https://github.com/dotnet/runtime/blob/main/docs/design/tools/illink/feature-checks.md#boolean-andor, so I think we can close this.