Abstract out LocalBuilder, emit Locals with new ILGenerator (dotnet#93809)

buyaa-n · AaronRobinsonMSFT · jkotas · liveans · commit de69e83c05da · 2023-11-09T12:23:21.000+01:00
* Abstract LocalBuilder, emit LocalBuilder in ILGenerator

* Apply suggestions from code review

Co-authored-by: Aaron Robinson &lt;arobins@microsoft.com&gt;

* Remove public LocalBuilder.Method, and apply other feedback

* Avoid invalid cast exception.

Co-authored-by: Jan Kotas &lt;jkotas@microsoft.com&gt;

---------

Co-authored-by: Aaron Robinson &lt;arobins@microsoft.com&gt;
Co-authored-by: Jan Kotas &lt;jkotas@microsoft.com&gt;
diff --git a/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj
@@ -162,14 +162,14 @@
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\CustomAttributeBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\DynamicILGenerator.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\DynamicMethod.CoreCLR.cs" />
-    <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\LocalBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeAssemblyBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeConstructorBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeEnumBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeEventBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeFieldBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeGenericTypeParameterBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeILGenerator.cs" />
+    <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeLocalBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeMethodBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeModuleBuilder.cs" />
     <Compile Include="$(BclSourcesRoot)\System\Reflection\Emit\RuntimeParameterBuilder.cs" />
diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicILGenerator.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicILGenerator.cs
@@ -36,14 +36,14 @@ public override LocalBuilder DeclareLocal(Type localType, bool pinned)
         {
             ArgumentNullException.ThrowIfNull(localType);
 
-            LocalBuilder localBuilder;
+            RuntimeLocalBuilder localBuilder;
 
             RuntimeType? rtType = localType as RuntimeType;
 
             if (rtType == null)
                 throw new ArgumentException(SR.Argument_MustBeRuntimeType);
 
-            localBuilder = new LocalBuilder(m_localCount, localType, m_methodBuilder, pinned);
+            localBuilder = new RuntimeLocalBuilder(m_localCount, localType, m_methodBuilder, pinned);
             // add the localType to local signature
             m_localSignature.AddArgument(localType, pinned);
             m_localCount++;
diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeILGenerator.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeILGenerator.cs
@@ -819,8 +819,8 @@ public override void Emit(OpCode opcode, LocalBuilder local)
             ArgumentNullException.ThrowIfNull(local);
 
             // Puts the opcode onto the IL stream followed by the information for local variable local.
-            int tempVal = local.GetLocalIndex();
-            if (local.GetMethodBuilder() != m_methodBuilder)
+            int tempVal = local.LocalIndex;
+            if (local is not RuntimeLocalBuilder localBuilder || localBuilder.GetMethodBuilder() != m_methodBuilder)
             {
                 throw new ArgumentException(SR.Argument_UnmatchedMethodForLocal, nameof(local));
             }
@@ -1185,7 +1185,7 @@ public override LocalBuilder DeclareLocal(Type localType, bool pinned)
             // add the localType to local signature
             m_localSignature.AddArgument(localType, pinned);
 
-            return new LocalBuilder(m_localCount++, localType, methodBuilder, pinned);
+            return new RuntimeLocalBuilder(m_localCount++, localType, methodBuilder, pinned);
         }
 
         public override void UsingNamespace(string usingNamespace)
diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeLocalBuilder.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeLocalBuilder.cs
@@ -3,7 +3,7 @@
 
 namespace System.Reflection.Emit
 {
-    public sealed class LocalBuilder : LocalVariableInfo
+    internal sealed class RuntimeLocalBuilder : LocalBuilder
     {
         #region Private Data Members
         private readonly int m_localIndex;
@@ -13,9 +13,9 @@ public sealed class LocalBuilder : LocalVariableInfo
         #endregion
 
         #region Constructor
-        internal LocalBuilder(int localIndex, Type localType, MethodInfo methodBuilder)
+        internal RuntimeLocalBuilder(int localIndex, Type localType, MethodInfo methodBuilder)
             : this(localIndex, localType, methodBuilder, false) { }
-        internal LocalBuilder(int localIndex, Type localType, MethodInfo methodBuilder, bool isPinned)
+        internal RuntimeLocalBuilder(int localIndex, Type localType, MethodInfo methodBuilder, bool isPinned)
         {
             m_isPinned = isPinned;
             m_localIndex = localIndex;
@@ -25,14 +25,7 @@ internal LocalBuilder(int localIndex, Type localType, MethodInfo methodBuilder,
         #endregion
 
         #region Internal Members
-        internal int GetLocalIndex()
-        {
-            return m_localIndex;
-        }
-        internal MethodInfo GetMethodBuilder()
-        {
-            return m_methodBuilder;
-        }
+        internal MethodInfo GetMethodBuilder() => m_methodBuilder;
         #endregion
 
         #region LocalVariableInfo Override
diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj b/src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj
@@ -144,7 +144,6 @@
     <Compile Include="System\Reflection\Emit\CustomAttributeBuilder.cs" />
     <Compile Include="System\Reflection\Emit\DynamicILInfo.cs" />
     <Compile Include="System\Reflection\Emit\DynamicMethod.cs" />
-    <Compile Include="System\Reflection\Emit\LocalBuilder.cs" />
     <Compile Include="System\Reflection\Emit\ReflectionEmitThrower.cs" />
     <Compile Include="System\Reflection\Emit\SignatureHelper.cs" />
     <Compile Include="System\Reflection\EnumInfo.cs" />
diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Emit/LocalBuilder.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Emit/LocalBuilder.cs
diff --git a/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems b/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems
@@ -418,7 +418,6 @@
     <Compile Include="$(MSBuildThisFileDirectory)System\IFormatProvider.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\IFormattable.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Index.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)System\Reflection\Emit\ILGenerator.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\SearchValues\BitVector256.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\SearchValues\ProbabilisticWithAsciiCharSearchValues.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\SearchValues\SingleCharSearchValues.cs" />
@@ -674,7 +673,9 @@
     <Compile Include="$(MSBuildThisFileDirectory)System\Reflection\Emit\FieldOnTypeBuilderInstantiation.cs" Condition="'$(FeatureNativeAot)' != 'true'" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Reflection\Emit\FlowControl.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Reflection\Emit\GenericTypeParameterBuilder.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)System\Reflection\Emit\ILGenerator.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Reflection\Emit\Label.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)System\Reflection\Emit\LocalBuilder.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Reflection\Emit\MethodBuilder.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Reflection\Emit\MethodBuilderInstantiation.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)System\Reflection\Emit\MethodOnTypeBuilderInstantiation.cs" Condition="'$(FeatureNativeAot)' != 'true'" />
diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/LocalBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/LocalBuilder.cs
@@ -0,0 +1,16 @@
+﻿// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace System.Reflection.Emit
+{
+    public abstract class LocalBuilder : LocalVariableInfo
+    {
+        /// <summary>
+        /// Initializes a new instance of the <see cref="LocalBuilder"/> class.
+        /// </summary>
+        /// <remarks>
+        /// This constructor is invoked by derived classes.
+        /// </remarks>
+        protected LocalBuilder() { }
+    }
+}
diff --git a/src/libraries/System.Reflection.Emit.ILGeneration/ref/System.Reflection.Emit.ILGeneration.cs b/src/libraries/System.Reflection.Emit.ILGeneration/ref/System.Reflection.Emit.ILGeneration.cs
@@ -67,9 +67,9 @@ public virtual void ThrowException([System.Diagnostics.CodeAnalysis.DynamicallyA
         public static bool operator ==(System.Reflection.Emit.Label a, System.Reflection.Emit.Label b) { throw null; }
         public static bool operator !=(System.Reflection.Emit.Label a, System.Reflection.Emit.Label b) { throw null; }
     }
-    public sealed partial class LocalBuilder : System.Reflection.LocalVariableInfo
+    public abstract class LocalBuilder : System.Reflection.LocalVariableInfo
     {
-        internal LocalBuilder() { }
+        protected LocalBuilder() { }
         public override bool IsPinned { get { throw null; } }
         public override int LocalIndex { get { throw null; } }
         public override System.Type LocalType { get { throw null; } }
diff --git a/src/libraries/System.Reflection.Emit/src/Resources/Strings.resx b/src/libraries/System.Reflection.Emit/src/Resources/Strings.resx
@@ -186,6 +186,9 @@
   <data name="InvalidOperation_ShouldNotHaveMethodBody" xml:space="preserve">
     <value>Method body should not exist.</value>
   </data>
+  <data name="Argument_UnmatchedMethodForLocal" xml:space="preserve">
+    <value>Local passed in does not belong to this ILGenerator.</value>
+  </data>
   <data name="Argument_InvalidLabel" xml:space="preserve">
     <value>Invalid Label.</value>
   </data>
diff --git a/src/libraries/System.Reflection.Emit/src/System.Reflection.Emit.csproj b/src/libraries/System.Reflection.Emit/src/System.Reflection.Emit.csproj
@@ -12,6 +12,7 @@
     <Compile Include="System\Reflection\Emit\FieldBuilderImpl.cs" />
     <Compile Include="System\Reflection\Emit\GenericTypeParameterBuilderImpl.cs" />
     <Compile Include="System\Reflection\Emit\ILGeneratorImpl.cs" />
+    <Compile Include="System\Reflection\Emit\LocalBuilderImpl.cs" />
     <Compile Include="System\Reflection\Emit\MethodBuilderImpl.cs" />
     <Compile Include="System\Reflection\Emit\ModuleBuilderImpl.cs" />
     <Compile Include="System\Reflection\Emit\ParameterBuilderImpl.cs" />
diff --git a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ILGeneratorImpl.cs b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ILGeneratorImpl.cs
@@ -17,6 +17,7 @@ internal sealed class ILGeneratorImpl : ILGenerator
         private bool _hasDynamicStackAllocation;
         private int _maxStackSize;
         private int _currentStack;
+        private List<LocalBuilder> _locals = new();
         private Dictionary<Label, LabelHandle> _labelTable = new(2);
 
         internal ILGeneratorImpl(MethodBuilder methodBuilder, int size)
@@ -28,9 +29,9 @@ internal ILGeneratorImpl(MethodBuilder methodBuilder, int size)
         }
 
         internal int GetMaxStackSize() => _maxStackSize;
-
         internal InstructionEncoder Instructions => _il;
         internal bool HasDynamicStackAllocation => _hasDynamicStackAllocation;
+        internal List<LocalBuilder> Locals => _locals;
 
         public override int ILOffset => _il.Offset;
 
@@ -40,7 +41,19 @@ internal ILGeneratorImpl(MethodBuilder methodBuilder, int size)
         public override void BeginFaultBlock() => throw new NotImplementedException();
         public override void BeginFinallyBlock() => throw new NotImplementedException();
         public override void BeginScope() => throw new NotImplementedException();
-        public override LocalBuilder DeclareLocal(Type localType, bool pinned) => throw new NotImplementedException();
+
+        public override LocalBuilder DeclareLocal(Type localType, bool pinned)
+        {
+            if (_methodBuilder is not MethodBuilderImpl methodBuilder)
+                throw new NotSupportedException();
+
+            ArgumentNullException.ThrowIfNull(localType);
+
+            LocalBuilder local = new LocalBuilderImpl(_locals.Count, localType, methodBuilder, pinned);
+            _locals.Add(local);
+
+            return local;
+        }
 
         public override Label DefineLabel()
         {
@@ -228,7 +241,34 @@ public override void Emit(OpCode opcode, Label[] labels)
             }
         }
 
-        public override void Emit(OpCode opcode, LocalBuilder local) => throw new NotImplementedException();
+        public override void Emit(OpCode opcode, LocalBuilder local)
+        {
+            ArgumentNullException.ThrowIfNull(local);
+
+            if (local is not LocalBuilderImpl localBuilder || localBuilder.GetMethodBuilder() != _methodBuilder)
+            {
+                throw new ArgumentException(SR.Argument_UnmatchedMethodForLocal, nameof(local));
+            }
+
+            int tempVal = local.LocalIndex;
+            string name = opcode.Name!;
+
+            if (name.StartsWith("ldloca"))
+            {
+                _il.LoadLocalAddress(tempVal);
+            }
+            else if (name.StartsWith("ldloc"))
+            {
+                _il.LoadLocal(tempVal);
+            }
+            else if (name.StartsWith("stloc"))
+            {
+                _il.StoreLocal(tempVal);
+            }
+
+            UpdateStackSize(opcode);
+        }
+
         public override void Emit(OpCode opcode, SignatureHelper signature) => throw new NotImplementedException();
         public override void Emit(OpCode opcode, FieldInfo field) => throw new NotImplementedException();
         public override void Emit(OpCode opcode, MethodInfo meth) => throw new NotImplementedException();
diff --git a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/LocalBuilderImpl.cs b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/LocalBuilderImpl.cs
@@ -0,0 +1,35 @@
+﻿// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace System.Reflection.Emit
+{
+    internal sealed class LocalBuilderImpl : LocalBuilder
+    {
+        #region Private Data Members
+        private readonly int _localIndex;
+        private readonly Type _localType;
+        private readonly MethodInfo _method;
+        private readonly bool _isPinned;
+        #endregion
+
+        #region Constructor
+        internal LocalBuilderImpl(int index, Type type, MethodInfo method, bool isPinned)
+        {
+            _isPinned = isPinned;
+            _localIndex = index;
+            _localType = type;
+            _method = method;
+        }
+        #endregion
+
+        #region Internal Members
+        internal MethodInfo GetMethodBuilder() => _method;
+        #endregion
+
+        #region LocalVariableInfo Override
+        public override bool IsPinned => _isPinned;
+        public override Type LocalType => _localType;
+        public override int LocalIndex => _localIndex;
+        #endregion
+    }
+}
diff --git a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs
@@ -188,7 +188,9 @@ private void WriteMethods(TypeBuilderImpl typeBuilder, List<GenericTypeParameter
                 ILGeneratorImpl? il = method.ILGeneratorImpl;
                 if (il != null)
                 {
-                    offset = AddMethodBody(method, il, methodBodyEncoder);
+                    StandaloneSignatureHandle signature = il.Locals.Count == 0 ? default :
+                        _metadataBuilder.AddStandaloneSignature(_metadataBuilder.GetOrAddBlob(MetadataSignatureHelper.LocalSignatureEncoder(il.Locals, this)));
+                    offset = AddMethodBody(method, il, signature, methodBodyEncoder);
                 }
 
                 MethodDefinitionHandle methodHandle = AddMethodDefinition(method, method.GetMethodSignatureBlob(), offset, _nextParameterRowId);
@@ -237,11 +239,11 @@ private void WriteMethods(TypeBuilderImpl typeBuilder, List<GenericTypeParameter
             }
         }
 
-        private static int AddMethodBody(MethodBuilderImpl method, ILGeneratorImpl il, MethodBodyStreamEncoder methodBodyEncoder) =>
-            methodBodyEncoder.AddMethodBody(
+        private static int AddMethodBody(MethodBuilderImpl method, ILGeneratorImpl il, StandaloneSignatureHandle signature, MethodBodyStreamEncoder bodyEncoder) =>
+            bodyEncoder.AddMethodBody(
                 instructionEncoder: il.Instructions,
                 maxStack: il.GetMaxStackSize(),
-                localVariablesSignature: default, // TODO
+                localVariablesSignature: signature,
                 attributes: method.InitLocals ? MethodBodyAttributes.InitLocals : MethodBodyAttributes.None,
                 hasDynamicStackAllocation: il.HasDynamicStackAllocation);
 
diff --git a/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs b/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs
@@ -1,6 +1,7 @@
 ﻿// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.Reflection.Metadata;
 using System.Reflection.Metadata.Ecma335;
@@ -10,6 +11,20 @@ namespace System.Reflection.Emit
     // TODO: Only support simple signatures. More complex signatures (generics, array, byref, pointers etc) will be added.
     internal static class MetadataSignatureHelper
     {
+        internal static BlobBuilder LocalSignatureEncoder(List<LocalBuilder> locals, ModuleBuilderImpl module)
+        {
+            BlobBuilder localSignature = new();
+            LocalVariablesEncoder encoder = new BlobEncoder(localSignature).LocalVariableSignature(locals.Count);
+
+            foreach(LocalBuilder local in locals)
+            {
+                WriteSignatureForType(encoder.AddVariable().Type(local.LocalType.IsByRef, local.IsPinned),
+                    local.LocalType.IsByRef ? local.LocalType.GetElementType()! : local.LocalType, module);
+            }
+
+            return localSignature;
+        }
+
         internal static BlobBuilder FieldSignatureEncoder(Type fieldType, ModuleBuilderImpl module)
         {
             BlobBuilder fieldSignature = new();
diff --git a/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveILGeneratorTests.cs b/src/libraries/System.Reflection.Emit/tests/PersistableAssemblyBuilder/AssemblySaveILGeneratorTests.cs
diff --git a/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj b/src/mono/System.Private.CoreLib/System.Private.CoreLib.csproj
diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeILGenerator.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeILGenerator.Mono.cs
diff --git a/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeLocalBuilder.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeLocalBuilder.Mono.cs

Original file line number	Diff line number	Diff line change
`@@ -819,8 +819,8 @@ public override void Emit(OpCode opcode, LocalBuilder local)`
`819`	`819`	`ArgumentNullException.ThrowIfNull(local);`
`820`	`820`
`821`	`821`	`// Puts the opcode onto the IL stream followed by the information for local variable local.`
`822`		`- int tempVal = local.GetLocalIndex();`
`823`		`- if (local.GetMethodBuilder() != m_methodBuilder)`
	`822`	`+ int tempVal = local.LocalIndex;`
	`823`	`+ if (local is not RuntimeLocalBuilder localBuilder \|\| localBuilder.GetMethodBuilder() != m_methodBuilder)`
`824`	`824`	`{`
`825`	`825`	`throw new ArgumentException(SR.Argument_UnmatchedMethodForLocal, nameof(local));`
`826`	`826`	`}`
`@@ -1185,7 +1185,7 @@ public override LocalBuilder DeclareLocal(Type localType, bool pinned)`
`1185`	`1185`	`// add the localType to local signature`
`1186`	`1186`	`m_localSignature.AddArgument(localType, pinned);`
`1187`	`1187`
`1188`		`- return new LocalBuilder(m_localCount++, localType, methodBuilder, pinned);`
	`1188`	`+ return new RuntimeLocalBuilder(m_localCount++, localType, methodBuilder, pinned);`
`1189`	`1189`	`}`
`1190`	`1190`
`1191`	`1191`	`public override void UsingNamespace(string usingNamespace)`
Original file line number	Diff line number	Diff line change
`@@ -67,9 +67,9 @@ public virtual void ThrowException([System.Diagnostics.CodeAnalysis.DynamicallyA`
`67`	`67`	`public static bool operator ==(System.Reflection.Emit.Label a, System.Reflection.Emit.Label b) { throw null; }`
`68`	`68`	`public static bool operator !=(System.Reflection.Emit.Label a, System.Reflection.Emit.Label b) { throw null; }`
`69`	`69`	`}`
`70`		`- public sealed partial class LocalBuilder : System.Reflection.LocalVariableInfo`
	`70`	`+ public abstract class LocalBuilder : System.Reflection.LocalVariableInfo`
`71`	`71`	`{`
`72`		`- internal LocalBuilder() { }`
	`72`	`+ protected LocalBuilder() { }`
`73`	`73`	`public override bool IsPinned { get { throw null; } }`
`74`	`74`	`public override int LocalIndex { get { throw null; } }`
`75`	`75`	`public override System.Type LocalType { get { throw null; } }`