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

LilyPad: Delay load dependent DLLs #3053

Open
wants to merge 1 commit into
base: master
from

Conversation

@CookiePLMonster
Copy link
Contributor

commented Aug 2, 2019

This PR "fixes" a possible crash when enumerating plugins. As LilyPad depends on dinput8.dll, it was loaded and unloaded together with LilyPad, however it seems that DInput8 on Windows 10 regressed and now such rapid load/unload can cause it to crash.

With this PR, dinput8.dll and other dependant DLLs are delay loaded, so they are loaded before they are actually needed. Only dinput8.dll required this to work around the crash, but I also included the others (hid.dll and setupapi.dll), since it looks like it's totally possible to go through an entire session without using their functions. This should also speed up enumeration and startup, since less DLLs load at these points now.

Overview of DInput8 bug which could have resulted in a crash when enumerating LilyPad:
https://cookieplmonster.github.io/2019/08/02/win10-dinput8-crash/

@willkuer

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

I just read a blog entry that such delayed loading can lead to unexpected behavior on resource release/dll unloading/app shutdown as the order of dll unloading is not known anymore to the app and hence all dll’s will be unloaded in arbitrary order and if one dll holds a reference to another dll’s content it will break.

Might occur when playing a game (hence loading all dlls) and then switching to another plugin dll or maybe even closing pcsx2.

I will check whether I can find the blog entry. Maybe it’s unrelated...

@willkuer

This comment has been minimized.

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

You're absolutely right, I forgot about the shutdown sequences - although the article you linked, and the one it links to provides answers for your concerns and also a proper way for me to ensure nothing bad happens:

Note that this problem occurs only at process shutdown. If CONTOSO.DLL unloads via a runtime call to Free­Library, it will still be able to call into WIDGET.DLL because it hasn’t yet called Free­Library on WIDGET.DLL. But during process shutdown, the module loader needs to free all the things, and the outstanding Load­Library won’t prevent that from happening.

So basically, delay loading will not cause problems when switching plugins (since LilyPad unloads via FreeLibrary) but it might cause problems on terminating PCSX2. But then:

The solution is to bypass widget cleanup if the DLL_PROCESS_DETACH handler realizes that the process is terminating. Just leak the widget. The building is being demolished. You don’t need to sweep the floors.

And that part is trivial - I'll edit the PR in a bit.

LilyPad: Delay load dependent DLLs to speed up enumeration and work a…
…round a Windows 10 DInput8 DLL bug

Overview of DInput8 bug which could have resulted in a crash when enumerating LilyPad:
https://cookieplmonster.github.io/2019/08/02/win10-dinput8-crash/

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:lilypad-dinput-crash-fix branch from 68a3d57 to 6917fc3 Aug 3, 2019

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

So I inspected the code again and even when terminating PCSX2, plugins are unloaded via FreeLibrary, so it's safe for delay loading. That said, I guarded process detach routines against attempting to free again if the process is terminating, as per MSFT guidelines. This should pre-emptively solve any issues with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.