Skip to content

Commit

Permalink
Fixed IntPtr result buffers for real
Browse files Browse the repository at this point in the history
  • Loading branch information
@pardeike
pardeike committed Feb 10, 2020
1 parent c6ef002 commit 6c7e2b0
Show file tree
Hide file tree
Showing 5 changed files with 101 additions and 64 deletions.
7 changes: 6 additions & 1 deletion Harmony/GlobalSuppressions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
[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)")]
36 changes: 20 additions & 16 deletions Harmony/Internal/MethodPatcher.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -197,14 +200,19 @@ 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;
patchName = patchName.Replace("<>", "");

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))
Expand All @@ -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(
Expand All @@ -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];
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down
116 changes: 72 additions & 44 deletions Harmony/Internal/NativeThisPointerCheck.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions Harmony/Public/PatchProcessor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion HarmonyTests/IL/TestMethodBodyReader.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 6c7e2b0

Please sign in to comment.