From 6c7e2b02d6acf7fe7427472ccb6523732099c8c7 Mon Sep 17 00:00:00 2001
From: Andreas Pardeike <andreas@pardeike.net>
Date: Mon, 10 Feb 2020 03:31:44 +0100
Subject: [PATCH] Fixed IntPtr result buffers for real

---
 Harmony/GlobalSuppressions.cs              |   7 +-
 Harmony/Internal/MethodPatcher.cs          |  36 ++++---
 Harmony/Internal/NativeThisPointerCheck.cs | 116 +++++++++++++--------
 Harmony/Public/PatchProcessor.cs           |   4 +-
 HarmonyTests/IL/TestMethodBodyReader.cs    |   2 +-
 5 files changed, 101 insertions(+), 64 deletions(-)

diff --git a/Harmony/GlobalSuppressions.cs b/Harmony/GlobalSuppressions.cs
index 7cbe047a..55c52344 100644
--- a/Harmony/GlobalSuppressions.cs
+++ b/Harmony/GlobalSuppressions.cs
@@ -8,4 +8,9 @@
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Performance", "CA1825:Avoid zero-length array allocations.")]
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Performance", "CA1813:Avoid unsealed attributes")]
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Performance", "CA1810:Initialize reference type static fields inline")]
-[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types")]
\ No newline at end of file
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1031:Do not catch general exception types")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Code Quality", "IDE0051:Remove unused private members", Justification = "<Pending>", Scope = "member", Target = "~F:HarmonyLib.Sandbox.SomeStruct.b1")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Code Quality", "IDE0051:Remove unused private members", Justification = "<Pending>", Scope = "member", Target = "~F:HarmonyLib.Sandbox.SomeStruct.b2")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Code Quality", "IDE0051:Remove unused private members", Justification = "<Pending>", Scope = "member", Target = "~F:HarmonyLib.Sandbox.SomeStruct.b3")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0060:Remove unused parameter", Justification = "<Pending>", Scope = "member", Target = "~M:HarmonyLib.Sandbox.GetStruct(System.IntPtr,System.IntPtr)~HarmonyLib.Sandbox.SomeStruct")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0060:Remove unused parameter", Justification = "<Pending>", Scope = "member", Target = "~M:HarmonyLib.Sandbox.GetStructReplacement(HarmonyLib.Sandbox,System.IntPtr,System.IntPtr,System.IntPtr)")]
diff --git a/Harmony/Internal/MethodPatcher.cs b/Harmony/Internal/MethodPatcher.cs
index 22948e0d..ea933816 100644
--- a/Harmony/Internal/MethodPatcher.cs
+++ b/Harmony/Internal/MethodPatcher.cs
@@ -29,7 +29,6 @@ internal class MethodPatcher
 		readonly List<MethodInfo> finalizers;
 		readonly int idx;
 		readonly bool firstArgIsReturnBuffer;
-		readonly OpCode Ldarg_instance;
 		readonly Type returnType;
 		readonly DynamicMethodDefinition patch;
 		readonly ILGenerator il;
