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

Wine stability improvements #1769

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marzent
Copy link
Contributor

@marzent marzent commented Apr 13, 2024

This uses MinHook for the Addon Hooks, as there seems to be a race with Reloaded somewhere that leads to execution of random code after a while.

Also makes DALAMUD_LEGACY_CORRUPTED_STATE_EXCEPTION configurable, which is highly undesirable to have enabled with Wine, as this can mask underlying issues and lead to random crashes as well.

@marzent marzent requested a review from a team as a code owner April 13, 2024 21:10
@KazWolfe
Copy link
Member

n.b. holding this PR as we're allegedly seeing some issues with MinHook with certain plugins (including Simple Tweaks). See #linux-and-dev channel in the Dalamud Discord.

We're also looking at how best to handle DALAMUD_LEGACY_CORRUPTED_STATE_EXCEPTION since ideally we'd disable this on Windows as well, but this may break certain plugin behaviors.

@marzent
Copy link
Contributor Author

marzent commented Apr 20, 2024

Indeed DALAMUD_LEGACY_CORRUPTED_STATE_EXCEPTION should ideally not exist and plugins that rely on that, are basically doing a CrashRandomyl(), especially with memory writes (well also with reads if it hits a guard page); can remove it and just disable corrupted state exceptions if that is better...

The Reloaded issue does indeed need some more looking into, in any case something is not behaving as it should. If this change does also create issues on Windows that might also be worth investigating, since that is a bit easier to debug.

@goaaats
Copy link
Member

goaaats commented Apr 20, 2024

We definitely can't "just" turn it off, we'll have to have some kind of migration plan. Either way it's a hard sell to devs, we'll talk about it internally, it's good to have the option for now though.

@KazWolfe KazWolfe self-assigned this Jun 3, 2024
@KazWolfe KazWolfe added core Dalamud core linux This Issue applies only to Linux environments. labels Jun 3, 2024
@marzent
Copy link
Contributor Author

marzent commented Jun 10, 2024

I dropped the hook changes part for now...
If legacy state exceptions are supposed to just be unconditionally gone without fallback for API X, this can also be done.

@sersorrel
Copy link

I dropped the hook changes part for now...

is the thinking that the problems those changes addressed are now fixed in a different way, or will be fixed with Dawntrail, or something else?

@marzent
Copy link
Contributor Author

marzent commented Jun 10, 2024

I believe it to be an issue somewhere inside reloaded for stacked hooks; am currently trying to come up with a better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Dalamud core linux This Issue applies only to Linux environments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants