PersistedAssemblyBuilder: local tokens emitted incorrectly because of the scoping levels

[buyaa-n]

In the PersistedAssemblyBuilder ILGenerator local variables are added to a Scope, this is important for PDB builder to emit local variables for each local Scope, but for assembly builder Scopes has no effect, all locals loaded from the Scopes added to a list:

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

Lines 931 to 948 in 248d6f4

	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;

and used for creating local signature for a method body:

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

Lines 361 to 363 in 248d6f4

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

When locals created in nested scope with different order, for example inner scope locals created before the outer scope locals are listed after the outer scope locals and added to the signature with that order and causing the bug #114097

As we need to keep the locals scoped for PDB generation, I think we should order the locals list by id before generating the locals signature

Fixes #114097

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

[steveharter]

Thanks Buyaa for picking this up!

[steveharter]

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

[buyaa-n]

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

[steveharter]

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.

[buyaa-n]

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.

[steveharter]

Thanks again.