Linker appears to incorrectly trim static field initialization logic

[tannergooding]

The following simple program fails with a segmentation fault:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <InvariantGlobalization>false</InvariantGlobalization>
  </PropertyGroup>

</Project>

using System;

namespace ConsoleApp7
{
    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine(DateTime.Now);
        }
    }
}

$ dotnet publish -c Release -r linux-x64 -p:PublishTrimmed=true -p:TrimMode=link
$ ./bin/Release/net6.0/linux-x64/publish/ConsoleApp7
Segmentation fault (core dumped)

This appears to be because System.Globalization.GlobalizationMode is trimmed down to just the following:

namespace System.Globalization
{
  internal static class GlobalizationMode
  {
    internal static bool Invariant => false;

    internal static bool UseNls => false;
  }
}

The original file is here: https://source.dot.net/#System.Private.CoreLib/GlobalizationMode.Unix.cs,f6b65aaea1440971

As you can see, the Invariant property has a field initializer which is getting trimmed where it should not be as it has necessary side effects. My guess is that the backing field that gets generated for such logic is getting trimmed because it is private and not returned anywhere: (https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0ATEBqAPgAQCYBGAWACh8BmAAn2KTsJoGEaBvCm7mgSwDsALjCj8AhgBs6DGsAgQpASX4A3MVF5ihHGgHMYggNw0AvjQC8NAOIGrEiMEm8AXmMG8I/ZWo1bBAWQgMGAAKAEpDLh5oqO4ABw01YWlGOQVrW3tHCRc3Dy9VdU0hQODw2I4K6PwAdhoAM0kAZxhI8miTChMgA=)

[vitek-karas]

I think this is a problem with how the substitutions are setup in CoreLib. The substitution is defined like this:

    <type fullname="System.Globalization.GlobalizationMode">
      <method signature="System.Boolean get_Invariant()" body="stub" value="true" feature="System.Globalization.Invariant" featurevalue="true" />
      <method signature="System.Boolean get_Invariant()" body="stub" value="false" feature="System.Globalization.Invariant" featurevalue="false" />
    </type>

Linker will blindly do this - so if the feature is disabled it will replace the method body of the get_Invariant with return false - no questions asked. If we still need to call the GetGlobalizationInvariantMode, then we will have to somehow change the code and substitutions so that it's still called but the get_Invariant is still "known" to return false.

[tannergooding]

Linker will blindly do this - so if the feature is disabled it will replace the method body of the get_Invariant with return false - no questions asked

That shouldn't be an issue, the initialization logic isn't part of the method body, it is part of the static constructor (as in .cctor). The linker is incorrectly trimming the .cctor when the method body of get_Invariant is replaced.

[sbomer]

The type is beforefieldinit - my understanding is that this only guarantees that the .cctor is executed at or before the first static field access. If with the substitution, the generated private field is never accessed, removing the .cctor seems like a safe optimization.

[tannergooding]

That might be the case, in which there likely needs to be something else that ensures the relevant initializer is still called.

In which case, is this a scenario the linker can/should warn about? It seems like an easy pit of failure, particularly that setting a method body to return x causes downstream side effects related to static initializers.

[MichalStrehovsky]

The substituted method can be side effecting in many ways and most of the time it's exactly what people want to get rid of (e.g. there could be side effects in the code called from the method). How should a warning for that look like and how far would we go with the warnings?

This specific example in CoreLib is weird because CoreLib dug out its own pit of failure - the eliminated cctor is actually guarding a different part of the codebase that maybe should have had its own cctor instead.

[tannergooding]

The substituted method can be side effecting in many ways and most of the time it's exactly what people want to get rid

I 100% agree. However, the difference here is that the substituted property also gets rid of a related field initializer that is not part of the method being substituted and which may be non-obvious to consumers.

How should a warning for that look like and how far would we go with the warnings?

I'd consider this a case where, based on the questions we get in dotnet/runtime and dotnet/roslyn related to static initializers, that a warning is likely worthwhile because the behavior isn't necessarily well understood.

This may even be a case where more experienced users misinterpret how this will function based on their understanding of JIT optimizations. For example, it would be completely reasonable to assume that setting a property to return false sets the backing static readonly field and therefore that the .cctor and associated initialization logic remains. Under the JIT, this would still generate correctly optimized code with the relevant dead code elimination, etc.

I would think just a simple warning that the method being substituted caused a static field initializer to be trimmed seems like a reasonable thing to do. The user could then annotate in the ILLink.Descriptors.xml that this correct/expected.

This specific example in CoreLib is weird because CoreLib dug out its own pit of failure - the eliminated cctor is actually guarding a different part of the codebase that maybe should have had its own cctor instead.

This is fairly standard practice in .NET and I would not consider doing it a pit of failure. The pit of failure, IMO, is that the linker has this separate substitution concept that is not linked to what the rest of .NET uses and so assumptions on how it works are new and different and do not mesh well with existing .NET code.

That is, we aren't telling the linker to substitute the "System.Globalization.Invariant" app config switch nor the DOTNET_SYSTEM_GLOBALIZATION_INVARIANT environment variable (both of which correctly integrate with all existing IL, which have been tested, and which the JIT correctly optimizes to a constant). Instead, the linker fully substitutes the method body using this new mechanism and while likely useful for some scenarios, represents modifications that are not tested, that may not be accounted for, and which may cause undesired downstream side effects.