Writing an empty Transpiler changes the behavior of the patched method about harmonyx HOT 12 CLOSED

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.

from harmonyx.

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.

from harmonyx.

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.

from harmonyx.

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

from harmonyx.

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!

from harmonyx.

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.

from harmonyx.

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%.

from harmonyx.

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.

from harmonyx.

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 🤞

from harmonyx.

@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.

from harmonyx.

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 🤞

from harmonyx.

@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.

from harmonyx.