Use [SkipLocalsInit] & remove code for ILLink to strip locals init by layomia · Pull Request #37541 · dotnet/runtime

layomia · 2020-06-06T10:43:39Z

Fixes #35527.

ghost · 2020-06-06T10:43:48Z

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

stephentoub

Thanks for working on this.

Have you confirmed .locals init is appropriately not being output?

src/coreclr/src/System.Private.CoreLib/src/ShouldSkipLocalsInit.cs

src/libraries/Directory.Build.props

stephentoub · 2020-06-06T11:29:54Z

src/libraries/Directory.Build.targets

+    <When Condition="'$(ShouldSkipLocalsInit)' == 'true' and '$(IsNETCoreApp)' == 'true'">
+      <PropertyGroup >
+        <!-- This is needed to use the SkipLocalsInitAttribute. -->
+        <AllowUnsafeBlocks>true</AllowUnsafeBlocks>


Can we remove this from some .csproj files as a result?

@layomia and I checked offline and we really only want to add this attribute to net5.0 configurations, as we were only using the linker to remove locals init for those configurations. I would totally agree with you in removing duplicate properties if all configurations applied, but because it is only netcoreappcurrent then I think it will be messy to have this property basically have conditions in projects that say '$(TargetFramework)' != '$(NetCoreAppCurrent)' so I would probably suggest to live this as is for now.

src/mono/netcore/System.Private.CoreLib/src/ShouldSkipLocalsInit.cs

src/libraries/Common/src/ShouldSkipLocalsInit.cs

src/libraries/Directory.Build.props

src/mono/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj

src/libraries/Directory.Build.targets

layomia · 2020-06-09T18:25:55Z

Have you confirmed .locals init is appropriately not being output?

Yes, I've double-checked with various assemblies.

src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems

src/libraries/Directory.Build.props

src/libraries/Directory.Build.targets

joperezr

This LGTM other than that one small comment about probably renaming the attribute file. Thanks for fixing this @layomia 😄

src/libraries/Directory.Build.targets

eerhardt

Looks great. Thanks for getting this done.

Use [SkipLocalsInit] & remove code for ILLink to strip locals init

3e0d767

layomia added this to the 5.0 milestone Jun 6, 2020

layomia requested review from eerhardt and joperezr June 6, 2020 10:43

layomia requested review from EgorBo, akoeplinger, marek-safar, steveisok and vargaz as code owners June 6, 2020 10:43

layomia self-assigned this Jun 6, 2020

Dotnet-GitSync-Bot added the area-Infrastructure label Jun 6, 2020

stephentoub reviewed Jun 6, 2020

View reviewed changes