From 533f5ee72ea0499ff468588445655292e9aab135 Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 25 Jun 2020 10:06:43 -0700 Subject: [PATCH 1/9] Adding generation of substitution files for trimming out resources when feature switch is present --- eng/ILLink.Resources.Substitutions.template | 9 +++++ eng/illink.targets | 38 +++++++++++++++++++-- src/libraries/Common/src/System/SR.cs | 3 +- 3 files changed, 46 insertions(+), 4 deletions(-) create mode 100644 eng/ILLink.Resources.Substitutions.template diff --git a/eng/ILLink.Resources.Substitutions.template b/eng/ILLink.Resources.Substitutions.template new file mode 100644 index 00000000000000..18d9e57d21b89d --- /dev/null +++ b/eng/ILLink.Resources.Substitutions.template @@ -0,0 +1,9 @@ + + + + + + + + + \ No newline at end of file diff --git a/eng/illink.targets b/eng/illink.targets index 082d7e85c589ac..cabd4e1df9bb8a 100644 --- a/eng/illink.targets +++ b/eng/illink.targets @@ -40,7 +40,6 @@ $(MSBuildProjectDirectory)/ILLinkTrim_LibraryBuild.xml $(IntermediateOutputPath)ILLink.Descriptors.xml - $(MSBuildProjectDirectory)/ILLink.Substitutions.xml $(IntermediateOutputPath)ILLink.Substitutions.xml @@ -49,7 +48,6 @@ - @@ -105,8 +103,42 @@ + + + $(IntermediateOutputPath)ILLink.Resources.Substitutions.xml + + + + + + + + + + + $(MSBuildThisFileDirectory)ILLink.Resources.Substitutions.template + + + + + + + + + + diff --git a/src/libraries/Common/src/System/SR.cs b/src/libraries/Common/src/System/SR.cs index 69a233ed668e33..33ccfff0588a5e 100644 --- a/src/libraries/Common/src/System/SR.cs +++ b/src/libraries/Common/src/System/SR.cs @@ -17,7 +17,8 @@ internal partial class SR // could compile each module with a different setting for this. We want to make sure there's a consistent behavior // that doesn't depend on which native module this method got inlined into. [MethodImpl(MethodImplOptions.NoInlining)] - private static bool UsingResourceKeys() => false; + private static bool UsingResourceKeys() => + AppContext.TryGetSwitch("System.SR.UsingResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; internal static string GetResourceString(string resourceKey, string? defaultString = null) { From 2899fe46b62496791bc755327c6c98d3548bed71 Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 25 Jun 2020 11:54:30 -0700 Subject: [PATCH 2/9] Adding support for CoreLib as well and fix issue with SR returning empty string --- ...ns.template => ILLink.Substitutions.Resources.template} | 0 eng/illink.targets | 7 ++++--- .../src/System.Private.CoreLib.Shared.projitems | 2 ++ src/libraries/System.Private.CoreLib/src/System/SR.cs | 2 +- 4 files changed, 7 insertions(+), 4 deletions(-) rename eng/{ILLink.Resources.Substitutions.template => ILLink.Substitutions.Resources.template} (100%) diff --git a/eng/ILLink.Resources.Substitutions.template b/eng/ILLink.Substitutions.Resources.template similarity index 100% rename from eng/ILLink.Resources.Substitutions.template rename to eng/ILLink.Substitutions.Resources.template diff --git a/eng/illink.targets b/eng/illink.targets index cabd4e1df9bb8a..34ee550c6500d7 100644 --- a/eng/illink.targets +++ b/eng/illink.targets @@ -106,9 +106,10 @@ $(IntermediateOutputPath)ILLink.Resources.Substitutions.xml + true - + @@ -116,12 +117,12 @@ library so that if a consumer wants to run the linker they can specify a feature switch to strip out all resources from the assembly. --> - $(MSBuildThisFileDirectory)ILLink.Resources.Substitutions.template + $(MSBuildThisFileDirectory)ILLink.Substitutions.Resources.template true c5ed3c1d-b572-46f1-8f96-522a85ce1179 + System.Private.CoreLib.Strings + true diff --git a/src/libraries/System.Private.CoreLib/src/System/SR.cs b/src/libraries/System.Private.CoreLib/src/System/SR.cs index 73e7e0015526b4..74e3a47276ca93 100644 --- a/src/libraries/System.Private.CoreLib/src/System/SR.cs +++ b/src/libraries/System.Private.CoreLib/src/System/SR.cs @@ -21,7 +21,7 @@ internal static partial class SR // Needed for debugger integration internal static string GetResourceString(string resourceKey) { - return GetResourceString(resourceKey, string.Empty); + return GetResourceString(resourceKey, null); } private static string InternalGetResourceString(string key) From 9b493ee0cf0516799ddd2c202c5887e6fa6a1624 Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 25 Jun 2020 12:09:01 -0700 Subject: [PATCH 3/9] Cache result of AppContext Switch lookup --- src/libraries/Common/src/System/SR.cs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/libraries/Common/src/System/SR.cs b/src/libraries/Common/src/System/SR.cs index 33ccfff0588a5e..a24e7338620950 100644 --- a/src/libraries/Common/src/System/SR.cs +++ b/src/libraries/Common/src/System/SR.cs @@ -10,6 +10,9 @@ namespace System { internal partial class SR { + // cache result of AppContext Switch System.SR.UsingResourceKeys + private static bool? s_usingResourceKeys; + // This method is used to decide if we need to append the exception message parameters to the message when calling SR.Format. // by default it returns false. // Native code generators can replace the value this returns based on user input at the time of native code generation. @@ -17,8 +20,14 @@ internal partial class SR // could compile each module with a different setting for this. We want to make sure there's a consistent behavior // that doesn't depend on which native module this method got inlined into. [MethodImpl(MethodImplOptions.NoInlining)] - private static bool UsingResourceKeys() => - AppContext.TryGetSwitch("System.SR.UsingResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; + private static bool UsingResourceKeys() + { + if (s_usingResourceKeys == null) + { + s_usingResourceKeys = AppContext.TryGetSwitch("System.SR.UsingResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; + } + return s_usingResourceKeys.Value; + } internal static string GetResourceString(string resourceKey, string? defaultString = null) { From 35a4164ef4dc6b1c46aee45cc3d1d877ccf136be Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 25 Jun 2020 15:37:14 -0700 Subject: [PATCH 4/9] Fixing allConfigurations leg and addressing feedback --- eng/ILLink.Substitutions.Resources.template | 6 +++--- eng/illink.targets | 4 ++-- src/libraries/Common/src/System/SR.cs | 10 ++++++++-- .../src/System.Collections.Immutable.csproj | 3 +++ .../src/System.Diagnostics.DiagnosticSource.csproj | 3 +++ .../System.IO.Packaging/src/System.IO.Packaging.csproj | 1 + .../src/System.Threading.Channels.csproj | 1 + 7 files changed, 21 insertions(+), 7 deletions(-) diff --git a/eng/ILLink.Substitutions.Resources.template b/eng/ILLink.Substitutions.Resources.template index 18d9e57d21b89d..62428b0bb648fd 100644 --- a/eng/ILLink.Substitutions.Resources.template +++ b/eng/ILLink.Substitutions.Resources.template @@ -1,9 +1,9 @@ - - + + - + \ No newline at end of file diff --git a/eng/illink.targets b/eng/illink.targets index 34ee550c6500d7..b09d5c2fc8fe34 100644 --- a/eng/illink.targets +++ b/eng/illink.targets @@ -118,8 +118,8 @@ from the assembly. --> + Inputs="$(MSBuildProjectFullPath)" + Outputs="$(ILLinkResourcesSubstitutionIntermediatePath)"> $(MSBuildThisFileDirectory)ILLink.Substitutions.Resources.template diff --git a/src/libraries/Common/src/System/SR.cs b/src/libraries/Common/src/System/SR.cs index a24e7338620950..ace20cb905b4bd 100644 --- a/src/libraries/Common/src/System/SR.cs +++ b/src/libraries/Common/src/System/SR.cs @@ -10,8 +10,10 @@ namespace System { internal partial class SR { - // cache result of AppContext Switch System.SR.UsingResourceKeys +#if (!NETSTANDARD1_0 && !NETSTANDARD1_1 && !NET45) // AppContext is not supported on < NetStandard1.3 or < .NET Framework 4.5 so no need to cache the value. + // cache result of AppContext Switch System.Resources.UsingResourceKeys private static bool? s_usingResourceKeys; +#endif // This method is used to decide if we need to append the exception message parameters to the message when calling SR.Format. // by default it returns false. @@ -22,11 +24,15 @@ internal partial class SR [MethodImpl(MethodImplOptions.NoInlining)] private static bool UsingResourceKeys() { +#if (!NETSTANDARD1_0 && !NETSTANDARD1_1 && !NET45) // AppContext is not supported on < NetStandard1.3 or < .NET Framework 4.5 if (s_usingResourceKeys == null) { - s_usingResourceKeys = AppContext.TryGetSwitch("System.SR.UsingResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; + s_usingResourceKeys = AppContext.TryGetSwitch("System.Resources.UsingResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; } return s_usingResourceKeys.Value; +#else + return false; +#endif } internal static string GetResourceString(string resourceKey, string? defaultString = null) diff --git a/src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj b/src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj index a942bf3a506cda..09ff1a3d15361e 100644 --- a/src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj +++ b/src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj @@ -105,6 +105,9 @@ + + + diff --git a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj index 52f340c36ac42b..f18ad5915f4a5e 100644 --- a/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj +++ b/src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj @@ -90,6 +90,9 @@ + + + diff --git a/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj b/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj index a2452e457a281e..44240d763755f3 100644 --- a/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj +++ b/src/libraries/System.IO.Packaging/src/System.IO.Packaging.csproj @@ -11,6 +11,7 @@ true + diff --git a/src/libraries/System.Threading.Channels/src/System.Threading.Channels.csproj b/src/libraries/System.Threading.Channels/src/System.Threading.Channels.csproj index 998a54c36e3562..f3adbd227e09db 100644 --- a/src/libraries/System.Threading.Channels/src/System.Threading.Channels.csproj +++ b/src/libraries/System.Threading.Channels/src/System.Threading.Channels.csproj @@ -31,6 +31,7 @@ Link="Common\System\Collections\Concurrent\SingleProducerConsumerQueue.cs" /> + From 9e634e33dc852d50d82cc792efd832ff77b925ab Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 25 Jun 2020 17:03:38 -0700 Subject: [PATCH 5/9] Fixing thread safety issue by removing caching --- src/libraries/Common/src/System/SR.cs | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/libraries/Common/src/System/SR.cs b/src/libraries/Common/src/System/SR.cs index ace20cb905b4bd..c9c5b4267f41c1 100644 --- a/src/libraries/Common/src/System/SR.cs +++ b/src/libraries/Common/src/System/SR.cs @@ -10,11 +10,6 @@ namespace System { internal partial class SR { -#if (!NETSTANDARD1_0 && !NETSTANDARD1_1 && !NET45) // AppContext is not supported on < NetStandard1.3 or < .NET Framework 4.5 so no need to cache the value. - // cache result of AppContext Switch System.Resources.UsingResourceKeys - private static bool? s_usingResourceKeys; -#endif - // This method is used to decide if we need to append the exception message parameters to the message when calling SR.Format. // by default it returns false. // Native code generators can replace the value this returns based on user input at the time of native code generation. @@ -22,18 +17,12 @@ internal partial class SR // could compile each module with a different setting for this. We want to make sure there's a consistent behavior // that doesn't depend on which native module this method got inlined into. [MethodImpl(MethodImplOptions.NoInlining)] - private static bool UsingResourceKeys() - { + private static bool UsingResourceKeys() => #if (!NETSTANDARD1_0 && !NETSTANDARD1_1 && !NET45) // AppContext is not supported on < NetStandard1.3 or < .NET Framework 4.5 - if (s_usingResourceKeys == null) - { - s_usingResourceKeys = AppContext.TryGetSwitch("System.Resources.UsingResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; - } - return s_usingResourceKeys.Value; + AppContext.TryGetSwitch("System.Resources.UsingResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; #else - return false; + false; #endif - } internal static string GetResourceString(string resourceKey, string? defaultString = null) { From 576e14d050ab9f2d3c59e87b0966a9cd002c252f Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 25 Jun 2020 18:53:21 -0700 Subject: [PATCH 6/9] Addressing feedback --- eng/ILLink.Substitutions.Resources.template | 6 +++--- src/libraries/Common/src/System/SR.cs | 9 +++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/eng/ILLink.Substitutions.Resources.template b/eng/ILLink.Substitutions.Resources.template index 62428b0bb648fd..4baa06b1778c9f 100644 --- a/eng/ILLink.Substitutions.Resources.template +++ b/eng/ILLink.Substitutions.Resources.template @@ -1,9 +1,9 @@ - - + + - + \ No newline at end of file diff --git a/src/libraries/Common/src/System/SR.cs b/src/libraries/Common/src/System/SR.cs index c9c5b4267f41c1..748c94a84c2f9b 100644 --- a/src/libraries/Common/src/System/SR.cs +++ b/src/libraries/Common/src/System/SR.cs @@ -10,16 +10,21 @@ namespace System { internal partial class SR { +#if (!NETSTANDARD1_0 && !NETSTANDARD1_1 && !NET45) // AppContext is not supported on < NetStandard1.3 or < .NET Framework 4.5 + private static bool s_usingResourceKeys = AppContext.TryGetSwitch("System.Resources.UseResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; +#endif + // This method is used to decide if we need to append the exception message parameters to the message when calling SR.Format. - // by default it returns false. + // by default it returns the value of System.Resources.UseResourceKeys AppContext switch or false if not specified. // Native code generators can replace the value this returns based on user input at the time of native code generation. + // The Linker is also capable of replacing the value of this method when the application is being trimmed. // Marked as NoInlining because if this is used in an AoT compiled app that is not compiled into a single file, the user // could compile each module with a different setting for this. We want to make sure there's a consistent behavior // that doesn't depend on which native module this method got inlined into. [MethodImpl(MethodImplOptions.NoInlining)] private static bool UsingResourceKeys() => #if (!NETSTANDARD1_0 && !NETSTANDARD1_1 && !NET45) // AppContext is not supported on < NetStandard1.3 or < .NET Framework 4.5 - AppContext.TryGetSwitch("System.Resources.UsingResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; + s_usingResourceKeys; #else false; #endif From 507fb889deaae5a6970119b4e780157267c1a594 Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Thu, 25 Jun 2020 22:35:58 -0700 Subject: [PATCH 7/9] Apply suggestions from code review Co-authored-by: Jan Kotas --- src/libraries/Common/src/System/SR.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/libraries/Common/src/System/SR.cs b/src/libraries/Common/src/System/SR.cs index 748c94a84c2f9b..aaaee66b2b7dcd 100644 --- a/src/libraries/Common/src/System/SR.cs +++ b/src/libraries/Common/src/System/SR.cs @@ -11,17 +11,13 @@ namespace System internal partial class SR { #if (!NETSTANDARD1_0 && !NETSTANDARD1_1 && !NET45) // AppContext is not supported on < NetStandard1.3 or < .NET Framework 4.5 - private static bool s_usingResourceKeys = AppContext.TryGetSwitch("System.Resources.UseResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; + private static readonly bool s_usingResourceKeys = AppContext.TryGetSwitch("System.Resources.UseResourceKeys", out bool usingResourceKeys) ? usingResourceKeys : false; #endif // This method is used to decide if we need to append the exception message parameters to the message when calling SR.Format. // by default it returns the value of System.Resources.UseResourceKeys AppContext switch or false if not specified. // Native code generators can replace the value this returns based on user input at the time of native code generation. // The Linker is also capable of replacing the value of this method when the application is being trimmed. - // Marked as NoInlining because if this is used in an AoT compiled app that is not compiled into a single file, the user - // could compile each module with a different setting for this. We want to make sure there's a consistent behavior - // that doesn't depend on which native module this method got inlined into. - [MethodImpl(MethodImplOptions.NoInlining)] private static bool UsingResourceKeys() => #if (!NETSTANDARD1_0 && !NETSTANDARD1_1 && !NET45) // AppContext is not supported on < NetStandard1.3 or < .NET Framework 4.5 s_usingResourceKeys; From e3f19ac49657aa6495767beb94d22b8ecaa3a623 Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Mon, 29 Jun 2020 15:27:42 -0700 Subject: [PATCH 8/9] Addressing feedback and adding tests --- eng/ILLink.Substitutions.Resources.template | 11 ++-- eng/illink.targets | 1 - eng/testing/linker/project.csproj.template | 1 + eng/testing/linker/trimmingTests.targets | 3 +- src/libraries/Common/src/System/SR.cs | 4 +- .../tests/System.Runtime.Tests.csproj | 1 + .../tests/System/UseResourceKeysTest.cs | 50 +++++++++++++++++++ .../System.Runtime.TrimmingTests.proj | 5 ++ .../VerifyResourcesGetTrimmedTest.cs | 27 ++++++++++ 9 files changed, 94 insertions(+), 9 deletions(-) create mode 100644 src/libraries/System.Runtime/tests/System/UseResourceKeysTest.cs create mode 100644 src/libraries/System.Runtime/tests/TrimmingTests/VerifyResourcesGetTrimmedTest.cs diff --git a/eng/ILLink.Substitutions.Resources.template b/eng/ILLink.Substitutions.Resources.template index 4baa06b1778c9f..9381443ba3915b 100644 --- a/eng/ILLink.Substitutions.Resources.template +++ b/eng/ILLink.Substitutions.Resources.template @@ -1,9 +1,10 @@ - - - + + + + - + - \ No newline at end of file + diff --git a/eng/illink.targets b/eng/illink.targets index b09d5c2fc8fe34..5111008b6dbc5e 100644 --- a/eng/illink.targets +++ b/eng/illink.targets @@ -103,7 +103,6 @@ - $(IntermediateOutputPath)ILLink.Resources.Substitutions.xml true diff --git a/eng/testing/linker/project.csproj.template b/eng/testing/linker/project.csproj.template index 75c4bd26d5731c..ef63096e8019c0 100644 --- a/eng/testing/linker/project.csproj.template +++ b/eng/testing/linker/project.csproj.template @@ -6,6 +6,7 @@ {RuntimeIdentifier} {RuntimePackDir} {TargetingPackDir} + <_ExtraTrimmerArgs>{ExtraTrimmerArgs} $(_ExtraTrimmerArgs) /p:DebugType=Embedded + + + --feature System.Resources.UseSystemResourceKeys true + diff --git a/src/libraries/System.Runtime/tests/TrimmingTests/VerifyResourcesGetTrimmedTest.cs b/src/libraries/System.Runtime/tests/TrimmingTests/VerifyResourcesGetTrimmedTest.cs new file mode 100644 index 00000000000000..fd486aab4c9782 --- /dev/null +++ b/src/libraries/System.Runtime/tests/TrimmingTests/VerifyResourcesGetTrimmedTest.cs @@ -0,0 +1,27 @@ +using System; + +class Program +{ + static int Main(string[] args) + { + try + { + throw new AggregateException(); + } + catch (Exception e) + { + // When the Trimmer recieves the feature switch to use resource keys then exception + // messages shouldn't return the exception message resource, but instead the resource + // key. This test is passing in the feature switch so we make sure that the resources + // got trimmed correctly. + if (e.Message == "AggregateException_ctor_DefaultMessage") + { + return 100; + } + else + { + return -1; + } + } + } +} \ No newline at end of file From ccdf0a0d6c931b64bcb727d3c7316bd8f91df5ba Mon Sep 17 00:00:00 2001 From: Jose Perez Rodriguez Date: Tue, 30 Jun 2020 12:21:06 -0700 Subject: [PATCH 9/9] Fix typo --- eng/illink.targets | 1 - .../System.Runtime/tests/System/UseResourceKeysTest.cs | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/eng/illink.targets b/eng/illink.targets index 5111008b6dbc5e..fc38a10979f854 100644 --- a/eng/illink.targets +++ b/eng/illink.targets @@ -133,7 +133,6 @@ -