Update MonoMod to 25.0.0, add Harmony 2.3 features

[Windows10CE]

No description provided.

[ManlyMarco]

I'm not too knowledgeable about the monomod/IL parts that were changed so I'll just leave some comments.

This build doesn't work on BepInEx 5.4.21, preloader fails with

System.TypeLoadException: Could not resolve type with token 0100001b (from typeref, class/assembly MonoMod.Utils.Platform, MonoMod.Utils, Version=22.1.29.1, Culture=neutral, PublicKeyToken=null)
  at BepInEx.Preloader.PreloaderRunner.PreloaderPreMain () [0x00000] in <fc9d7fbc6dcb44cf87be11d8d92ae161>:0
...

[ManlyMarco]

Why was this changed?

[Windows10CE]

ILHookExtensions was using reflection to make MonoMod think something wasn't applied when it was, but I'm not sure that would even work under reorg, so I just got rid of the jank and did Undo + Apply instead (which is what this really should've been doing in the first place)

It's a bit slower, but more "correct", and its not like patching was fast anyway

[ManlyMarco]

Also, cd0a86b is already merged into master as 4e71dc4 so it's not necessary.

[ManlyMarco]

@Windows10CE I didn't merge this PR yet because it still breaks current BepInEx releases.

[ManlyMarco]

I attempted to make BepInEx5 load with this version of HarmonyX but it just hard crashes Unity after I managed to get rid of all the compilation errors BepInEx/BepInEx@ecb4b57
There's also some APIs straight up missing now, like DynDllImportAttribute.

If you (or anyone else) could made a PR for BepInEx to make it work with this PR without regressions then I would be able to merge both.

Edit: Looks like v2.10 also broke BepInEx5 but only in Unity 5.x games with a preloader crash but it did work in later versions.

[nike4613]

This build doesn't work on BepInEx 5.4.21, preloader fails with

System.TypeLoadException: Could not resolve type with token 0100001b (from typeref, class/assembly MonoMod.Utils.Platform, MonoMod.Utils, Version=22.1.29.1, Culture=neutral, PublicKeyToken=null)
  at BepInEx.Preloader.PreloaderRunner.PreloaderPreMain () [0x00000] in <fc9d7fbc6dcb44cf87be11d8d92ae161>:0
...

Looing at this again because somebody else pointed me at it... This looks like the wrong version of Utils. Are you sure you had Reorg's Utils (25.0.0) available to the game?

[kohanis]

From what I've checked:
GetExecutingAssembly fix does not work as intended (what's it even for, tho?)
MonoMod.RuntimeDetour 25.x.x is really broken on net3.5
MonoMod.RuntimeDetour 25.x.x Hook does not work on mono

Tests on net3.5 only pass because forward to latest version 4.5+ fm

[nike4613]

25 should work on .NET 3.5, unless I broke it recently. Mono I'm less sure of, if you can get a testable repro, I can probably fix it pretty quickly.

[kohanis]

@nike4613 https://github.com/kohanis/MonoModTest

[nike4613]

The EvenMoreFakeRandom issue is simply inlining related (.NET 6 and newer don't have an issue in this test because the calling method is not tiered up, so is not compiled with opts), but you're right, .NET 3.5 got broken somewhere along the line. Opened a tracking issue MonoMod/MonoMod#156

[ManlyMarco]

I'll wait and see what happens then. if the fix is going to take a long time I can revert this PR.

[ManlyMarco]

@nike4613 Any updates? If it looks like the fix is going to take time I'll revert this PR so that others can get merged.

[nike4613]

I haven't been working on it much. I'll see what I can figure out today though.

[Meivyn]

If someone is willing to try this: master...Meivyn:HarmonyX:ci#diff-3d39eb790b32ee18c77608a6057e09e25bda2215d6ed915aa75855d5cfbd64fc

I am unsure what this Assembly.GetExecutingAssembly fix is meant for. But since NativeHook implementation doesn't seem to support that, I replaced it with a Core detour that should fix the Unity crash resulting from its use.

[nike4613]

.NET 3.5 is fixed in the reorganize branch now. I've been talking to @Meivyn, and I believe that the reported Mono issues were in fact issues with the implementation in HarmonyX (using NativeHook for detouring a managed method).

[kohanis]

Yes, in my local repo I used detour to fix it.
But I also don't understand what fix is even for, because it basically does nothing

[nike4613]

Uploaded RuntimeDetour 25.1.0-prerelease.2 with the above fix in it.

[ManlyMarco]

@kohanis @Meivyn Can you check if the PR above fixes .Net 3.5 support for you?

[Meivyn]

It fixed .NET 3.5 support from what I could see with the repro case, but it still fails with Mono. More precisely, I found out that it doesn't seem to like the return 0 in the EvenMoreFakeRandom method specifically in release build. Having an empty body with just return seems to cause the same behavior.

I do not have a game that makes use of net35 though, so I can't test on it, and I can't tell how that new bug will affect Harmony. I don't think it would though, considering that this seems rather... specific. And likely relevant to the method's body length, according to @nike4613.

[kohanis]

Original test case works fine on my side with both mono 6.12(+unity mono from 2022.3.9f1) and .net framework 3.5, MonoTest one was originally wrong usage.
@Meivyn Which mono version fails EvenMoreFakeRandom hook?

[Meivyn]

I used the one in Unity 2019.4.28f1. I think it's 5.11?

[kohanis]

Ok, my bad, there is such a thing with short bodies with unity mono (original mono works). But I'm not so optimistic about the rarity of cases
Also, found hard crash with mono <=5.4.1 and a case of NotImplementedException in unity 2017 and 5.2.0 mono, will report it later (I love NotImplementedException in production btw)
// In short: still problems with unity <=2017

[kohanis]

MonoMod/MonoMod#160
MonoMod/MonoMod#161

[ManlyMarco]

Thanks, I'll wait for those to be resolved then.

(I love NotImplementedException in production btw)

I think it sums Unity up pretty well :)

[Meivyn]

Reverse patches seem to be broken somehow. This patch is failing on Harmony 2.11.0, and works fine on 2.10.2: https://github.com/Aeroluna/Heck/blob/master/Heck/HarmonyPatches/PlayViewInterrupter.cs#L76

It throws the exception.

[ManlyMarco]

Reverse patches seem to be broken somehow. This patch is failing on Harmony 2.11.0, and works fine on 2.10.2: https://github.com/Aeroluna/Heck/blob/master/Heck/HarmonyPatches/PlayViewInterrupter.cs#L76

It throws the exception.

Details? Maybe it adds IL now?

[Meivyn]

Problem is that I don't have more details. There's no error nor anything in Harmony logs that could help me figure it out. I'm not familiar with reverse patches, but that NotImplementedException at that line is now thrown, and it was not before the update.

[Meivyn]

Why this was released without a prior fix of Assembly.GetExecutingAssembly? And no one was able to look into the reverse patches issue?

[ManlyMarco]

Reverse patchers appear to work fine so we'd need an exact setup to reproduce the issue.

[Meivyn]

Actually the issue might've been caused by my attempt at fixing the Assembly.GetExecutingAssembly hook? BSIPA isn't able to launch without a fix for that, Unity just hard crashes, so it's hard to tell as I can't try without it. I don't know how reverse patches work, it might depend on that.

Here is my attempt at fixing it again, if you don't mind trying: master...Meivyn:HarmonyX:ci#diff-3d39eb790b32ee18c77608a6057e09e25bda2215d6ed915aa75855d5cfbd64fc

I did try to make a test case, my patch looks like this:

[HarmonyPatch(typeof(MainMenuViewController), "HandleMenuButton")]
internal class Patch
{
    [HarmonyReversePatch]
    private static void ReversePatch(MainMenuViewController.MenuButton menuButton)
    {
        throw new NotImplementedException("Reverse patch has not been executed.");

        IEnumerable<CodeInstruction> Transpiler(IEnumerable<CodeInstruction> instructions)
        {
            return instructions;
        }
    }
}

And it throws this when applying the patch, so I can't go further:

HarmonyLib.HarmonyException: IL Compile Error (unknown location) ---> System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
  at Mono.Collections.Generic.Collection`1[T].get_Item (System.Int32 index) [0x00009] in <1f38765308444399b8db490e5065ff6f>:0
  at MonoMod.Utils.Cil.CecilILGenerator._EmitInlineVar (Mono.Cecil.Cil.OpCode opcode, System.Int32 index) [0x0004c] in <119a8e5581064dd58d95959152c2aa57>:0
  at MonoMod.Utils.Cil.CecilILGenerator.Emit (System.Reflection.Emit.OpCode opcode, System.Byte arg) [0x0001d] in <119a8e5581064dd58d95959152c2aa57>:0
  at (wrapper dynamic-method) MonoMod.Utils.DynamicMethodDefinition.EmitOpcodeWithOperand(MonoMod.Utils.Cil.CecilILGenerator,System.Reflection.Emit.OpCode,object)
  at (wrapper delegate-invoke) System.Action`3[MonoMod.Utils.Cil.CecilILGenerator,System.Reflection.Emit.OpCode,System.Object].invoke_void_T1_T2_T3(MonoMod.Utils.Cil.CecilILGenerator,System.Reflection.Emit.OpCode,object)
  at HarmonyLib.Internal.Util.EmitterExtensions.Emit (MonoMod.Utils.Cil.CecilILGenerator il, System.Reflection.Emit.OpCode opcode, System.Object operand) [0x00000] in <0d9c13022e544ce89c806f0f14d8e072>:0
  at HarmonyLib.Internal.Patching.ILManipulator.WriteTo (Mono.Cecil.Cil.MethodBody body, System.Reflection.MethodBase original) [0x0017a] in <0d9c13022e544ce89c806f0f14d8e072>:0
  at HarmonyLib.PatchFunctions+<>c__DisplayClass3_0.<ReversePatch>b__1 (MonoMod.Cil.ILContext ctx) [0x0016f] in <0d9c13022e544ce89c806f0f14d8e072>:0
  at MonoMod.Cil.ILContext.Invoke (MonoMod.Cil.ILContext+Manipulator manip) [0x00092] in <119a8e5581064dd58d95959152c2aa57>:0
  at MonoMod.RuntimeDetour.DetourManager+ManagedDetourState.InvokeManipulator (MonoMod.RuntimeDetour.DetourManager+ILHookEntry entry, Mono.Cecil.MethodDefinition def) [0x0001c] in <667886c0b83048cbaa73a76b37c17c40>:0
  at MonoMod.RuntimeDetour.DetourManager+ManagedDetourState.UpdateEndOfChain () [0x0008b] in <667886c0b83048cbaa73a76b37c17c40>:0
  at MonoMod.RuntimeDetour.DetourManager+ManagedDetourState.AddILHook (MonoMod.RuntimeDetour.DetourManager+SingleILHookState ilhook, System.Boolean takeLock) [0x00079] in <667886c0b83048cbaa73a76b37c17c40>:0
  at MonoMod.RuntimeDetour.ILHook.Apply () [0x00052] in <667886c0b83048cbaa73a76b37c17c40>:0
  at HarmonyLib.PatchFunctions.ReversePatch (HarmonyLib.HarmonyMethod standin, System.Reflection.MethodBase original, System.Reflection.MethodInfo postTranspiler, System.Reflection.MethodInfo postManipulator) [0x0016e] in <0d9c13022e544ce89c806f0f14d8e072>:0
   --- End of inner exception stack trace ---
  at HarmonyLib.PatchClassProcessor.ReportException (System.Exception exception, System.Reflection.MethodBase original) [0x00045] in <0d9c13022e544ce89c806f0f14d8e072>:0
  at HarmonyLib.PatchClassProcessor.Patch () [0x00095] in <0d9c13022e544ce89c806f0f14d8e072>:0
  at HarmonyLib.Harmony.<PatchAll>b__11_0 (System.Type type) [0x00007] in <0d9c13022e544ce89c806f0f14d8e072>:0
  at HarmonyLib.CollectionExtensions.Do[T] (System.Collections.Generic.IEnumerable`1[T] sequence, System.Action`1[T] action) [0x00014] in <0d9c13022e544ce89c806f0f14d8e072>:0
  at HarmonyLib.Harmony.PatchAll (System.Reflection.Assembly assembly) [0x00006] in <0d9c13022e544ce89c806f0f14d8e072>:0
  at HarmonyLib.Harmony.CreateAndPatchAll (System.Reflection.Assembly assembly, System.String harmonyInstanceId) [0x0006a] in <0d9c13022e544ce89c806f0f14d8e072>:0
  at BSPlugin5.Plugin..ctor (IPA.Logging.Logger logger, IPA.Loader.PluginMetadata metadata) [0x0000c] in C:\Users\Meivyn\RiderProjects\BSPlugin5\BSPlugin5\Plugin.cs:22
  at (wrapper dynamic-method) System.Object.lambda_method(System.Runtime.CompilerServices.Closure,IPA.Loader.PluginMetadata)
  at IPA.Loader.PluginExecutor.Create () [0x00009] in C:\Users\Meivyn\repos\BeatSaber-IPA-Reloaded\IPA.Loader\Loader\PluginExecutor.cs:57
  at IPA.Loader.PluginLoader.InitPlugin (IPA.Loader.PluginMetadata meta, System.Collections.Generic.IEnumerable`1[T] alreadyLoaded) [0x001ec] in C:\Users\Meivyn\repos\BeatSaber-IPA-Reloaded\IPA.Loader\Loader\PluginLoader.cs:957