Localsinit #24723

t-camaia · 2018-02-09T02:42:31Z

SkipLocalsInitAttribute can be used on methods to avoid locals initialization.

While decoding Well Known Attributes, SourceMemberMethodSymbol will clear the localsAreZeroed flag if SkipLocalsInitAttribute is found. This flag will be passed later to the corresponding ILBuilder and MethodBody.

The current ILVisualizer completely ignores the localsAreZeroed flag and always initializes locals (see dotnet/metadata-tools#11 and dotnet/metadata-tools#12). The tests will certainly fail until the changes are effected.

Some test scenarios were added in order to assure the expected behavior.

…ly working on method declarations

VSadov · 2018-02-09T19:18:12Z

src/Compilers/Core/Portable/CodeGen/MethodBody.cs

@@ -102,7 +105,7 @@ private static ImmutableArray<Cci.SequencePoint> GetSequencePoints(SequencePoint

        ImmutableArray<Cci.ExceptionHandlerRegion> Cci.IMethodBody.ExceptionRegions => _exceptionHandlers;

-        bool Cci.IMethodBody.LocalsAreZeroed => true;
+        bool Cci.IMethodBody.LocalsAreZeroed => _localsAreZeroed;


Now that this may vary, it should be another key in the method body cache. Just having same IL is not enough to say that methods may share the body.
Look at SerializeMethodBody

I wonder how this could be tested. We need to make sure the caching is not interfering.

Perhaps we can use code that relies on the zero init happening or not happening (probably using stackalloc or "out" parameters in VB), then we may just run bunch of similar methods that only differ in this bit and see if results are as expected?

In reply to: 167321124 [](ancestors = 167321124)

This is a good point, but I haven't tested to see whether it's likely that you get non-zero memory when running stackalloc. Our tests would have a dependency on having non-zero random memory, wouldn't they?

I was talking to @agocke about this. We use a body from the cache if we can guarantee that localSignatureHandleOpt.IsNil returns true. localSignatureHandleOpt is obtained from SerializeLocalVariablesSignature, which seems to return a default object only if there are no locals. Maybe adding another check is not necessary. I still don't know how to test it, though.

The AreLocalsZeroed bool is now being used during IL mapping in cache. A test was added to check if method bodies that differ only by SkipLocalsInitAttribute have different Relative Virtual Addresses. The test was failing before the changes in the cache mapping and it's working now.

VSadov · 2018-02-09T19:31:50Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

@@ -358,6 +361,14 @@ public override Symbol AssociatedSymbol
            }
        }

+        public bool LocalsAreZeroed


I think this should be virtual on MethodSymbol and just return "true" in the default implementation. Then you do not need to type-check in MethodCompiler.
You will need this anyways when dealing with synthesized members.

Consider the name: AreLocalsZeroed. The rationale here is that we should try to prefix boolean member names with a true / false word: skip, is, are, etc ....

Good suggestion.

All occurrences of LocalsAreZeroed were changed to AreLocalsZeroed.

VSadov · 2018-02-09T19:55:25Z

Done with reviewing iteration 3.
I think we need to make sure method body caching is not interfering with the bit.
Also we need to have some E2E testing where we actually run code that depends on this bit. I think it should be possible.

agocke · 2018-02-09T20:50:58Z

src/Test/Utilities/Portable/CommonTestBase.cs

