-
Notifications
You must be signed in to change notification settings - Fork 271
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
Add a new "Hybrid" Dalamud load method #1668
base: master
Are you sure you want to change the base?
Conversation
if (numberOfBytesWritten != 1) | ||
throw new Exception("Failed to restore entry point instruction"); | ||
VirtualProtectEx(p.Handle, entryPoint, 1, entryPointProtection, out _); | ||
FlushInstructionCache(p.Handle, entryPoint, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.codeproject.com/Articles/132742/Writing-Windows-Debugger-Part-2#RevertBP_at_SA
CONTEXT ctx;
GetThreadContext(mt, &ctx);
ctx.Rip--;
SetThreadContext(mt, &ctx);
is needed to decrement the instruction pointer back to where the INT3 was; otherwise, the instruction pointer will point to the second byte of the first instruction, resulting in sub esp, 0x28
instead of sub rsp, 0x28
, causing crash due to truncated stack pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now
I'm extremely suspicious of adding even more complexity/variables into something that already is very difficult to troubleshoot. Why is this necessary, why can we not just fix what we have? |
0b02740
to
a4ca85f
Compare
Yeah I have been thinking of only creating a PR with 6296dcf , but this felt "free" in a way since it builds on top of it and its codepaths aren't executed at all with what XL currently does. It worked well for me personally for my usecase and it is sufficiently different from entrypoint in that it might be at least an alternative if there are issues with the other modes... The advantage here is that the main thread is virtually untouched (apart from being debugged until the ntdll loader gives off control to the game). These two could probably be mashed together somehow with a spinlock at the entrypoint plus remote thread, but I wanted to make minimal changes to existing entrypoint to not cause any regressions I didn't notice while testing. I feel like this is also conceptually easier to understand than rewriting the entrypoint (and personally I still don't understand how/why the stack pointer moves there). |
I think this can be simplified a bit more though |
This doesn't have any functional impact on entrypoint.
For the legacy dll injection method, this suspends the main thread for a tiny while after the game window is created, but is still showing only a black screen.
With "wait for plugins" enabled, it will block there similarly to how entrypoint blocks at the games entrypoint now, allowing TitleEdit for example to work reliably there now.
The new hybrid option is similar to the dll injection method, however it sets a breakpoint at the games entrypoint, suspends the games main thread and then detaches itself, resulting in behaviour similar to entrypoint without using assembly or touching the stack of the original entrypoint and rewriting large parts of it, before it continues execution.
Also it moves all of the heavy lifting for this new mode back into managed Dalamud.Injector (theoretically everything, including the entry point calculation can be moved back into Injector, but it would be two functions doing the same thing being implemented in different languages, and calling from c++ into c# is much more problematic there)
This makes, at least on my system, "wait for plugins" work reliably with wine now, with both DllInject and Hybrid.