search

BepInEx / HarmonyX

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

Fix for unintentional removal of some `leave` instructions #96

Closed kohanis closed 5 months ago

kohanis commented 6 months ago

My vision of #65 fix. Yes, this will keep 2 leave instructions, but it's jump anyway, no harm done. Upstream harmony makes it this way. And, if someone wants to, they can find a way to remove these duplicates without offset shifting later on

ManlyMarco commented 6 months ago

One issue I can see is if a transpiler is looking for the leave it will most likely not check that it's the last leave of the two, which can lead to unexpected behaviour. I believe this was the intent for deduplicating the leaves in the first place.

kohanis commented 6 months ago

That's not going to happen. These instructions are added after all transpilers have already been applied, and repatching takes original instructions anyway

EDIT: I think this deduplication was intended to clean up potential errors on the part of the user, like moving labels or blocks unintentionally. But from my point of view it just ends up causing more problems for those who know what they are doing. Even though this is easily bypassed by adding a Nop after each Leave, I still don't understand the need for it

ManlyMarco commented 6 months ago

In that case this should work, yeah. @mgreter can you confirm if this fixes the issue for you?

StarFluxGames commented 6 months ago

Can confirm, this also fixes the issue I was having as well.

mgreter commented 6 months ago

Seems OK to me, as it is basically the same fix I had initially proposed, just without the proposed fix to cleanup the added leave statements in https://github.com/BepInEx/HarmonyX/pull/77/files. So for me, good to go! FWIW it may be helpful to have helpers to decide if you want to move the jump label or not. E.g. like a API insertCode(Ops[], offset x, bool MoveJumpLabel).

Meivyn commented 6 months ago

Stumbled upon another case of an issue due to this deduplication while patching state machines, this seems to cause more issues than it was originally solving. Is there any ETA on merging/releasing this?

ManlyMarco commented 6 months ago

I'm basically only waiting for more people to test this.

Meivyn commented 6 months ago

The Nop workaround doesn't even seem to work in my new case. It seems that it only works when it's the next instruction that has an exception block, which makes sense considering the deduplication condition.

None of the patches we are using seem to break by this PR.

ManlyMarco commented 6 months ago

I just ran some tests and everything seems to be working with this PR and latest stable bep5. I'll merge this once the #79 situation is resolved.