Synchronizing dumper with monomod and fixing ILLabel dumping

Harmony/Internal/Util/CecilEmitter.cs

@@ -1,10 +1,12 @@
 using HarmonyLib.Tools;
 using Mono.Cecil;
 using Mono.Cecil.Cil;
+using Mono.Collections.Generic;
 using MonoMod.Cil;
 using MonoMod.Utils;
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.IO;
 using System.Linq;
 using System.Reflection;
@@ -19,17 +21,40 @@ namespace HarmonyLib.Internal.Util;
 /// <summary>
 /// Basic safe DLL emitter for dynamically generated <see cref="MethodDefinition"/>s.
 /// </summary>
-/// <remarks>Based on https://github.com/MonoMod/MonoMod.Common/blob/master/Utils/DMDGenerators/DMDCecilGenerator.cs</remarks>
+/// <remarks>Based on https://github.com/MonoMod/MonoMod/blob/reorganize/src/MonoMod.Utils/DMDGenerators/DMDCecilGenerator.cs</remarks>
 internal static class CecilEmitter
 {
 	private static readonly ConstructorInfo UnverifiableCodeAttributeConstructor =
 		typeof(UnverifiableCodeAttribute).GetConstructor(Type.EmptyTypes);
 
 	public static void Dump(MethodDefinition md, IEnumerable<string> dumpPaths, MethodBase original = null)
 	{
+		using var module = DumpImpl(md, original);
+
+		foreach (var settingsDumpPath in dumpPaths)
+		{
+			var fullPath = Path.GetFullPath(settingsDumpPath);
+			try
+			{
+				Directory.CreateDirectory(fullPath);
+				using var stream = File.OpenWrite(Path.Combine(fullPath, $"{module.Name}.dll"));
+				module.Write(stream);
+			}
+			catch (Exception e)
+			{
+				Logger.Log(Logger.LogChannel.Error, () => $"Failed to dump {md.GetID(simple: true)} to {fullPath}: {e}");
+			}
+		}
+	}
+
+	internal static ModuleDefinition DumpImpl(MethodDefinition md, MethodBase original = null)
+	{
+		if(md.Body is null)
+			throw new ArgumentException($"Body of MethodDefinition '{md.Name}' to dump is null");
+
 		var name = $"HarmonyDump.{SanitizeTypeName(md.GetID(withType: false, simple: true))}.{Guid.NewGuid().GetHashCode():X8}";
 		var originalName = (original?.Name ?? md.Name).Replace('.', '_');
-		using var module = ModuleDefinition.CreateModule(name,
+		var module = ModuleDefinition.CreateModule(name,
 			new ModuleParameters
 			{
 				Kind = ModuleKind.Dll, ReflectionImporterProvider = MMReflectionImporter.ProviderNoDefault
@@ -47,51 +72,75 @@ public static void Dump(MethodDefinition md, IEnumerable<string> dumpPaths, Meth
 
 		module.Types.Add(td);
 
-		MethodDefinition clone = null;
-
-		var isVolatile = new TypeReference("System.Runtime.CompilerServices", "IsVolatile", module,
-			module.TypeSystem.CoreLibrary);
-
-		Relinker relinker = (mtp, _) => mtp == md ? clone : module.ImportReference(mtp);
-
-		clone =
+		var clone =
 			new MethodDefinition(original?.Name ?? "_" + md.Name.Replace(".", "_"), md.Attributes, module.TypeSystem.Void)
 			{
 				MethodReturnType = md.MethodReturnType,
 				Attributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.Static,
 				ImplAttributes = MethodImplAttributes.IL | MethodImplAttributes.Managed,
 				DeclaringType = td,
-				HasThis = false
+				HasThis = false,
+				NoInlining = true
 			};
 		td.Methods.Add(clone);
 
+#pragma warning disable IDE0039 // Use local function
+		Relinker relinker = (mtp, _) =>
+		{
+			if (mtp == md)
+				return clone!;
+			if (mtp is MethodReference mr)
+			{
+				if (mr.FullName == md.FullName
+				    && mr.DeclaringType.FullName == md.DeclaringType.FullName
+				    && mr.DeclaringType.Scope.Name == md.DeclaringType.Scope.Name)
+					return clone!;
+			}
+			return module.ImportReference(mtp);
+		};
+#pragma warning restore IDE0039 // Use local function
+
 		foreach (var param in md.Parameters)
 			clone.Parameters.Add(param.Clone().Relink(relinker, clone));
 
 		clone.ReturnType = md.ReturnType.Relink(relinker, clone);
 		var body = clone.Body = md.Body.Clone(clone);
 
-		foreach (var variable in clone.Body.Variables)
+		foreach (var variable in body.Variables)
 			variable.VariableType = variable.VariableType.Relink(relinker, clone);
 
-		foreach (var handler in clone.Body.ExceptionHandlers.Where(handler => handler.CatchType != null))
+		foreach (var handler in body.ExceptionHandlers.Where(handler => handler.CatchType != null))
 			handler.CatchType = handler.CatchType.Relink(relinker, clone);
 
+		/* see below
+		var isVolatile = new TypeReference("System.Runtime.CompilerServices", "IsVolatile", module,
+			module.TypeSystem.CoreLibrary);
+		*/
 		foreach (var instr in body.Instructions)
 		{
 			var operand = instr.Operand;
 			operand = operand switch
 			{
 				ParameterDefinition param  => clone.Parameters[param.Index],
-				ILLabel label => label.Target,
+				ILLabel label => LabelFix(label, body.Instructions, md.Body.Instructions, originalName),
 				IMetadataTokenProvider mtp => mtp.Relink(relinker, clone),
 				_                          => operand
 			};
 
-			if (instr.Previous?.OpCode == OpCodes.Volatile &&
-			    operand is FieldReference fref &&
-			    (fref.FieldType as RequiredModifierType)?.ModifierType != isVolatile)
-				fref.FieldType = new RequiredModifierType(isVolatile, fref.FieldType);
+			// System.Reflection doesn't contain any volatility info.
+			// System.Reflection.Emit presumably does something similar to this.
+			// Mono.Cecil thus isn't aware of the volatility as part of the imported field reference.
+			// The modifier is still necessary though.
+			// This is done here instead of the copier as Harmony and other users can't track modreqs
+
+			// This isn't actually a valid transformation though. A ldfld or stfld can have the volatile
+			// prefix, without having modreq(IsVolatile) on the field. Adding the modreq() causes the runtime
+			// to not be able to find the field.
+			/*if (instr.Previous?.OpCode == OpCodes.Volatile &&
+			   operand is FieldReference fref &&
+			   (fref.FieldType as RequiredModifierType)?.ModifierType != tr_IsVolatile) {
+			   fref.FieldType = new RequiredModifierType(tr_IsVolatile, fref.FieldType);
+			}*/
 
 			instr.Operand = operand;
 		}
@@ -105,20 +154,7 @@ operand is FieldReference fref &&
 				new ParameterDefinition("<>_this", ParameterAttributes.None, type.Relink(relinker, clone)));
 		}
 
-		foreach (var settingsDumpPath in dumpPaths)
-		{
-			var fullPath = Path.GetFullPath(settingsDumpPath);
-			try
-			{
-				Directory.CreateDirectory(fullPath);
-				using var stream = File.OpenWrite(Path.Combine(fullPath, $"{module.Name}.dll"));
-				module.Write(stream);
-			}
-			catch (Exception e)
-			{
-				Logger.Log(Logger.LogChannel.Error, () => $"Failed to dump {md.GetID(simple: true)} to {fullPath}: {e}");
-			}
-		}
+		return module;
 	}
 
 	private static string SanitizeTypeName(string typeName)
@@ -129,4 +165,14 @@ private static string SanitizeTypeName(string typeName)
 			.Replace("<", "{")
 			.Replace(">", "}");
 	}
+
+	private static Instruction LabelFix(ILLabel label, Collection<Instruction> clone, Collection<Instruction> original, string originalName)
+	{
+		Debug.Assert(clone.Count == original.Count);
+		var target = label.Target;
+		var idx = original.IndexOf(target);
+		if (idx == -1)
+			throw new NullReferenceException($"ILLabel from dump copy of '{originalName}' cannot be relinked. It points to '{target}', which was not found in the original body");
+		return clone[idx];
+	}
 }

HarmonyTests/Extras/Dump.cs

@@ -0,0 +1,36 @@
+using HarmonyLib.Internal.Util;
+using HarmonyLibTests;
+using MonoMod.Cil;
+using MonoMod.Utils;
+using NUnit.Framework;
+using System;
+using System.Linq;
+
+
+namespace HarmonyTests.Extras;
+
+[TestFixture]
+public class Dump : TestLogger
+{
+	[Test]
+	public void Test_DumpIlLabel()
+	{
+		using var dmd = new DynamicMethodDefinition("DumpIlLabelTestMethod", typeof(void), Type.EmptyTypes);
+		var il = new ILContext(dmd.Definition);
+		var cur = new ILCursor(il);
+
+		var label = cur.DefineLabel();
+		cur.EmitBr(label);
+		cur.EmitRet();
+		label.Target = cur.Prev;
+
+		var method = cur.Method;
+
+		using var dumpedModule = CecilEmitter.DumpImpl(method);
+		var dumpedMethod = dumpedModule.Types.SelectMany(i => i.Methods).FirstOrDefault();
+		Assert.NotNull(dumpedMethod);
+
+		var instructions = dumpedMethod!.Body.Instructions;
+		Assert.True(instructions[0].Operand == instructions[1]);
+	}
+}