Skip to content
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.

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

Trimmability and compiler optimization regression in FSharp.Core.dll with .NET 8.0.300 SDK #17381

Closed
mrvoorhe opened this issue Jul 3, 2024 · 5 comments

Comments

@mrvoorhe
Copy link

mrvoorhe commented Jul 3, 2024

Please provide a succinct description of the issue.

In .nuget\packages\fsharp.core\8.0.200\lib\netstandard2.0\FSharp.Core.dll there is a method Microsoft.FSharp.Core.PrintfImpl.FormatParser<TPrinter,TState,TResidue,TResult>.buildCaptureFunc

Inside that method there is reflection.

When you look at the compiled IL (I'm going to use decompiled C# from ILSpy since that's going to be easiest to read) the GetMethod calls look like

captureMethName = "CaptureFinal" + ((IFormattable)(object)captureCount).ToString(null, CultureInfo.InvariantCulture);
mi = typeof(Specializations<TState, TResidue, TResult>).GetMethod(captureMethName, BindingFlags.Static | BindingFlags.NonPublic);

And for context here is the source code.

let captureMethName = "CaptureFinal" + string captureCount
let mi = typeof<Specializations<'State, 'Residue, 'Result>>.GetMethod(captureMethName, AllStatics)

The key thing to note is that AllStatics is inlined. This inlining allows ILLink to detect the dependency and mark Specializations<TState, TResidue, TResult>.CaptureFinal1

So all is good with FSharp.Core.dll from 8.0.200.

Now to the problem, If you look at the same method in FSharp.Core.dll from 8.0.300 it looks different.

captureMethName = "CaptureFinal" + ((IFormattable)(object)captureCount).ToString(null, CultureInfo.InvariantCulture);
mi = typeof(Specializations<TState, TResidue, TResult>).GetMethod(captureMethName, AllStatics);

Notice that AllStatics has not been inlined. This leads to ILLink not being able to detect the dependency and leads to a MissingMethodException if this code path is hit on a trimmed assembly.

I dropped the following code into an F# library to play around with the behavior.

namespace ClassLibrary5

open System.Reflection

module Say =
    let private AllStatics = BindingFlags.Public ||| BindingFlags.NonPublic ||| BindingFlags.Static
    
    let private GetStaticMethods (t: System.Type) =
        t.GetMethods(AllStatics)
        |> Array.filter (fun m -> m.IsStatic)
        |> Array.map (fun m -> m.Name)

Using the 8.0.202 .NET SDK, when I compile in Debug, AllStatics is not inlined. When I compile in Release, AllStatics is inlined.

If I take that same repro, and compile it in Release using the 8.0.300 .NET SDK AllStatics is no longer inlined.

Expected behavior

AllStatics is inlined.

Actual behavior

AllStatis is not inlined.

Known workarounds

You could use a link.xml file to workaround the trimming problem.

@vzarytovskii
Copy link
Member

vzarytovskii commented Jul 3, 2024

It has changed in #15484, it's not inlined because inline logic has changed - it will no longer inline anything private.

It was changed back in #17189 (at least I believe it fixed it):

Here's result of building it with 9.0.100-preview.5:

internal static BindingFlags AllStatics
{
    [CompilerGenerated]
    [DebuggerNonUserCode]
    get
    {
        return BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic;
    }
}

internal static string[] GetStaticMethods(Type t)
{
    MethodInfo[] array = ArrayModule.Filter(GetStaticMethods@10.@_instance, t.GetMethods(BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic));
    if (array == null)
    {
        throw new ArgumentNullException("array");
    }
    string[] array2 = new string[array.Length];
    for (int i = 0; i < array2.Length; i++)
    {
        array2[i] = array[i].Name;
    }
    return array2;
}

It should also be in 8.0.400 (I am unsure about their release schedule).

@baronfel
Copy link
Member

baronfel commented Jul 3, 2024

8.0.400 won't release until August - but at this point any backport of this fix to the 8.0.3xx SDKs wouldn't be releasing until August's patch release anyway so there's no chance for this to be fixed sooner than that.

@mrvoorhe
Copy link
Author

mrvoorhe commented Jul 3, 2024

So it sounds like this issue will be fixed in an upcoming release?

@vzarytovskii
Copy link
Member

So it sounds like this issue will be fixed in an upcoming release?

Yeah, it appears so. If it won't, please bump the issue (or create a new one).

@mrvoorhe
Copy link
Author

mrvoorhe commented Jul 3, 2024

Great! I'll close this then. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants