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

Multiple RuntimeHostConfigurationOption for the same switch #28568

Open
AustinWise opened this issue Oct 11, 2022 · 7 comments
Open

Multiple RuntimeHostConfigurationOption for the same switch #28568

AustinWise opened this issue Oct 11, 2022 · 7 comments
Milestone

Comments

@AustinWise
Copy link
Contributor

AustinWise commented Oct 11, 2022

Software Versions

I'm using .NET 7 RC1 on macOS ARM64.

Overview

Let's use the switch System.Threading.Thread.EnableAutoreleasePool for an example.

In the linker's Microsoft.NET.ILLink.targets file, it sets the property AutoreleasePoolSupport to true if it is not currently set. This will cause the SDK's Microsoft.NET.Sdk.targets file to create a RuntimeHostConfigurationOption item for System.Threading.Thread.EnableAutoreleasePool. If the user had already created their own System.Threading.Thread.EnableAutoreleasePool item for the System.Threading.Thread.EnableAutoreleasePool switch, they will now have multiple RuntimeHostConfigurationOption entries for the same switch. What finally ends up in the runtime config might not be what they expect.

This problem applies to other properties the linker sets, not just AutoreleasePoolSupport.

Reproduction Program

Use dotnet new console and then edit the csproj file to contain:

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

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net7.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PublishTrimmed>true</PublishTrimmed>
  </PropertyGroup>

  <ItemGroup>
    <RuntimeHostConfigurationOption Include="System.Threading.Thread.EnableAutoreleasePool" Value="true" Trim="true" />
  </ItemGroup>

  <Target Name="PrintStuff" AfterTargets="Publish">
    <Message Text="RuntimeHostConfigurationOption: %(RuntimeHostConfigurationOption.Identity)=%(RuntimeHostConfigurationOption.Value)" Importance="High" />
  </Target>

</Project>

Then run:

