Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for unintentional removal of some leave instructions #96

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Fix for unintentional removal of some leave instructions #96

merged 1 commit into from
Jan 19, 2024

Conversation

kohanis
Copy link
Contributor

@kohanis kohanis commented Dec 17, 2023

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
Copy link
Member

ManlyMarco commented Dec 20, 2023

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
Copy link
Contributor Author

kohanis commented Dec 20, 2023

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
Copy link
Member

ManlyMarco commented Dec 20, 2023

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

@StarFluxGames
Copy link

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

@mgreter
Copy link

mgreter commented Dec 22, 2023

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
Copy link
Contributor

Meivyn commented Dec 29, 2023

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
Copy link
Member

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

@Meivyn
Copy link
Contributor

Meivyn commented Dec 30, 2023

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
Copy link
Member

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.

@ManlyMarco ManlyMarco merged commit 2ea021a into BepInEx:master Jan 19, 2024
ManlyMarco added a commit that referenced this pull request Jan 19, 2024
Changes are exactly as expected, IL is functionally the same just with more jumps
@kohanis kohanis deleted the fix/issue-65 branch February 14, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants