BepInEx / HarmonyX

Harmony built on top of MonoMod.RuntimeDetours with additional features
MIT License
329 stars 42 forks source link

Wrong decode Bge_Un_S, decoding as Bge_Un ... #69

Open deltaone opened 1 year ago

deltaone commented 1 year ago

2023-03-26_222846 2023-03-26_222902

Use this patch function, original Harmony - ok, HarmonyX - failed ... After debugger - HarmonyX decode Bge_Un_S as Bge_Un ...


        static IEnumerable<CodeInstruction> Transpiler(IEnumerable<CodeInstruction> instructions)
        {           
            return new CodeMatcher(instructions)
                .MatchEndForward(
                    new CodeMatch(i => i.opcode == OpCodes.Ldsfld  && ((FieldInfo)i.operand).Name == "worldSurface"),
//                  new CodeMatch(OpCodes.Bge_Un_S),
                    new CodeMatch(OpCodes.Bge_Un),
                    new CodeMatch(OpCodes.Ret))
                .ThrowIfInvalid("MinecartDiggerHelper.TryDigging(): Error! Can't find pattern!")
                .SetAndAdvance(OpCodes.Nop, null)
                .InstructionEnumeration();
        }
Neutron3529 commented 1 year ago

this might be feature since Bge_Un_S and Bge_Un do the same thing.

ManlyMarco commented 1 year ago

It's on purpose. I don't remember what the reasoning was but it was solid, Horse should know. https://github.com/BepInEx/HarmonyX/blob/46b77d08eac32f8d17f0cbe761bd74cd2be21ff7/Harmony/Internal/Patching/ILManipulator.cs#LL25C64-L25C64

RoboPhred commented 12 months ago

I'm running into this same issue. I was hoping to convince a project to switch to HarmonyX but since it seems HX rewrites instructions its breaking transpilers of other stuff built with it...

Would you be willing to accept making NormalizeInstructions an optional step that can be disabled?

ManlyMarco commented 12 months ago

Sadly it looks like Horse is on a hiatus so someone else that knows what's up will have to look into this. I'm worried about changing this in case it breaks existing mods, since I assume that fixing some edge case is why the commit happened in the first place. It'd be nice if there was an easy way to compare results between fix and no fix in bulk and do it for a bunch of games...