@@ -472,7 +472,7 @@ private void AddReferencedCompilations(IEnumerable<Compilation> referencedCompil

        #region IL Verification

-        internal abstract string VisualizeRealIL(IModuleSymbol peModule, CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers);
+        internal abstract string VisualizeRealIL(IModuleSymbol peModule, CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers, Cci.IMethodBody methodBody);


Instead of passing the method body, why not just pass the boolean for LocalsAreZeroed? Do we need the method body for anything else?

You have a point.

agocke · 2018-02-09T20:53:28Z

src/Compilers/Core/Portable/CodeGen/MethodBody.cs

@@ -102,7 +105,7 @@ private static ImmutableArray<Cci.SequencePoint> GetSequencePoints(SequencePoint

        ImmutableArray<Cci.ExceptionHandlerRegion> Cci.IMethodBody.ExceptionRegions => _exceptionHandlers;

-        bool Cci.IMethodBody.LocalsAreZeroed => true;
+        bool Cci.IMethodBody.LocalsAreZeroed => _localsAreZeroed;


This is a good point, but I haven't tested to see whether it's likely that you get non-zero memory when running stackalloc. Our tests would have a dependency on having non-zero random memory, wouldn't they?

jaredpar · 2018-02-09T23:33:18Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

@@ -166,6 +166,8 @@ public void EnsureMetadataVirtual()

        private OverriddenOrHiddenMembersResult _lazyOverriddenOrHiddenMembers;

+        protected bool _localsAreZeroed;


Protected members should not begin with an _. Really protected members should be Pascal cased but somehow other members here are camelCased. Lots of bad examples.

Actually, I'll set it to private. I just realized there's no benefit in it being protected.

jaredpar · 2018-02-09T23:34:27Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

@@ -358,6 +361,14 @@ public override Symbol AssociatedSymbol
            }
        }

+        public bool LocalsAreZeroed


Consider the name: AreLocalsZeroed. The rationale here is that we should try to prefix boolean member names with a true / false word: skip, is, are, etc ....

jaredpar · 2018-02-09T23:35:00Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

@@ -1128,6 +1139,10 @@ private void DecodeWellKnownAttributeAppliedToMethod(ref DecodeWellKnownAttribut
                    arguments.Diagnostics.Add(ErrorCode.ERR_SecurityCriticalOrSecuritySafeCriticalOnAsync, arguments.AttributeSyntaxOpt.Location, arguments.AttributeSyntaxOpt.GetErrorDisplayName());
                }
            }
+            else if (attribute.IsTargetAttribute(this, AttributeDescription.SkipLocalsInitAttribute))
+            {
+                _localsAreZeroed &= false;


Why not set this directly to false here?

You have a point. I don't know why I did that.

AlekseyTs · 2018-02-12T19:27:54Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

@@ -1128,6 +1139,10 @@ private void DecodeWellKnownAttributeAppliedToMethod(ref DecodeWellKnownAttribut
                    arguments.Diagnostics.Add(ErrorCode.ERR_SecurityCriticalOrSecuritySafeCriticalOnAsync, arguments.AttributeSyntaxOpt.Location, arguments.AttributeSyntaxOpt.GetErrorDisplayName());
                }
            }
+            else if (attribute.IsTargetAttribute(this, AttributeDescription.SkipLocalsInitAttribute))
+            {
+                _localsAreZeroed &= false;


_localsAreZeroed &= false; [](start = 16, length = 26)

It feels like we should follow the pattern for other well-known attributes and keep this information in attribute data. #Closed

I've made some changes. I followed the pattern from other simple boolean attributes like DynamicSecurityMethodAttribute and SuppressUnmanagedCodeSecurityAttribute.

AlekseyTs · 2018-02-12T19:28:48Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberMethodSymbol.cs

+        {
+            get
+            {
+                return _localsAreZeroed;


_localsAreZeroed [](start = 23, length = 16)

Why is it safe to return this value? Are we certain attributes have been bound and processed? #Closed

…calsAreZeroed added in MethodSymbol

AlekseyTs · 2018-02-12T20:00:17Z

src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs

@@ -1346,7 +1346,7 @@ public override object VisitField(FieldSymbol symbol, TypeCompilationState argum
            var localSlotManager = new LocalSlotManager(variableSlotAllocatorOpt);
            var optimizations = compilation.Options.OptimizationLevel;

-            ILBuilder builder = new ILBuilder(moduleBuilder, localSlotManager, optimizations);
+            ILBuilder builder = new ILBuilder(moduleBuilder, localSlotManager, optimizations, (method as SourceMemberMethodSymbol)?.LocalsAreZeroed ?? true);


(method as SourceMemberMethodSymbol)?.LocalsAreZeroed ?? true [](start = 94, length = 61)

It looks like this is not going to cover lambdas and local functions. #Closed

Our main objective right now is making sure that the attribute works on the method where it's directly applied. In future PRs we'll propagate it to nested lambdas and local functions.

AlekseyTs · 2018-02-12T20:02:42Z

src/Compilers/Core/Portable/CodeGen/ILBuilder.cs

        }

+        public bool LocalsAreZeroed => _localsAreZeroed;


public bool LocalsAreZeroed => _localsAreZeroed; [](start = 8, length = 48)

It looks like it is not necessary to have this property on ILBuilder or have it inside ILBuilder because ILBuilder itself doen't take any advantage of it. #Closed

Never mind, it looks like ILBuilderToString is using this property.

In reply to: 167670235 [](ancestors = 167670235)

AlekseyTs · 2018-02-12T20:06:28Z

src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs

@@ -959,7 +959,7 @@ internal override string VisualizeRealIL(IModuleSymbol peModule, CompilationTest
        /// - winmd
        /// - global methods
        /// </remarks>
-        internal unsafe static string VisualizeRealIL(PEModuleSymbol peModule, CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers)
+        internal unsafe static string VisualizeRealIL(PEModuleSymbol peModule, CompilationTestData.MethodData methodData, IReadOnlyDictionary<int, string> markers, Cci.IMethodBody methodBody)


Cci.IMethodBody methodBody [](start = 164, length = 26)

It feels like an overkill to require all consumers to pass an instance of Cci.IMethodBody just so we can get LocalsAreZeroed value. If this is all we need, we should simply have localsAreZeroed as a parameter. #Closed

AlekseyTs · 2018-02-12T20:07:30Z

src/Compilers/VisualBasic/Portable/Compilation/MethodCompiler.vb

@@ -1507,7 +1507,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
                optimizations = OptimizationLevel.Release
            End If

-            Dim builder As ILBuilder = New ILBuilder(moduleBuilder, localSlotManager, optimizations)
+            Dim builder As ILBuilder = New ILBuilder(moduleBuilder, localSlotManager, optimizations, localsAreZeroed:=True)


localsAreZeroed:=True) [](start = 101, length = 22)

Are we planning to add recognition of the attribute to VB as well? #Pending

Only C#, as far as I know.

AlekseyTs · 2018-02-12T20:14:00Z

src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs

+        /// <summary>
+        /// Returns true if locals are to be initialized
+        /// </summary>
+        public virtual bool LocalsAreZeroed


public virtual bool LocalsAreZeroed [](start = 8, length = 35)

Adding virtual properties at the root is bug prone. We leant to always add abstract properties and have each derived type implement it and then have targeted tests to cover each implementation. I think you'd be better off with the cast in MethodCompiler, otherwise you will need to add and test implementations for retargeted methods, PE methods, etc. Does that worth it? #Closed

Alternatively, we can explicitly throw for all symbols for which we don't expect to call this API. Again this requires this method to have no default implementation. #Closed

Can't we keep the virtual property and throw if the default implementation is called? Then only the method in the derived class could be used. I feel like it has the same result with less effort.

The problem with having the property be virtual is future derivations of MethodSymbol. A year from now if a new derivation of MethodSymbol is added the developer may or may not know to re-evaluate the behavior of LocalsAreZeroed in the context of the type. Whether they do or don't comes down to the dillegence of the developer, and code reviewers, involved in the change. By making it abstract though we force the developer to think about this property and make a decision on its value.

I guess, it is an option to throw from default implementation in this particular situation (for non public API used in very specific place and time in the pipeline), then, assuming that we have adequate test coverage, tests should catch derived classes that fail to override the method. #Closed

In the end I set it to abstract. We're returning true in some derived classes for now (mostly because of synthesized members).

AlekseyTs · 2018-02-12T20:16:11Z

Done with review pass (iteration 4). #Closed

…AreLocalsZeroed verifies if SkipLocalsInitAttribute is decoded before returning

… same AreLocalsZeroed flag

jaredpar · 2018-02-20T16:22:48Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs

@@ -163,6 +163,11 @@ public override DllImportData GetDllImportData()
            return null;
        }

+        public override bool AreLocalsZeroed
+        {
+            get { throw ExceptionUtilities.Unreachable; }


Do we not generate any code for this symbol?

Looks like it's only used in the semantic model, so I suspect not.

agocke · 2018-02-20T19:13:27Z

src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs

@@ -340,6 +340,8 @@ internal override TypeSymbol IteratorElementType

        public override DllImportData GetDllImportData() => null;

+        public override bool AreLocalsZeroed => throw ExceptionUtilities.Unreachable;


I think all of these can be replaced with a single override that returns true in SourceMethodSymbol

Just talked with @AlekseyTs and he thinks we should be explicit in each callsite. I think that's a fine decision as well and we can go with that.

However, I would be very surprised if this didn't throw whenever we try to emit local functions, so I think we'll have to provide a real implementation, even if it's accompanied by a prototype comment saying that the full spec will be implemented in another PR.

To answer my own question, by the time we end up at creating a MethodBody, LocalFunctions and similar are all turned into SynthesizedInstance or similar symbols, so throwing is actually the right thing to do here, I think.

agocke · 2018-02-21T19:23:30Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedIntrinsicOperatorSymbol.cs

@@ -163,6 +163,11 @@ public override DllImportData GetDllImportData()
            return null;
        }

+        public override bool AreLocalsZeroed
+        {
+            get { throw ExceptionUtilities.Unreachable; }


Looks like it's only used in the semantic model, so I suspect not.

agocke · 2018-02-21T19:26:56Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs

+
+            Assert.NotEqual(methodInit.RelativeVirtualAddress, methodSkip.RelativeVirtualAddress);
+        }
+


I think we should also have a few negative tests, like putting the attribute on the class or on the assembly and checking to make sure that this doesn't cause locals init to be emitted, at least as the code is now. Eventually, when you make that work, those tests will fail and we can fix them up to assert the right thing.

AlekseyTs · 2018-02-21T22:21:10Z

src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs

@@ -2949,7 +2949,7 @@ private int SerializeMethodBody(MethodBodyStreamEncoder encoder, IMethodBody met
            // Don't do small body method caching during deterministic builds until this issue is fixed
            // https://github.com/dotnet/roslyn/issues/7595
            int bodyOffset;
-            if (!_deterministic && isSmallBody && _smallMethodBodies.TryGetValue(methodBody.IL, out bodyOffset))
+            if (!_deterministic && isSmallBody && _smallMethodBodies.TryGetValue((methodBody.IL, methodBody.AreLocalsZeroed), out bodyOffset))


(methodBody.IL, methodBody.AreLocalsZeroed) [](start = 81, length = 43)

Consider extracting this tuple into a local and reusing. #Closed

AlekseyTs · 2018-02-21T22:22:28Z

src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs

@@ -4177,5 +4177,20 @@ protected override void AddItem(T item, int index)
                _structuralIndex.Add(item, index);
            }
        }
+
+        private class ByteSequenceBoolTupleComparer : IEqualityComparer<(ImmutableArray<byte>, bool)>


ByteSequenceBoolTupleComparer [](start = 22, length = 29)

Consider adding a private parameter-less constructor #Closed

AlekseyTs · 2018-02-21T22:37:58Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_WellKnownAttributes.cs

+public class C
+{
+    [System.Runtime.CompilerServices.SkipLocalsInitAttribute]
+    public unsafe void M_skip()


public unsafe void M_skip() [](start = 4, length = 27)

Consider adding another copy of each method and assert that methods with attribute have their body unified and methods without an attribute have their body unified. Also, consider adding two more methods with slightly different body and one of them with attribute, verify that their bodies are not unified with any other method. #Closed

AlekseyTs · 2018-02-21T22:42:42Z

Done with review pass (iteration 8) #Closed

agocke

LGTM

AlekseyTs

LGTM (iteration 9)

t-camaia added 3 commits February 8, 2018 18:00

SkipLocalsInitAttribute recognized as a WellKnownAttribute and proper…

339bac2

…ly working on method declarations

ILVisualizer locals init depends on LocalsAreZeroed flag

1712070

SkipLocalsInitAttribute on method tests added

cd07338

t-camaia requested a review from a team as a code owner February 9, 2018 02:42

t-camaia requested review from agocke and VSadov February 9, 2018 02:42

VSadov reviewed Feb 9, 2018

View reviewed changes

agocke reviewed Feb 9, 2018

View reviewed changes

jaredpar reviewed Feb 9, 2018

View reviewed changes

AlekseyTs reviewed Feb 12, 2018

View reviewed changes

LocalsAreZeroed passed as parameter instead of methodBody. Virtual Lo…

dc64263

…calsAreZeroed added in MethodSymbol

AlekseyTs reviewed Feb 12, 2018

View reviewed changes

t-camaia added 3 commits February 13, 2018 19:14

LocalsAreZeroed renamed to AreLocalsZeroed. SourceMemberMethodSymbol.…

665e690

…AreLocalsZeroed verifies if SkipLocalsInitAttribute is decoded before returning

AreLocalsZeroed in MethodSymbol changed from 'virtual' to 'abstract'

ecb278d

SerializeMethodBody cannot use body from cache if they don't have the…

bd4f01b

… same AreLocalsZeroed flag

jaredpar reviewed Feb 20, 2018

View reviewed changes

agocke reviewed Feb 20, 2018

View reviewed changes

agocke reviewed Feb 21, 2018

View reviewed changes

Packages and tests updated. HashCode issue fixed.

3c1f7d7

t-camaia requested a review from a team as a code owner February 21, 2018 22:11

AlekseyTs reviewed Feb 21, 2018

View reviewed changes

agocke approved these changes Feb 21, 2018

View reviewed changes

MetadataWriter and tests updated

a6d98d0

AlekseyTs approved these changes Feb 22, 2018

View reviewed changes

t-camaia merged commit ad944e6 into dotnet:features/localsinit Feb 22, 2018

		@@ -166,6 +166,8 @@ public void EnsureMetadataVirtual()

		private OverriddenOrHiddenMembersResult _lazyOverriddenOrHiddenMembers;

		protected bool _localsAreZeroed;

		@@ -340,6 +340,8 @@ internal override TypeSymbol IteratorElementType

		public override DllImportData GetDllImportData() => null;

		public override bool AreLocalsZeroed => throw ExceptionUtilities.Unreachable;


		Assert.NotEqual(methodInit.RelativeVirtualAddress, methodSkip.RelativeVirtualAddress);
		}

Localsinit #24723

Localsinit #24723

Conversation

t-camaia commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Feb 21, 2018 • edited Loading

agocke left a comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs Feb 12, 2018 •

edited

Loading

AlekseyTs Feb 12, 2018 •

edited

Loading

AlekseyTs Feb 12, 2018 •

edited

Loading

AlekseyTs Feb 12, 2018 •

edited

Loading

AlekseyTs Feb 12, 2018 •

edited

Loading

AlekseyTs Feb 12, 2018 •

edited

Loading

AlekseyTs Feb 12, 2018 •

edited

Loading

AlekseyTs Feb 12, 2018 •

edited

Loading

AlekseyTs Feb 13, 2018 •

edited

Loading

AlekseyTs commented Feb 12, 2018 •

edited

Loading

AlekseyTs Feb 21, 2018 •

edited

Loading

AlekseyTs Feb 21, 2018 •

edited

Loading

AlekseyTs Feb 21, 2018 •

edited

Loading

AlekseyTs commented Feb 21, 2018 •

edited

Loading