PersistedAssemblyBuilder generates invalid IL

[osen1]

Description

PersistedAssemblyBuilder generates an invalid DLL under special circumstances. Replace PersistedAssemblyBuilder by Lokad.ILPack.AssemblyGenerator, and it works. I tried to reduce the code example to the minimum (see reproduction steps), but I'm still not sure what causes the problem.

I'm using an ILGenerator to write something that looks like that:

public static void Test()
{
if (false)
{
    while (Util.CCC())
    {
        int num = Util.NNN();
        if (3 == num)
        {
            Util.OOO();
        }
    }
}
TestClass b = TestClass.New();
Util.EEE(b);
}

public class TestClass : object
{
    public static TestClass New()
    {
        return new TestClass();
    }
}


public static class Util
{
    public static bool CCC()
    {
        return false;
    }

    public static void EEE(TestClass b)
    {
    }

    public static int NNN()
    {
        return 3;
    }

    public static void OOO()
    {
    }
}

Reproduction Steps

using System;
using System.Reflection;
using System.Reflection.Emit;

PersistedAssemblyBuilder persistedAssemblyBuilder = new PersistedAssemblyBuilder(new AssemblyName("Test"), typeof(object).Assembly);
Emit(persistedAssemblyBuilder);
persistedAssemblyBuilder.Save("Test.dll");

AssemblyBuilder assemblyBuilder2 = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("Test"), AssemblyBuilderAccess.RunAndCollect);
Emit(assemblyBuilder2);
Lokad.ILPack.AssemblyGenerator ag = new Lokad.ILPack.AssemblyGenerator();
ag.GenerateAssembly(assemblyBuilder2, "Test_ILPack.dll");


static void Emit(AssemblyBuilder assemblyBuilder)
{
  ModuleBuilder moduleBuilder = assemblyBuilder.DefineDynamicModule("Test");
  TypeBuilder typeBuilder = moduleBuilder.DefineType("Class1", TypeAttributes.Class | TypeAttributes.Public);
  MethodBuilder methb = typeBuilder.DefineMethod("Test", MethodAttributes.Public | MethodAttributes.Static, CallingConventions.Standard, typeof(void), []);
  ILGenerator ilGenerator = methb.GetILGenerator();
  Label label0 = ilGenerator.DefineLabel();
  ilGenerator.Emit(OpCodes.Ldc_I4_0);
  ilGenerator.Emit(OpCodes.Ldc_I4_0);
  ilGenerator.Emit(OpCodes.Beq, label0);
  ilGenerator.BeginScope();
  Label label1 = ilGenerator.DefineLabel();
  Label label2 = ilGenerator.DefineLabel();
  ilGenerator.MarkLabel(label1);
  MethodInfo methodInfo = typeof(Util).GetMethod(nameof(Util.CCC), new Type[] { });
  ilGenerator.Emit(OpCodes.Call, methodInfo);
  ilGenerator.Emit(OpCodes.Ldc_I4, 0);
  ilGenerator.Emit(OpCodes.Beq, label2);
  methodInfo = typeof(Util).GetMethod(nameof(Util.NNN), new Type[] { });
  ilGenerator.Emit(OpCodes.Call, methodInfo);
  LocalBuilder lb1 = ilGenerator.DeclareLocal(typeof(System.Int32));
  Label label3 = ilGenerator.DefineLabel();
  EmitStLoc(ilGenerator, lb1.LocalIndex);
  ilGenerator.Emit(OpCodes.Ldc_I4_3);
  EmitLdLoc(ilGenerator, lb1.LocalIndex);
  ilGenerator.Emit(OpCodes.Ceq);
  ilGenerator.Emit(OpCodes.Ldc_I4_0);
  ilGenerator.Emit(OpCodes.Beq, label3);
  methodInfo = typeof(Util).GetMethod(nameof(Util.OOO), new Type[] { });
  ilGenerator.Emit(OpCodes.Call, methodInfo);
  ilGenerator.Emit(OpCodes.Br, label3);
  ilGenerator.MarkLabel(label3);
  ilGenerator.Emit(OpCodes.Br, label1);
  ilGenerator.MarkLabel(label2);
  ilGenerator.EndScope();
  ilGenerator.Emit(OpCodes.Br, label0);
  ilGenerator.MarkLabel(label0);
  methodInfo = typeof(TestClass).GetMethod(nameof(TestClass.New), new Type[] { });
  ilGenerator.Emit(OpCodes.Call, methodInfo);
  LocalBuilder lb2 = ilGenerator.DeclareLocal(typeof(TestClass));
  EmitStLoc(ilGenerator, lb2.LocalIndex);
  EmitLdLoc(ilGenerator, lb2.LocalIndex);
  methodInfo = typeof(Util).GetMethod(nameof(Util.EEE), new Type[] { typeof(TestClass) });
  ilGenerator.Emit(OpCodes.Call, methodInfo);
  ilGenerator.Emit(OpCodes.Ret);

  typeBuilder.CreateType();
}

