search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

ilcompiler: System.Diagnostics.Debugger.IsSupported has already been added #86186

Closed OwnageIsMagic closed 1 year ago

OwnageIsMagic commented 1 year ago

Description

Error on dotnet publish -c Release if <DebuggerSupport>false</DebuggerSupport> and <PublishAot>true</PublishAot> specified in .csproj

EXEC : error : An item with the same key has already been added. Key: System.Diagnostics.Debugger.IsSupported [WorkerService1\WorkerService1.csproj]
  System.ArgumentException: An item with the same key has already been added. Key: System.Diagnostics.Debugger.IsSupported
     at System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException[T](T) + 0x14
     at System.Collections.Generic.Dictionary`2.TryInsert(TKey, TValue, InsertionBehavior) + 0x27f
     at System.Collections.Generic.Dictionary`2.AddRange(IEnumerable`1) + 0x13b
     at ILCompiler.FeatureSwitchManager..ctor(ILProvider, Logger, IEnumerable`1) + 0x37
     at ILCompiler.Program.Run() + 0xeb2
     at ILCompiler.ILCompilerRootCommand.<>c__DisplayClass203_0.<.ctor>b__0(InvocationContext context) + 0x20c

.nuget\packages\microsoft.dotnet.ilcompiler\8.0.0-preview.3.23174.8\build\Microsoft.NETCore.Native.targets(266,5): error MSB3073

Reproduction Steps

Expected behavior

Actual behavior

Regression?

No response

Known Workarounds

No response

Configuration

SDK: Version: 8.0.100-preview.3.23178.7 Commit: e300b0e1e6

Other information

No response

ghost commented 1 year ago

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

Issue Details
### Description Error on `dotnet publish -c Release` if `false` specified in `.csproj` ``` EXEC : error : An item with the same key has already been added. Key: System.Diagnostics.Debugger.IsSupported [WorkerService1\WorkerService1.csproj] System.ArgumentException: An item with the same key has already been added. Key: System.Diagnostics.Debugger.IsSupported at System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException[T](T) + 0x14 at System.Collections.Generic.Dictionary`2.TryInsert(TKey, TValue, InsertionBehavior) + 0x27f at System.Collections.Generic.Dictionary`2.AddRange(IEnumerable`1) + 0x13b at ILCompiler.FeatureSwitchManager..ctor(ILProvider, Logger, IEnumerable`1) + 0x37 at ILCompiler.Program.Run() + 0xeb2 at ILCompiler.ILCompilerRootCommand.<>c__DisplayClass203_0.<.ctor>b__0(InvocationContext context) + 0x20c .nuget\packages\microsoft.dotnet.ilcompiler\8.0.0-preview.3.23174.8\build\Microsoft.NETCore.Native.targets(266,5): error MSB3073 ``` ### Reproduction Steps ### Expected behavior ### Actual behavior ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration SDK: Version: 8.0.100-preview.3.23178.7 Commit: e300b0e1e6 ### Other information _No response_
Author: OwnageIsMagic
Assignees: -
Labels: `area-NativeAOT-coreclr`
Milestone: -
vitek-karas commented 1 year ago

This is caused by this line: https://github.com/dotnet/runtime/blob/4c0f2e7bd4cb86917e362052e0642df600984b6c/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets#L257

Basically there's a collision if a feature switch is specified by RuntimeHostConfigurationOption (and it has the Trim=true metadata) and there's also an ilc-specific property to set this - like IlcKeepManagedDebuggerSupport in this case.

In this case the SDK will always add --feature:System.Diagnostics.Debugger.IsSupported=false (because the IlcKeepManagedDebuggerSupport is not set at all). And so if it's also set via RuntimeHostConfigurationOption it will create a collision. In this case the DebuggerSupport is a public property which is mapped to RuntimeHostConfigurationOption item System.Diagnostics.Debugger.IsSupported (and trim enabled).

I think there are two problems to solve here:

ilc compiler fails if the same feature switch is specified twice on the command line

For comparison, illink will use the last value specified. It's unclear if this is intentional, or not. I can see arguments for both sides. But maybe for consistency reasons we should implement the same behavior as illink (take the last value on the command line). It's possible to get this with illink by manually modifying the RuntimeHostConfigurationOption item group (which is considered public), so it's a publicly observable behavior for illink. Note that solving this doesn't solve the problem below (it just makes it not fail, but maybe makes it worse by silently using only one of the two values).

The SDK targets don't handle any possible collision

