Writing an empty Transpiler changes the behavior of the patched method

[Meivyn]

There is currently a bug in Harmony that makes a method behave differently while being patched by a transpiler. This issue was initially encountered by @Aeroluna.

It doesn't have to be empty, but I didn't know how to better express this issue. Feel free to edit the title.

Reproduce steps

1. Install Beat Saber and BSIPA.

2. Make a mod that contains this transpiler returning unchanged instructions.

   [HarmonyPatch(typeof(BeatmapDataLoader), "GetBeatmapDataFromBeatmapSaveData")]
   internal class BeatmapDataLoaderGetBeatmapDataFromBeatmapSaveDataPatch
   {
       private static IEnumerable<CodeInstruction> Transpiler(IEnumerable<CodeInstruction> instructions)
       {
           return instructions;
       }
   }

3. Add a postfix that logs something when the offending method in the transpiled method runs.

   [HarmonyPatch(typeof(DefaultEnvironmentEventsFactory), nameof(DefaultEnvironmentEventsFactory.InsertDefaultEnvironmentEvents))]
   internal class DefaultEnvironmentEventsFactoryInsertDefaultEnvironmentEventsPatch
   {
       private static void Postfix()
       {
           Plugin.Log.Notice("InsertDefaultEnvironmentEvents");
       }
   }

4. Put the mod into Beat Saber\Plugins.

5. Run the game and start a level.

Alternatively, use the attached plugin: BSPlugin1.zip

Expected behavior

The DefaultEnvironmentEventsFactory.InsertDefaultEnvironmentEvents method shouldn't run and the postfix shouldn't log anything.

Actual behavior

The DefaultEnvironmentEventsFactory.InsertDefaultEnvironmentEvents method runs and the postfix is logging.

Environment

• OS: Windows 11
• .NET environment: Unity 2019.4.28 Mono
• HarmonyX version: v2.7.0 to latest (v2.10.1 as of this writing)

Additional information

I made MonoMod dumps for the unpatched and patched method.

Here's a diff of the patched method, after applying the transpiler:

A workaround has been found, which consists to add a Nop instruction after the Leave instruction. By using the same workaround on this test case, the resulting method is exactly the same as the original, according to the dumps.

[Windows10CE]

How are you getting a MonoMod dump for the method without patching it? Are there other mods at play here? It seems very odd that adding a Nop inside a finally block would change a branch instruction halfway up the IL. I would try to investigate this myself, but that's a bit difficult without a VR headset to use.

[Meivyn]

Sorry, I meant "unpatched" as in "before patch". MonoMod dumps both when Harmony patches a method.

To make these dumps, I didn't use any mod other than the provided one.

You also don't need to have a headset, you can launch the game using fpfc in its arguments, which would allow you to control the game with the mouse. Still need to buy it though, which might be a bit pointless without a headset.

[ManlyMarco]

Can you turn on full harmony debug logging and send the log? Not sure how it's done in that launcher. Adding [HarmonyDebug] attribute to the patches might be enough.

[Meivyn]

I suppose that you are talking about IL debug? Then here you go _latest.log

[mgreter]

We are experiencing the same issue after 7 Days to Die updated HarmonyX with A21 to 2.10.0.0.
I don't have much to add yet. Trying to see if I can find more info to add to this issue here later.
FWIW here the full reproduction case for 7 Days to Die A21 (b317):

[HarmonyPatch(typeof(ItemClassesFromXml), "parseItem")]
static class ItemClassesFromXmlParseItem
{
    static IEnumerable<CodeInstruction> Transpiler(IEnumerable<CodeInstruction> instructions) => instructions;
    static void Postfix() => Log.Out("++ Postfix for ItemClassesFromXml.parseItem called");
    static void Prefix() => Log.Out("++ Prefix for ItemClassesFromXml.parseItem called");
}

Pre/Postfix still work as expected, but when transpiler patch is active, game will throw in the original method!

[mgreter]

Here are some log files from adding the [HarmonyDebug] flag.

With transpiler enabled: patch.bad.log (crash) and only with pre/postfix: patch.ok.log (working).

Beside some address changes, there were a few additional OPs added in the bad version related to try/catch!?