static void EmitLdLoc(ILGenerator ilGenerator, int variableOffset)
{
  if (variableOffset < 255)
  {
    switch (variableOffset)
    {
      case 0:
        ilGenerator.Emit(OpCodes.Ldloc_0);
        break;
      case 1:
        ilGenerator.Emit(OpCodes.Ldloc_1);
        break;
      case 2:
        ilGenerator.Emit(OpCodes.Ldloc_2);
        break;
      case 3:
        ilGenerator.Emit(OpCodes.Ldloc_3);
        break;
      default:
        ilGenerator.Emit(OpCodes.Ldloc_S, (byte)variableOffset);
        break;
    }
  }
  else
  {
    ilGenerator.Emit(OpCodes.Ldloc, variableOffset);
  }
}

static void EmitStLoc(ILGenerator ilGenerator, int variableOffset)
{
  if (variableOffset < 255)
  {
    switch (variableOffset)
    {
      case 0:
        ilGenerator.Emit(OpCodes.Stloc_0);
        break;
      case 1:
        ilGenerator.Emit(OpCodes.Stloc_1);
        break;
      case 2:
        ilGenerator.Emit(OpCodes.Stloc_2);
        break;
      case 3:
        ilGenerator.Emit(OpCodes.Stloc_3);
        break;
      default:
        ilGenerator.Emit(OpCodes.Stloc_S, (byte)variableOffset);
        break;
    }
  }
  else
  {
    ilGenerator.Emit(OpCodes.Stloc, variableOffset);
  }
}

Expected behavior

No error

Actual behavior

My Test() method, decompiled with ILSpy, looks like that:

public static void Test()
{
    //IL_003c: Expected I4, but got O
    //IL_0047: Expected O, but got I4
    //IL_0018: Expected O, but got I4
    if (false)
    {
	while (Util.CCC())
	{
		TestClass testClass = (TestClass)Util.NNN();
		if (3 == (nint)testClass)
		{
			Util.OOO();
		}
	}
    }
    int num = (int)TestClass.New();
    Util.EEE((TestClass)num);
}

Obviously, running it will crash : "System.InvalidProgramException: Common Language Runtime detected an invalid program.".

Regression?

I believe it has never worked since PersistedAssemblyBuilder was introduced, but I haven't tried it before 9.0.2
However, it has worked correctly with .Net Framework 4.8 and AssemblyBuilderAccess.Save

Known Workarounds

https://github.com/Lokad/ILPack

Configuration

I have seen this in .Net 9.0.2 and 9.0.3
It's probably irrelevant, but I'm using Windows 11 and x64

Other information

No response

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

[KalleOlaviNiemitalo]

Your code starts a nested scope, declares LocalBuilder lb1 = ilGenerator.DeclareLocal(typeof(System.Int32)); there, ends the nested scope, and declares LocalBuilder lb2 = ilGenerator.DeclareLocal(typeof(TestClass)); in the method scope. lb1.LocalIndex becomes 0 and lb2.LocalIndex becomes 1. Your code uses those LocalIndex values when emitting ldloc and stloc instructions, so to be consistent, the local variable signature should list first lb1 and then lb2. However, it actually lists first lb2 and then lb1. ILGeneratorImpl.Scope.GetLocals() returns first the locals of that scope and then recursively the locals of child scopes:

runtime/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ILGeneratorImpl.cs

Lines 929 to 947 in 80aa709

	internal List<LocalBuilder> GetAllLocals()
	{
	List<LocalBuilder> locals = new List<LocalBuilder>();
	if (_locals != null)
	{
	locals.AddRange(_locals);
	}
	if (_children != null)
	{
	foreach (Scope child in _children)
	{
	locals.AddRange(child.GetAllLocals());
	}
	}
	return locals;
	}

As a workaround, you could remove all BeginScope and EndScope calls. In addition, the ilGenerator.Emit(OpCodes.Ldstr, "SWT"); call in your code looks spurious.

Perhaps this can be fixed by removing ILGeneratorImpl.Scope.GetLocals() and instead making ILGeneratorImpl.DeclareLocal add the new LocalBuilder directly to a list in ILGeneratorImpl itself.

[KalleOlaviNiemitalo]

making ILGeneratorImpl.DeclareLocal add the new LocalBuilder directly to a list in ILGeneratorImpl itself

Like .NET Framework does here: https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/mscorlib/system/reflection/emit/ilgenerator.cs#L1292-L1293

I wonder why DeclareLocal was implemented differently in .NET Core.