dotnet publish -r osx-arm64 --self-contained
cat bin/Debug/net7.0/osx-arm64/publish/*.runtimeconfig.json

Expected

The dotnet publish command prints a single entry for System.Threading.Thread.EnableAutoreleasePool and the mytest.runtimeconfig.json has the switch set to true.

Actual

Dotnet publish output

  mytest -> /Users/austin/src/mytest/bin/Debug/net7.0/osx-arm64/publish/
  RuntimeHostConfigurationOption: System.Threading.Thread.EnableAutoreleasePool=true
  RuntimeHostConfigurationOption: Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability=true
  RuntimeHostConfigurationOption: System.ComponentModel.TypeConverter.EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization=false
  RuntimeHostConfigurationOption: System.Resources.ResourceManager.AllowCustomResourceTypes=false
  RuntimeHostConfigurationOption: System.Runtime.InteropServices.BuiltInComInterop.IsSupported=false
  RuntimeHostConfigurationOption: System.Runtime.InteropServices.EnableConsumingManagedCodeFromNativeHosting=false
  RuntimeHostConfigurationOption: System.Runtime.InteropServices.EnableCppCLIHostActivation=false
  RuntimeHostConfigurationOption: System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization=false
  RuntimeHostConfigurationOption: System.StartupHookProvider.IsSupported=false
  RuntimeHostConfigurationOption: System.Threading.Thread.EnableAutoreleasePool=false
  RuntimeHostConfigurationOption: System.Text.Encoding.EnableUnsafeUTF7Encoding=false

Note that System.Threading.Thread.EnableAutoreleasePool appears multiple times, once true and once false.

Runtime config file:

{
  "runtimeOptions": {
    "tfm": "net7.0",
    "includedFrameworks": [
      {
        "name": "Microsoft.NETCore.App",
        "version": "7.0.0-rc.1.22426.10"
      }
    ],
    "configProperties": {
      "System.Threading.Thread.EnableAutoreleasePool": false,
      "Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability": true,
      "System.ComponentModel.TypeConverter.EnableUnsafeBinaryFormatterInDesigntimeLicenseContextSerialization": false,
      "System.Resources.ResourceManager.AllowCustomResourceTypes": false,
      "System.Runtime.InteropServices.BuiltInComInterop.IsSupported": false,
      "System.Runtime.InteropServices.EnableConsumingManagedCodeFromNativeHosting": false,
      "System.Runtime.InteropServices.EnableCppCLIHostActivation": false,
      "System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization": false,
      "System.StartupHookProvider.IsSupported": false,
      "System.Text.Encoding.EnableUnsafeUTF7Encoding": false
    }
  }
}

Note that System.Threading.Thread.EnableAutoreleasePool is false, despite what our project file specified.

Workaround

Use the AutoreleasePoolSupport property instead of the RuntimeHostConfigurationOption item.

Possible fixes

Perhaps the Microsoft.NET.ILLink.targets file should also check if the user has set a RuntimeHostConfigurationOption for the switch before setting the AutoreleasePoolSupport property.

Another possibility is the SDK's targets should check for existing RuntimeHostConfigurationOption item before setting it second time.

Or perhaps something should check for duplicate RuntimeHostConfigurationOption entries for the same switch.

For what it's worth, NativeAOT's ILC exits with a failure message when it see the same app context switch multiple times.

@AustinWise
Copy link
Contributor Author

I filed this in dotnet/linker, but maybe this is a dotnet/sdk problem?

@marek-safar
Copy link
Contributor

@vitek-karas @sbomer what do you think?

@sbomer
Copy link
Member

sbomer commented Oct 13, 2022

To me this looks like it falls out of the fact that the property feature switches take precedence over RuntimeHostConfigurationOption. For example, <ServerGarbageCollection>true</ServerGarbageCollection> wins over <RuntimeHostConfigurationOption Include="System.GC.Server" Value="false" />.

This happens because the RuntimeHostConfigurationOption itemgroup is appended to in SDK targets (after the project file) depending on the property setting, and the last options wins.

It would technically be possible to fix this in the linker targets (by checking for existing items in RuntimeHostConfigurationOption), but I think this better belongs in the SDK. Otherwise we would also have to fix it in other SDK targets that set default values for the feature switches, like in the Wasm targets.

I would argue that RuntimeHostConfigurationOption should win over the properties because it is a lower-level setting. Fixing that would also fix this issue.

@sbomer
Copy link
Member

sbomer commented Oct 13, 2022

Another point is that some of the feature switch properties have other behaviors in addition to setting RuntimeHostConfigurationOption: DebuggerSupport will control whether the trimmer removes symbols, but this isn't implied by RuntimeHostConfigurationOption.

The supported way to turn back on features which are by default off in trimmed apps is to use the MSBuild properties. Using RuntimeHostConfigurationOption might not do everything associated with the property, so it should be reserved for advanced use cases. But I do think ideally it would win over the property setting.

@sbomer sbomer transferred this issue from dotnet/linker Oct 13, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NetSDK untriaged Request triage from a team member labels Oct 13, 2022
@dsplaisted dsplaisted added the needs team triage Requires a full team discussion label Nov 9, 2022
@dsplaisted dsplaisted removed their assignment Nov 9, 2022
@dsplaisted dsplaisted removed the untriaged Request triage from a team member label Nov 9, 2022
@ghost
Copy link

ghost commented Nov 9, 2022

@dotnet/linker-contrib a new issue has been filed in the ILLink area, please triage

@marcpopMSFT marcpopMSFT removed needs team triage Requires a full team discussion Area-ILLink labels Nov 9, 2022
@marcpopMSFT
Copy link
Member

Potentially the SDK could throw an error when we detect duplicate values for the runtime config but that would be a "breaking change". Leave it to do Daniel if he wants to make that change but it doesn't seem high priority to me. A better solution is to detect that the value is already set and not overwrite the value from what the customer set.

@marcpopMSFT marcpopMSFT added this to the Backlog milestone Nov 9, 2022
@marcpopMSFT
Copy link
Member

@vitek-karas @sbomer moved this to the backlog (so lower priority). Please comment if you feel that we should try to solve this. Seems fairly niche.

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

No branches or pull requests

5 participants