Skip to content

PersistedAssemblyBuilder: local tokens emitted incorrectly because of the scoping levels #114758

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
merged 4 commits into from
Apr 22, 2025
Merged

PersistedAssemblyBuilder: local tokens emitted incorrectly because of the scoping levels #114758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b1d149b
Local ids emmitted incorrectly becuase of the scoping level
buyaa-n Apr 17, 2025
c5cd436
Merge branch 'main' of github.com:dotnet/runtime into fix-114097
buyaa-n Apr 17, 2025
de24670
Apply feedback
buyaa-n Apr 17, 2025
087ad3c
Merge branch 'main' into fix-114097
steveharter Apr 22, 2025
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
2 changes: 2 additions & 0 deletions src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/SignatureHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ internal static class MetadataSignatureHelper
{
internal static BlobBuilder GetLocalSignature(List<LocalBuilder> locals, ModuleBuilderImpl module)
{
locals.Sort((l1, l2) => l1.LocalIndex - l2.LocalIndex); // sort by created order
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to add a List<LocalBuilder> field to ILGenerator and add to it whenever DeclareLocal() is called and then use that? That should prevent the need for sorting, but requires some dual tracking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is an another way, if you prefer that let me know. For me there is not much difference. Note that this GetLocalSignature(...) method only called once per method.


BlobBuilder localSignature = new();
LocalVariablesEncoder encoder = new BlobEncoder(localSignature).LocalVariableSignature(locals.Count);

Expand Down
82 changes: 82 additions & 0 deletions ...aries/System.Reflection.Emit/tests/PortablePdb/ILGeneratorScopesAndSequencePointsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.IO;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using System.Runtime.Loader;
using Xunit;

namespace System.Reflection.Emit.Tests
Expand Down Expand Up @@ -368,5 +369,86 @@ private class TestDocument : ISymbolDocumentWriter
public void SetCheckSum(Guid algorithmId, byte[] checkSum) => throw new NotImplementedException();
public void SetSource(byte[] source) => throw new NotImplementedException();
}

[Fact]
public void InnerScopeLocalCreatedBeforeOuterScopeLocal()
{

PersistedAssemblyBuilder ab = AssemblySaveTools.PopulateAssemblyBuilderAndTypeBuilder(out TypeBuilder type);
Emit(type);

using var stream = new MemoryStream();
ab.Save(stream);

stream.Seek(0, SeekOrigin.Begin);
var assembly = AssemblyLoadContext.Default.LoadFromStream(stream);
var method = assembly.GetType("MyType")!.GetMethod("Test");
method!.Invoke(null, null); // should not throw

static void Emit(TypeBuilder typeBuilder)
{
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();
ilGenerator.Emit(OpCodes.Stloc_0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this use EmitStLoc(ilGenerator, lb1.LocalIndex) like the original issue (here and below using lb2) instead of hard-coding the 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That method doesn't affect the bug, just adds more rows, and only called twice

ilGenerator.Emit(OpCodes.Ldc_I4_3);
ilGenerator.Emit(OpCodes.Ldloc_0);
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.MarkLabel(label3);
ilGenerator.Emit(OpCodes.Br, label1);
ilGenerator.MarkLabel(label2);
ilGenerator.EndScope();
ilGenerator.MarkLabel(label0);
methodInfo = typeof(TestClass).GetMethod(nameof(TestClass.New), new Type[] { });
ilGenerator.Emit(OpCodes.Call, methodInfo);
LocalBuilder lb2 = ilGenerator.DeclareLocal(typeof(TestClass));
ilGenerator.Emit(OpCodes.Stloc_1);
ilGenerator.Emit(OpCodes.Ldloc_1);
methodInfo = typeof(Util).GetMethod(nameof(Util.EEE), new Type[] { typeof(TestClass) });
ilGenerator.Emit(OpCodes.Call, methodInfo);
ilGenerator.Emit(OpCodes.Ret);

typeBuilder.CreateType();
}
}
}

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


public static class Util
{
public static bool CCC() => false;

public static void EEE(TestClass b) { }

public static int NNN() => 3;

public static void OOO() { }
}
}
Loading