Skip to content

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

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.

Sign up for GitHub

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

Merged
layomia merged 6 commits into from
Jun 10, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e0d767
Use [SkipLocalsInit] & remove code for ILLink to strip locals init
layomia Jun 6, 2020
ba4e7d2
Address review feedback
layomia Jun 8, 2020
0e44a48
Simply condition
layomia Jun 9, 2020
37c23a6
Clean up
layomia Jun 9, 2020
21c3bcc
Fix condition
layomia Jun 9, 2020
0099ce0
Merge remote-tracking branch 'upstream/master' into skiplocalsinit
layomia Jun 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion eng/illink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,6 @@
<ILLink AssemblyPaths="$(ILLinkTrimInputAssembly)"
RootAssemblyNames=""
OutputDirectory="$(ILLinkTrimOutputPath)"
ClearInitLocals="$(ILLinkClearInitLocals)"
ExtraArgs="$(ILLinkArgs)"
ToolExe="$(_DotNetHostFileName)"
ToolPath="$(_DotNetHostDirectory)" />
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/src/System.Private.CoreLib/System.Private.CoreLib.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
<Configurations>Debug;Release;Checked</Configurations>
<Platforms>x64;x86;arm;arm64</Platforms>

<ILLinkClearInitLocals>true</ILLinkClearInitLocals>
<ILLinkTrimAssembly>true</ILLinkTrimAssembly>
<ILLinkTrimXml>$(IntermediateOutputPath)System.Private.CoreLib.xml</ILLinkTrimXml>
</PropertyGroup>
Expand Down
6 changes: 6 additions & 0 deletions src/libraries/Common/src/SkipLocalsInit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

// Used to indicate to the compiler that the .locals init flag should not be set in method headers.
[module: System.Runtime.CompilerServices.SkipLocalsInit]
3 changes: 0 additions & 3 deletions src/libraries/Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,6 @@

<PropertyGroup>
<EnableDefaultItems>false</EnableDefaultItems>

<!-- Clear the init locals flag on all src projects, except those in VB, where we can't use spans. -->
<ILLinkClearInitLocals Condition="'$(IsSourceProject)' == 'true' and '$(Language)' != 'VB'">true</ILLinkClearInitLocals>
</PropertyGroup>

<PropertyGroup Condition="'$(BuildAllConfigurations)' != 'true'">
Expand Down
19 changes: 19 additions & 0 deletions src/libraries/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,4 +239,23 @@
<None Include="$(MSBuildProjectDirectory)\**\*.cs"
Exclude="$(DefaultItemExcludes);$(DefaultExcludesInProjectFolder);@(Compile)" />
</ItemGroup>

<PropertyGroup>
<SkipLocalsInit Condition="'$(SkipLocalsInit)' == '' and '$(IsSourceProject)' == 'true' and '$(MSBuildProjectExtension)' == '.csproj' and '$(IsNETCoreApp)' == 'true' and '$(TargetFramework)' == '$(NetCoreAppCurrent)'">true</SkipLocalsInit>
</PropertyGroup>

<!--Instructs compiler not to emit .locals init, using SkipLocalsInitAttribute.-->
<Choose>
<When Condition="'$(SkipLocalsInit)' == 'true'">
<PropertyGroup >
<!-- This is needed to use the SkipLocalsInitAttribute. -->
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
Copy link
Member

@stephentoub stephentoub Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@joperezr joperezr Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

</PropertyGroup>

<ItemGroup>
<Compile Include="$(CommonPath)SkipLocalsInit.cs" Link="Common\SkipLocalsInit.cs" />
</ItemGroup>
</When>
</Choose>

</Project>
3 changes: 3 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1058,6 +1058,9 @@
<Compile Include="$(CommonPath)Interop\Windows\Normaliz\Interop.Normalization.cs">
<Link>Common\Interop\Windows\Normaliz\Interop.Normalization.cs</Link>
</Compile>
<Compile Include="$(CommonPath)SkipLocalsInit.cs">
<Link>Common\SkipLocalsInit.cs</Link>
</Compile>
<Compile Include="$(CommonPath)System\HResults.cs">
<Link>Common\System\HResults.cs</Link>
</Compile>
Expand Down
4 changes: 2 additions & 2 deletions ...ies/System.Private.CoreLib/src/System/Runtime/CompilerServices/SkipLocalsInitAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
namespace System.Runtime.CompilerServices
{
/// <summary>
/// Used to indicate to the compiler that the <c>.locals init</c> flag should
/// not be set in nested method headers when emitting to metadata.
/// Used to indicate to the compiler that the <c>.locals init</c>
/// flag should not be set in method headers.
/// </summary>
/// <remarks>
/// This attribute is unsafe because it may reveal uninitialized memory to
Expand Down
1 change: 0 additions & 1 deletion src/mono/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
<Configurations>Debug;Release;Checked</Configurations>
<Platforms>x64;x86;arm;arm64;wasm</Platforms>

<ILLinkClearInitLocals>true</ILLinkClearInitLocals>
<ILLinkTrimAssembly>true</ILLinkTrimAssembly>
<ILLinkTrimXml>$(MSBuildThisFileDirectory)src\LinkerDescriptor\System.Private.CoreLib.xml</ILLinkTrimXml>
</PropertyGroup>
Expand Down