BepInEx / HarmonyX

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

Patch does not run if specific other patch exists #71

Open SpikeHimself opened 1 year ago

SpikeHimself commented 1 year ago

Apologies in advance for the vague description. This is a mind boggling issue for me (I'm new to this) and I'm not even sure if Harmony is at fault. I'm hoping someone can point me in the right direction with this.

Consider this pseudo code:

void DoThings()
{
    OneThing();
    AnotherThing();
}

If I patch OneThing, that works as expected. Prefix, postfix, finalizer, all work fine.

Now, if I also patch DoThings, that works, but any patch for OneThing will now be skipped. That is to say, the debug logs say that the method did get patched, but the code does not run. It does not matter if it is a prefix, postfix, or finalizer. None of them run.

In fact, if the patch on DoThings is empty, like so:

void Prefix() { }

Obviously it will now not do anything, and yet it still breaks the other patch.

Is this expected behaviour?

The context in which I discovered this is the Unity game Valheim, and the bug (if it is one) happens in a mod I wrote, but was initially caused by another mod. Meaning, my mod had patched OneThing and that no longer worked because another mod had patched DoThings.

How can I debug this and figure out what's actually going on "behind the scenes"?

ManlyMarco commented 1 year ago

Try putting [HarmonyDebug] attributes on your outer prefix and see what the IL comes out like.

SpikeHimself commented 1 year ago

Thanks for your suggestion @ManlyMarco. It seems that the method is unchanged (as you'd expect with an empty prefix). I don't know what the IL would look like without the prefix existing (I'm not sure how to find out), but I can see that the call to the inner method is still there. (edit: I've used JetBrains dotPeek to view the original IL, and apart from differences in output formatting, and the call to my prefix in the Harmony output, the two are equal)

If someone reported this to me I'd say they were crazy and there must be different circumstances that caused the inner method not to be called. Yet here we are, I can reproduce this behaviour with 100% consistency :(

Working with IL isn't my favourite hobby (another rabbit hole!) but I suppose the next step is diving in with a transpiler, to figure out what the outer method is doing exactly? If that can be avoided I'm all for it..

The original method is a bit larger than the pseudo code example I provided, and it does some checks before calling the inner method. However, the mere existence of another patch surely wouldn't affect that..

SpikeHimself commented 1 year ago

Small update: with a crazy work-around I've been able to determine that the method I want to patch does run (in short: I was able to check for a value that it sets). So it really is just the patch that does not run.

(not entirely coincidentally I can use this same work-around to achieve what I wrote the patch for, so for my mod the issue is solved I think)

SpikeHimself commented 1 year ago

The plot thickens..

If I change the order in which I write the patches, both work.

In this example, only the outer patch is called:

    // this is the outer method
    [HarmonyPatch(typeof(Player), nameof(Player.PlacePiece))]
    static class Player_PlacePiece
    {
        static void Postfix()
        {
            // This runs
            Log.Debug("Player.PlacePiece.Postfix");
        }
    }

    // this is the inner method
    [HarmonyPatch(typeof(WearNTear), nameof(WearNTear.OnPlaced))]
    static class WearNTear_OnPlaced
    {
        static void Postfix()
        {
            // This does not run
            Log.Debug("WearNTear.OnPlaced.Postfix");
        }
    }

But here, the other way around, both run:

    // this is the inner method
    [HarmonyPatch(typeof(WearNTear), nameof(WearNTear.OnPlaced))]
    static class WearNTear_OnPlaced
    {
        static void Postfix()
        {
            // This runs!
            Log.Debug("WearNTear.OnPlaced.Postfix");
        }
    }

    // this is the outer method
    [HarmonyPatch(typeof(Player), nameof(Player.PlacePiece))]
    static class Player_PlacePiece
    {
        static void Postfix()
        {
            // This runs
            Log.Debug("Player.PlacePiece.Postfix");
        }
    }

Is there a way I can control which patch is applied first, bearing in mind that the outer patch might come from another mod that I have no control over?

ManlyMarco commented 1 year ago

Can you try the same but with type-wide patching instead of assembly-wide? https://github.com/BepInEx/HarmonyX/wiki/Patching-with-Harmony#attribute-based-patching-for-types It sounds like the outer patch still points to the original method instead of the patched method, somehow.

SpikeHimself commented 1 year ago

Okay, I now run this:

patcher.PatchAll(typeof(Player_PlacePiece));  // outer
patcher.PatchAll(typeof(WearNTear_OnPlaced));  // inner 

instead of

patcher.PatchAll();

Is that what you meant? The outcome is the same - the outer patch runs, the inner patch does not.

But if I swap the calls, both run:

patcher.PatchAll(typeof(WearNTear_OnPlaced));  // inner 
patcher.PatchAll(typeof(Player_PlacePiece));  // outer
ManlyMarco commented 1 year ago

Thanks, @ghorsington will have to take a look at it.

Windows10CE commented 1 year ago

This sounds like it's just an inlining issue, if you patch the outer first, the method gets compiled before the inner patch can be applied to disable inlining for the inner method. Inlined methods effectively aren't patched, because the JIT uses the original IL to inline the method. If you patch the inner first, MonoMod disables inlining for the method permanently, meaning the inner method will no longer be inlined, and both patches will run.

This issue will not be visible in IL, as the JIT is what does inlining.

This is a bit of a wontfix on HarmonyX's part, it can't globally disable inlining even if it wanted to (that would kill performance anyway), and there's no way to know all methods that will need to be patched in advance.

SpikeHimself commented 1 year ago

Thanks for the insight @Windows10CE

So is there something I can do to apply the inner patch before another mod applies the outer patch? That would solve the issue for me.

Windows10CE commented 1 year ago

Easiest way? Get your mod loaded before the other mod, either by getting the other mod to add a SoftDependency on your mod, or having your mod get sorted higher alphabetically.

Alternatively, you could write a preloader that adds the NoInlining flag to the inner method before the assembly even loads.

SpikeHimself commented 1 year ago

The more I think about this, the less sense it makes in my head. Why does the inner method get inlined when a harmony patch exists on the outer method, but not if it doesn't?

Getting other mods to add a soft dependency on my mod would be an impossible task. There are a plethora of mods that patch the outer method. It seems that in a game where you can build stuff, lots of mods are interested in the build event for some reason! :P Renaming my mod isn't great either as I would have to abandon the current mod and make a new one, risking my sanity as well as a loss in endorsements and users. Writing a preloader just because I want to know when a specific item is being built in a game seems like insane overkill, I'd like to avoid that approach at all costs.

I did write a silly work-around which checks the result of the inner method after a small delay, and in that way I am able to "emulate" the circumstances under which the inner patch "would have" run. It's very ugly but I guess it'll have to do.

Is there a way for me to detect that the outer method was patched? Because if it wasn't, I can patch the inner method and everything will be fine, and I won't need my work-around. I can then opt to only run my work-around if strictly necessary. (edit: I found Harmony.GetAllPatchedMethods(), yay!)

The only other thing for it, I suppose, would be to write "known issue" in the comment above my patch and move on with life.


Edit: Working around this issue like so:

var playerPlacePiecePatched =
    Harmony.GetAllPatchedMethods().Where(m => m.DeclaringType.Name.Equals("Player") && m.Name.Equals("PlacePiece")).Any();

if (!playerPlacePiecePatched)
{
    Log.Debug("Patching WearNTear.OnPlaced, yay!");
    patcher.PatchAll(typeof(WearNTear_OnPlaced));
}
else
{
    Log.Debug("Patching Piece.SetCreator, boo!");
    patcher.PatchAll(typeof(Piece_SetCreator));
}

Seems to get the job done when I patch the outer method myself.

(Piece.SetCreator is another inner method that is being called by the same outer method - it's not the event I'm looking for, but in its postfix I can, after a small delay, check the results of the inlined method)

Now it doesn't matter if my mod was loaded first or not - so long as the outer method was not patched yet, patching the inner one will work. And once the inner method is patched, patching the outer method does not cause problems.

ZenDragonX commented 2 months ago

I am also experiencing this problem. A year later. Same area of the code too. Player.PlacePiece and WearNTear.OnPlaced.

SpikeHimself commented 2 months ago

I am also experiencing this problem. A year later. Same area of the code too. Player.PlacePiece and WearNTear.OnPlaced.

I don't think this is going to be fixable by Harmony, at least not in the short term. The bug is created by the compiler's inlining, and so the function being patched no longer exists when the time comes for Harmony to apply its patches.

Working around it seems the only way to cope with it for now.