between the built-in knobs which are passed as feature switches and the general feature switch mechanism through RuntimeHostConfigurationOption. I think we should change the behavior here and pipe all of these knobs through RuntimeHostConfigurationOption and have some mechanism to decide on override behavior. Specifically (for this example of DebuggerSupport):

@MichalStrehovsky @sbomer for ideas/opinions.

vitek-karas commented 1 year ago

@OwnageIsMagic as for your issue, setting DebuggerSupport=false is unnecessary for PublishAot=true as it will be defaulted to that value. So as a workaround you can leave DebuggerSupport unset in your project file (if you only needed it for AOT that is).

vitek-karas commented 1 year ago

Actually, I just realized that this is probably wrong:

PublishAot should imply DebuggerSupport=false, just like we default other feature switches for trimming

We probably don't want to break debugging features for normal builds (dotnet build) - which is unlike pretty much any other feature switch. So we will need to come up with some other solution to this.

sbomer commented 1 year ago

From https://github.com/dotnet/runtime/pull/82696#issuecomment-1447482346, IlcKeepManagedDebuggerSupport was added as an undocumented escape hatch, so I don't think we should lift it to RuntimeHostConfigurationOption.

For comparison, illink will use the last value specified. It's unclear if this is intentional, or not.

IIRC I did it this way simply to match the behavior of other illink command-line flags (like --action).

It's possible to get this with illink by manually modifying the RuntimeHostConfigurationOption item group (which is considered public), so it's a publicly observable behavior for illink.

I like this way of looking at it - we should decide what UX makes most sense and implement that approach in both tools. And probably extend it to the other command-line flags as well. I suppose last-wins makes it easier to override settings that are exposed as an MSBuild ItemGroup. So I would go with last-wins. Maybe conflicting settings should produce a warning (but redundant settings that agree should just work).

Unfortunately SDK currently seems to not prioritize RuntimeHostConfigurationOption over the public properties - it will blindly append to the list if DebuggerSupport property is set. Which is basically another vector how to get duplicates.

I think this is the same issue tracked by https://github.com/dotnet/sdk/issues/28568.

MichalStrehovsky commented 1 year ago

Not default-setting DebuggerSupport was intentional - https://github.com/dotnet/runtime/pull/82696#issue-1600407492. We don't want NativeAOT apps to be undebbugable by default. We just want to strip the CoreLib goo to support managed debuggers since there's no debugger that would take advantage of it anyway.

Native AOT currently doesn't support DebuggerSupport - if we wanted to implement this, we'd also want to set NativeDebugSymbols to false, for example. But all of this is pretty untested. So currently the best thing we could do is to turn DebuggerSupport=true into a no-op.

I think accepting the last specified value should be fine. I think all that's needed to fix this is to replace this:

https://github.com/dotnet/runtime/blob/42425673bf16345b1a876eab137093d140a67a16/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/FeatureSwitchManager.cs#L31

with a foreach that sets the values one-by-one instead of passing IEnumerable to the constructor.

I'm marking this up for grabs.

OwnageIsMagic commented 1 year ago

@vitek-karas I'm using this as workaround

<PropertyGroup>
  <PublishRelease>true</PublishRelease>
  <PublishAot>true</PublishAot>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)' == 'Release'">
  <!-- fails with nice error -->
  <PublishSingle Condition="'$(PublishAot)' != 'true'">true</PublishSingle>
  <!-- fails with stack tace, so I filed this issue. Docs say it disabled by default for AOT, but don't says it's mandotory to be not specified -->
  <DebuggerSupport Condition="'$(PublishAot)' != 'true'">false</DebuggerSupport>
  ...
</PropertyGroup>
filipnavara commented 1 year ago

This is probably still broken / regressed:

System.ArgumentException: An item with the same key has already been added. Key: System.Diagnostics.Debugger.IsSupported
   at System.ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException[T](T) + 0x11
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey, TValue, InsertionBehavior) + 0x28a
   at System.Collections.Generic.Dictionary`2.AddRange(IEnumerable`1) + 0x17b
   at ILCompiler.ManifestResourceBlockingPolicy..ctor(Logger, IEnumerable`1) + 0x66
   at ILCompiler.Program.Run() + 0x1bd9
   at ILCompiler.ILCompilerRootCommand.<>c__DisplayClass215_0.<.ctor>b__0(InvocationContext context) + 0x2f3

Ref: https://github.com/xamarin/xamarin-macios/pull/18515 ... or the SDK is not picking up new enough ILCompiler.

filipnavara commented 1 year ago

Yeah, it's still broken. It crashes before the FeatureSwitchHashtable constructor is called.

filipnavara commented 1 year ago

Fixed in https://github.com/dotnet/runtime/pull/88230.