Makes sense as the original code is running into an (unexpected) exception with the empty transpiler patch.

[mgreter]

After some try and error I found at least where to be the culprit seems to be at.
I basically disabled applying the patches, which still errors when a transpiler is there:

private void WriteTranspilers()
{
	if (transpilers.Count == 0)
		return;

	Logger.Log(Logger.LogChannel.Info, () => $"Transpiling {original.FullDescription()}", debug);

	// Create a high-level manipulator for the method
	var manipulator = new ILManipulator(ctx.Body, debug);

	// Add in all transpilers
	// foreach (var transpiler in transpilers)
	//	 manipulator.AddTranspiler(transpiler.method);

	// Write new manipulated code to our body
	manipulator.WriteTo(ctx.Body, original);
}

Since this crashed, I went into the ILManipulator.WriteTo.

If I disable this completely (e.g. return early), all works as expected.
So looks to me that somewhere in there something isn't working 100%.

[mgreter]

So finally it seems I found the specifics that cause it to fail for the given example

// We need to handle exception handler opcodes specially because ILProcessor emits them automatically
// Case 1: leave + start or end of exception block => ILProcessor generates leave automatically
if ((cur.opcode == SRE.OpCodes.Leave || cur.opcode == SRE.OpCodes.Leave_S) &&
		(cur.blocks.Count > 0 || next?.blocks.Count > 0))
	goto mark_block;

When this part is disabled, it seems the original functions works again as expected.
Of course I have no idea why it was added in the first place, so probably not a proper fix!

I also dumped the ILs when entering: il.before.log and when exiting il.after.unpatched.log.
And when I apply my crude fix patch il.after.patched.log.

[mgreter]

FWIW I've added an alternative cleanup for duplicate leave jumps:

for (int i = 0; i < body.Instructions.Count - 1; i++)
{
	// Find two leave instructions in a row to clean up
	if (body.Instructions[i].OpCode != OpCodes.Leave &&
			body.Instructions[i].OpCode != OpCodes.Leave_S) continue;
	if (body.Instructions[i + 1].OpCode != OpCodes.Leave &&
			body.Instructions[i + 1].OpCode != OpCodes.Leave_S) continue;
	if (body.Instructions[i].Offset != body.Instructions[i + 1].Offset)
		Logger.Log(Logger.LogChannel.Warn, () =>
			"Found consecutive leave ops that don't agree");
	body.Instructions.RemoveAt(i + 1);
}

Now the IL code seems to agree where to jump after patching vs original.
Again, not sure what the original intent was to remove the original "leaves".
But it seems the ones inserted by ILProcessor are wrong sometimes!?

Hope with all this info we can get a proper fix in a reasonable time frame 🤞

[ManlyMarco]

@ghorsington added the leave hack in 506d4d2, if I remember correctly it was necessary to fix compatibility with certain transpliers that worked in older versions of harmony. There's probably a discussion related to this on the BepInEx discord server if you care to search the haystack.
He hasn't been active in a while so I wouldn't count on a fast fix unless you can spin up a PR.

[mgreter]

I of course can spin up a PR that fixes my issue, but not 100% sure it would break/regress the other case :-/
For that I don't have enough insight what the proper fix for that would be.
FWIW I've created #77, so let's see 🤞

[Meivyn]

@ghorsington added the leave hack in 506d4d2, if I remember correctly it was necessary to fix compatibility with certain transpliers that worked in older versions of harmony. There's probably a discussion related to this on the BepInEx discord server if you care to search the haystack. He hasn't been active in a while so I wouldn't count on a fast fix unless you can spin up a PR.

The discussion in question: https://discord.com/channels/623153565053222947/624275540844478474/913130139834007582

After digging a lot through it, the issue happens because of MonoMod's implementation of the IL generator. It emits a Leave instruction when calling EndExceptionBlock, then immediately after that, assigns a label to that instruction with MarkLabel, which adds it to a list of pending labels.

When the next instruction is emitted with Emit, it processes the list of pending labels with ProcessLabels which would effectively assign all of them to the current instruction, which might be wrong.

It seems that MonoMod's implementation is designed to be used with multiple Leave.