[osen1]

Thx. This code above is the result of heavily simplifying a much more complex real world example, done by me manually, therefore it's maybe a little bit of a mess (ldstr "SWT" should have been removed - sorry for that) , but also removing the nested scope isn't an option, although it works perfectly for this small code sample.

[steveharter]

Verified repro, and updated the original description to remove the "SWT" and added some c# formatting.

Instead of

  .locals init (int32 V_0, class [ConsoleApp368]ConsoleApp368.Program/TestClass V_1)

we're getting

  .locals init (class [ConsoleApp368]ConsoleApp368.Program/TestClass V_0, int32 V_1)

[buyaa-n]

Perhaps this can be fixed by removing ILGeneratorImpl.Scope.GetLocals() and instead making ILGeneratorImpl.DeclareLocal add the new LocalBuilder directly to a list in ILGeneratorImpl itself.

Like .NET Framework does here: https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/mscorlib/system/reflection/emit/ilgenerator.cs#L1292-L1293

I wonder why DeclareLocal was implemented differently in .NET Core.

Local variables scoped for adding symbol info with .NET Core PDB builder, LocalScope table should have a handle to the 1st local variable declared within that scope. Therefore local variables needed to be added to the scope it is declared. For more info check PortablePdb-Metadata.md

For reference LocalScopes added here:

runtime/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs

Lines 513 to 526 in a3fe5b1

	private void AddLocalScope(MethodDefinitionHandle methodHandle, ImportScopeHandle parentImport, LocalVariableHandle firstLocalVariableHandle, Scope scope)
	{
	parentImport = GetImportScopeHandle(scope._importNamespaces, parentImport);
	firstLocalVariableHandle = GetLocalVariableHandle(scope._locals, firstLocalVariableHandle);
	_pdbBuilder.AddLocalScope(methodHandle, parentImport, firstLocalVariableHandle,
	constantList: MetadataTokens.LocalConstantHandle(1), scope._startOffset, scope._endOffset - scope._startOffset);
	if (scope._children != null)
	{
	foreach (Scope childScope in scope._children)
	{
	AddLocalScope(methodHandle, parentImport, MetadataTokens.LocalVariableHandle(_pdbBuilder.GetRowCount(TableIndex.LocalVariable) + 1), childScope);
	}
	}

Also we cannot add local signatures on the go like in .NET framework because valid metadata tokens for a Type will not be populated until the assembly saved.

I think we should sort the Locals list for before populating locals signature on save

[KalleOlaviNiemitalo]

@buyaa-n what's the scenario that requires sorting? The reflection emit API only allows adding local variables to the current scope, not to an arbitrary scope given as a parameter, so the local variables of the scope should be contiguous already.

[KalleOlaviNiemitalo]

Do you mean like

1. DeclareLocal: method scope, LocalIndex=0
2. BeginScope: scope A
3. DeclareLocal: scope A, LocalIndex=1
4. EndScope: scope A
5. DeclareLocal: method scope, LocalIndex=2
6. BeginScope: scope B
7. DeclareLocal: scope B, LocalIndex=3
8. EndScope: scope B
9. DeclareLocal: method scope, LocalIndex=4

And then the signatures would be ordered by LocalIndex, but the portable PDB would contain

• LocalScope slot 1 for method scope: VariableList=100
  • LocalVariable slot 100: Index=0
  • LocalVariable slot 101: Index=2
  • LocalVariable slot 102: Index=4
• LocalScope slot 2 for scope A: VariableList=103
  • LocalVariable slot 103: Index=1
• LocalScope slot 3 for scope B: VariableList=104
  • LocalVariable slot 104: Index=3

[buyaa-n]

@buyaa-n what's the scenario that requires sorting? The reflection emit API only allows adding local variables to the current scope, not to an arbitrary scope given as a parameter, so the local variables of the scope should be contiguous already.

Sorting would not need for emitting PDB metadata, PDB builder will emit each local scope and as you mentioned locals will be contiguous within a scope. And it preserves the local index when adding local index:

runtime/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs

Line 538 in a3fe5b1

LocalVariableHandle localHandle = _pdbBuilder.AddLocalVariable(LocalVariableAttributes.None, local.LocalIndex,

Sorting needs while emitting the assembly, for assembly metadata builder local scopes are not needed/used. All locals added while creating locals signature handle for a method:

runtime/src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/ModuleBuilderImpl.cs

Lines 361 to 362 in 80aa709

	StandaloneSignatureHandle signature = il.LocalCount == 0 ? default :
	_metadataBuilder.AddStandaloneSignature(_metadataBuilder.GetOrAddBlob(MetadataSignatureHelper.GetLocalSignature(il.Locals, this)));

Because there is no way to set the index for the signature and locals will be added by the list order