@@ -58,10 +57,9 @@ internal MethodPatcher(MethodBase original, MethodBase source, List<MethodInfo>
 
 			idx = prefixes.Count() + postfixes.Count() + finalizers.Count();
 			firstArgIsReturnBuffer = NativeThisPointer.NeedsNativeThisPointerFix(original);
-			Ldarg_instance = firstArgIsReturnBuffer ? OpCodes.Ldarg_1 : OpCodes.Ldarg_0;
-			if (debug && firstArgIsReturnBuffer) FileLog.Log($"### Special case: Ldarg.0 is return buffer, not instance!");
+			if (debug && firstArgIsReturnBuffer) FileLog.Log($"### Special case: extra argument after 'this' is pointer to valuetype (simulate return value)");
 			returnType = AccessTools.GetReturnedType(original);
-			patch = CreateDynamicMethod(original, $"_Patch{idx}");
+			patch = CreateDynamicMethod(original, $"_Patch{idx}", debug);
 			if (patch == null)
 				throw new Exception("Could not create replacement method");
 
@@ -107,7 +105,7 @@ internal MethodInfo CreateReplacement(out Dictionary<int, CodeInstruction> final
 			}
 
 			if (firstArgIsReturnBuffer)
-				emitter.Emit(OpCodes.Ldarg_1); // load ref to return value
+				emitter.Emit(original.IsStatic ? OpCodes.Ldarg_0 : OpCodes.Ldarg_1); // load ref to return value
 
 			var skipOriginalLabel = il.DefineLabel();
 			var canHaveJump = AddPrefixes(privateVars, skipOriginalLabel);
@@ -116,7 +114,12 @@ internal MethodInfo CreateReplacement(out Dictionary<int, CodeInstruction> final
 			foreach (var transpiler in transpilers)
 				copier.AddTranspiler(transpiler);
 			if (firstArgIsReturnBuffer)
-				copier.AddTranspiler(NativeThisPointer.m_ArgumentShiftTranspiler);
+			{
+				if (original.IsStatic)
+					copier.AddTranspiler(NativeThisPointer.m_ArgumentShiftTranspilerStatic);
+				else
+					copier.AddTranspiler(NativeThisPointer.m_ArgumentShiftTranspilerInstance);
+			}
 
 			var endLabels = new List<Label>();
 			copier.Finalize(emitter, endLabels, out var endingReturn);
@@ -197,7 +200,7 @@ internal MethodInfo CreateReplacement(out Dictionary<int, CodeInstruction> final
 			return patch.Generate().Pin();
 		}
 
-		internal static DynamicMethodDefinition CreateDynamicMethod(MethodBase original, string suffix)
+		internal static DynamicMethodDefinition CreateDynamicMethod(MethodBase original, string suffix, bool debug)
 		{
 			if (original == null) throw new ArgumentNullException(nameof(original));
 			var patchName = original.Name + suffix;
@@ -205,6 +208,11 @@ internal static DynamicMethodDefinition CreateDynamicMethod(MethodBase original,
 
 			var parameters = original.GetParameters();
 			var parameterTypes = parameters.Types().ToList();
+
+			var firstArgIsReturnBuffer = NativeThisPointer.NeedsNativeThisPointerFix(original);
+			if (firstArgIsReturnBuffer)
+				parameterTypes.Insert(0, typeof(IntPtr));
+
 			if (original.IsStatic == false)
 			{
 				if (AccessTools.IsStruct(original.DeclaringType))
@@ -213,9 +221,6 @@ internal static DynamicMethodDefinition CreateDynamicMethod(MethodBase original,
 					parameterTypes.Insert(0, original.DeclaringType);
 			}
 
-			var firstArgIsReturnBuffer = NativeThisPointer.NeedsNativeThisPointerFix(original);
-			if (firstArgIsReturnBuffer)
-				parameterTypes.Insert(0, typeof(IntPtr));
 			var returnType = firstArgIsReturnBuffer ? typeof(void) : AccessTools.GetReturnedType(original);
 
 			var method = new DynamicMethodDefinition(
@@ -231,9 +236,9 @@ internal static DynamicMethodDefinition CreateDynamicMethod(MethodBase original,
 #else
 			var offset = (original.IsStatic ? 0 : 1) + (firstArgIsReturnBuffer ? 1 : 0);
 			if (firstArgIsReturnBuffer)
-				method.Definition.Parameters[0].Name = "retbuf";
+				method.Definition.Parameters[original.IsStatic ? 0 : 1].Name = "retbuf";
 			if (!original.IsStatic)
-				method.Definition.Parameters[firstArgIsReturnBuffer ? 1 : 0].Name = "this";
+				method.Definition.Parameters[0].Name = "this";
 			for (var i = 0; i < parameters.Length; i++)
 			{
 				var param = method.Definition.Parameters[i + offset];
@@ -331,7 +336,6 @@ void EmitCallParameter(MethodInfo patch, Dictionary<string, LocalBuilder> variab
 			var isInstance = original.IsStatic == false;
 			var originalParameters = original.GetParameters();
 			var originalParameterNames = originalParameters.Select(p => p.Name).ToArray();
-			var firstArgIsReturnBuffer = NativeThisPointer.NeedsNativeThisPointerFix(original);
 
 			// check for passthrough using first parameter (which must have same type as return type)
 			var parameters = patch.GetParameters().ToList();
@@ -359,11 +363,11 @@ void EmitCallParameter(MethodInfo patch, Dictionary<string, LocalBuilder> variab
 						var parameterIsRef = patchParam.ParameterType.IsByRef;
 						if (instanceIsRef == parameterIsRef)
 						{
-							emitter.Emit(Ldarg_instance);
+							emitter.Emit(OpCodes.Ldarg_0);
 						}
 						if (instanceIsRef && parameterIsRef == false)
 						{
-							emitter.Emit(Ldarg_instance);
+							emitter.Emit(OpCodes.Ldarg_0);
 							emitter.Emit(OpCodes.Ldobj, original.DeclaringType);
 						}
 						if (instanceIsRef == false && parameterIsRef)
@@ -396,7 +400,7 @@ void EmitCallParameter(MethodInfo patch, Dictionary<string, LocalBuilder> variab
 						emitter.Emit(patchParam.ParameterType.IsByRef ? OpCodes.Ldsflda : OpCodes.Ldsfld, fieldInfo);
 					else
 					{
-						emitter.Emit(Ldarg_instance);
+						emitter.Emit(OpCodes.Ldarg_0);
 						emitter.Emit(patchParam.ParameterType.IsByRef ? OpCodes.Ldflda : OpCodes.Ldfld, fieldInfo);
 					}
 					continue;
diff --git a/Harmony/Internal/NativeThisPointerCheck.cs b/Harmony/Internal/NativeThisPointerCheck.cs
index 5f7e5514..9f4e8c2f 100644
--- a/Harmony/Internal/NativeThisPointerCheck.cs
+++ b/Harmony/Internal/NativeThisPointerCheck.cs
@@ -8,10 +8,43 @@
 namespace HarmonyLib
 {
 	// A test for https://github.com/dotnet/coreclr/blob/master/Documentation/botr/clr-abi.md
-	//
+
+	internal class Sandbox
+	{
+		internal static bool hasNativeThis;
+		internal static readonly IntPtr magicValue = (IntPtr)0x12345678;
+
+		internal struct SomeStruct
+		{
+#pragma warning disable CS0169
+			readonly byte b1;
+			readonly byte b2;
+			readonly byte b3;
+#pragma warning restore CS0169
+		}
+
+		[MethodImpl(MethodImplOptions.NoInlining)]
+		internal SomeStruct GetStruct(IntPtr x, IntPtr y)
+		{
+			throw new Exception("This method should've been detoured!");
+		}
+
+		internal static void GetStructReplacement(Sandbox self, IntPtr ptr, IntPtr a, IntPtr b)
+		{
+			// Normal argument order:
+			// this, a, b
+
+			// If we have a native return buffer pointer, the order is:
+			// this, ptr, a, b
+
+			hasNativeThis = a == magicValue && b == magicValue;
+		}
+	}
+
 	internal class NativeThisPointer
 	{
-		internal static MethodInfo m_ArgumentShiftTranspiler = SymbolExtensions.GetMethodInfo(() => ArgumentShiftTranspiler(null));
+		internal static MethodInfo m_ArgumentShiftTranspilerStatic = SymbolExtensions.GetMethodInfo(() => ArgumentShiftTranspiler_Static(null));
+		internal static MethodInfo m_ArgumentShiftTranspilerInstance = SymbolExtensions.GetMethodInfo(() => ArgumentShiftTranspiler_Instance(null));
 		static readonly Dictionary<Type, int> sizes = new Dictionary<Type, int>();
 
 		static int SizeOf(Type type)
@@ -39,59 +72,48 @@ internal static bool NeedsNativeThisPointerFix(MethodBase method)
 			return HasNativeThis();
 		}
 
-		static readonly IntPtr magicValue = (IntPtr)0x12345678;
-		static bool hasTestResult, hasNativeThis;
+		internal static bool hasTestResult;
 		static bool HasNativeThis()
 		{
 			if (hasTestResult == false)
 			{
-				hasNativeThis = false;
+				Sandbox.hasNativeThis = false;
 				var self = new NativeThisPointer();
-				var original = AccessTools.DeclaredMethod(typeof(NativeThisPointer), "GetStruct");
-				var replacement = AccessTools.DeclaredMethod(typeof(NativeThisPointer), "GetStructReplacement");
+				var original = AccessTools.DeclaredMethod(typeof(Sandbox), nameof(Sandbox.GetStruct));
+				var replacement = AccessTools.DeclaredMethod(typeof(Sandbox), nameof(Sandbox.GetStructReplacement));
 				_ = Memory.DetourMethod(original, replacement);
-				_ = self.GetStruct(magicValue, magicValue);
+				_ = new Sandbox().GetStruct(Sandbox.magicValue, Sandbox.magicValue);
 				hasTestResult = true;
 			}
-			return hasNativeThis;
+			return Sandbox.hasNativeThis;
 		}
 
-		struct SomeStruct
+		private static IEnumerable<CodeInstruction> ArgumentShiftTranspiler_Static(IEnumerable<CodeInstruction> instructions)
 		{
-#pragma warning disable IDE0051
-#pragma warning disable CS0169
-			readonly byte b1;
-			readonly byte b2;
-			readonly byte b3;
-#pragma warning restore CS0169
-#pragma warning restore IDE0051
+			return ArgumentShifter(instructions, true);
 		}
 
-		[MethodImpl(MethodImplOptions.NoInlining)]
-#pragma warning disable IDE0060
-		SomeStruct GetStruct(IntPtr x, IntPtr y)
-#pragma warning restore IDE0060
+		private static IEnumerable<CodeInstruction> ArgumentShiftTranspiler_Instance(IEnumerable<CodeInstruction> instructions)
 		{
-			throw new Exception("This method should've been detoured!");
+			return ArgumentShifter(instructions, false);
 		}
 
-#pragma warning disable IDE0060
-#pragma warning disable IDE0051
-		static void GetStructReplacement(NativeThisPointer self, IntPtr ptr, IntPtr a, IntPtr b)
-#pragma warning restore IDE0051
-#pragma warning restore IDE0060
+		private static IEnumerable<CodeInstruction> ArgumentShifter(IEnumerable<CodeInstruction> instructions, bool methodIsStatic)
 		{
-			// Normal argument order:
-			// this, a, b
+			// We have two cases:
+			//
+			// Case A: instance method
+			// THIS , IntPtr , arg0 , arg1 , arg2 ...
+			//
+			// So we make space at index 1 by moving all Ldarg_[n] to Ldarg_[n+1]
+			// except Ldarg_0 which stays at positon #0
+			//
+			// Case B: static method
+			// IntPtr , arg0 , arg1 , arg2 ...
+			//
+			// So we make space at index 0 by moving all Ldarg_[n] to Ldarg_[n+1]
+			// including Ldarg_0
 
-			// If we have a native return buffer pointer, the order is:
-			// this, ptr, a, b
-
-			hasNativeThis = a == magicValue && b == magicValue;
-		}
-
-		private static IEnumerable<CodeInstruction> ArgumentShiftTranspiler(IEnumerable<CodeInstruction> instructions)
-		{
 			foreach (var instruction in instructions)
 			{
 				if (instruction.opcode == OpCodes.Ldarg_3)
@@ -116,11 +138,14 @@ private static IEnumerable<CodeInstruction> ArgumentShiftTranspiler(IEnumerable<
 					continue;
 				}
 
-				if (instruction.opcode == OpCodes.Ldarg_0)
+				if (methodIsStatic)
 				{
-					instruction.opcode = OpCodes.Ldarg_1;
-					yield return instruction;
-					continue;
+					if (instruction.opcode == OpCodes.Ldarg_0)
+					{
+						instruction.opcode = OpCodes.Ldarg_1;
+						yield return instruction;
+						continue;
+					}
 				}
 
 				if (instruction.opcode == OpCodes.Ldarg
@@ -130,9 +155,12 @@ private static IEnumerable<CodeInstruction> ArgumentShiftTranspiler(IEnumerable<
 					|| instruction.opcode == OpCodes.Starg_S)
 				{
 					var n = (int)instruction.operand;
-					instruction.operand = n + 1;
-					yield return instruction;
-					continue;
+					if (n > 0 || methodIsStatic)
+					{
+						instruction.operand = n + 1;
+						yield return instruction;
+						continue;
+					}
 				}
 
 				yield return instruction;
diff --git a/Harmony/Public/PatchProcessor.cs b/Harmony/Public/PatchProcessor.cs
index 9ff413d4..57423b80 100644
--- a/Harmony/Public/PatchProcessor.cs
+++ b/Harmony/Public/PatchProcessor.cs
@@ -244,7 +244,7 @@ public static Dictionary<string, Version> VersionInfo(out Version currentVersion
 		/// <returns>A list containing all the original CodeInstructions</returns>
 		public static List<CodeInstruction> GetOriginalInstructions(MethodBase original, ILGenerator generator = null)
 		{
-			var patch = MethodPatcher.CreateDynamicMethod(original, $"_Copy{Guid.NewGuid()}");
+			var patch = MethodPatcher.CreateDynamicMethod(original, $"_Copy{Guid.NewGuid()}", Harmony.DEBUG);
 			generator = generator ?? patch.GetILGenerator();
 			var reader = MethodBodyReader.GetInstructions(generator, original);
 			return reader.Select(ins => ins.GetCodeInstruction()).ToList();
@@ -256,7 +256,7 @@ public static List<CodeInstruction> GetOriginalInstructions(MethodBase original,
 		/// <returns>A list containing all the original CodeInstructions</returns>
 		public static List<CodeInstruction> GetOriginalInstructions(MethodBase original, out ILGenerator generator)
 		{
-			var patch = MethodPatcher.CreateDynamicMethod(original, $"_Copy{Guid.NewGuid()}");
+			var patch = MethodPatcher.CreateDynamicMethod(original, $"_Copy{Guid.NewGuid()}", Harmony.DEBUG);
 			generator = patch.GetILGenerator();
 			var reader = MethodBodyReader.GetInstructions(generator, original);
 			return reader.Select(ins => ins.GetCodeInstruction()).ToList();
diff --git a/HarmonyTests/IL/TestMethodBodyReader.cs b/HarmonyTests/IL/TestMethodBodyReader.cs
index 2e992601..7d164116 100644
--- a/HarmonyTests/IL/TestMethodBodyReader.cs
+++ b/HarmonyTests/IL/TestMethodBodyReader.cs
@@ -16,7 +16,7 @@ public void Test_CanGetInstructionsWithNoILGenerator()
 			var method = typeof(Class12).GetMethod(nameof(Class12.FizzBuzz));
 			var instrsNoGen = MethodBodyReader.GetInstructions(generator: null, method);
 
-			var dynamicMethod = MethodPatcher.CreateDynamicMethod(method, "_Patch");
+			var dynamicMethod = MethodPatcher.CreateDynamicMethod(method, "_Patch", false);
 			var instrsHasGen = MethodBodyReader.GetInstructions(dynamicMethod.GetILGenerator(), method);
 
 			Assert.AreEqual(instrsNoGen.Count, instrsHasGen.Count);