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

plugins: add arch-specific mumble_plugin_win32.h variants #2354

Merged
merged 3 commits into from Jun 23, 2016

Conversation

@mkrautz
Copy link
Member

commented Jun 19, 2016

No description provided.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2016

@mkrautz mkrautz force-pushed the mkrautz:procptr-plugins branch from f7ca5f0 to b7fa6b7 Jun 19, 2016

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jun 19, 2016

Isn't it better to use "mumble_plugin_win32.h" and "mumble_plugin_win64.h" as names?

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2016

Here, we're using "win32" as in the Win32 API. I think the names are OK.

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jun 19, 2016

Right, I agree. I thought that "win32" was referring to the architecture 😉

mkrautz added 3 commits Jun 20, 2016
plugins: add distinct header files for x86 and x64.
This is accomplished by making a 'generic' header,
mumble_plugin_win32_ptr_type.h

This header requires files that include it to
define PTR_TYPE and PTR_TYPE_CONCRETE.

In the old-style mumble_plugin_win32.h, PTR_TYPE
is set to 'void *', and PTR_TYPE_CONCRETE is
set to 'BYTE *'. The pModule varaible and the
getModuleAddr functions return PTR_TYPE_CONCRETE,
whereas the peekProc functions take PTR_TYPE.

The new-style arch-specific headers use the same
value for PTR_TYPE and PTR_TYPE_CONCRETE. The x86
variant uses procptr32_t. The x64 variant uses
procptr64_t.
plugins.pri, mumble_plugin_win32.h: add CONFIG(no-plugin-legacy-ptr) …
…to disable use of non-explicit mumble_plugin_win32.h header.

The idea is that we can use the CONFIG option until we've fixed the
remaining plugins.

Once all plugins have been transitioned over to the new explicit
pointer types, we can drop the mumble_plugin_win32.h header and
point people to mumble_plugin_win32_x86.h and mumble_plugin_win32_x64.h.

@mkrautz mkrautz force-pushed the mkrautz:procptr-plugins branch from b7fa6b7 to 6c2cf49 Jun 20, 2016

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2016

Fixed

Compile error for BF4, BF4_x86, Borderlands 2, GTAIV, GTAV, GW, L4D2, LOL, QL, STO and UT99.:

mumble_plugin_win32_ptr_type.h:83: error: C2664: 'BOOL ReadProcessMemory(HANDLE,LPCVOID,LPVOID,SIZE_T,SIZE_T *)': cannot convert argument 2 from >'procptr32_t' to 'LPCVOID'

from #2349 (comment).

PTAL.

@hacst

This comment has been minimized.

Copy link
Member

commented Jun 20, 2016

So we definitely don't want to handle 32 and 64 bit versions of a game in the same plugin?

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2016

It's a good question that I really don't know the answer to.

Currently, we already do multiple plugins for such games (bf4, bf4_x86).

Personally, I think the API will become confusing if we add getModuleAddr(pid, modname) and getModuleAddr(modname) variants that return procptr32_t and procptr64_t (probably called getModuleAddr32 and getModuleAddr64), while using overloaded peekProc's. If we do that, we should probably add peekProc32 and peekProc64 instead.

My current thinking is that requiring separate plugins is OK. I say this with the expectation that the vast majority of games do not ship multiple binaries anymore -- most are x64-only, some will be x86-only. A minority will ship both x86 and x64. But I don't know if that really holds.

@davidebeatrici thoughts?

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jun 20, 2016

I think that handling both versions of the game in a single plugin would be difficult (different executable and memory addresses), it's better to keep two separated plugins for now.
Also, as you said, majority of games do not ship multiple binaries anymore. GTA V, for example, is x64-only, while majority of games is x86-only.

@hacst

This comment has been minimized.

Copy link
Member

commented Jun 21, 2016

If it's actually rare (which it is bound to be going forward I guess) I'm fine with it. Otherwise I would've preferred only having one plugin. Sure, the plugin would have to keep separate addresses and maybe exe names depending on the game but it's not like that is a problem. They would likely share other logic and even if not presenting only one plugin to the user is cleaner.

Anyways. LGTM.

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jun 21, 2016

I agree, certainly it would be cleaner to have only one plugin for multiple game versions, but for now I encountered only Battlefield 4 that is both x86 and x64.
If I encounter another game that has multiple binaries, we will decide what to do 😉

@fwaggle

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

World of Warcraft supports both architectures, but I'm not presently a subscriber and crushed by other obligations so I think it's probably safe to say we don't support either at the moment. :/

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

Mumble supports the x86 one; has it got different executables?

@fwaggle

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

Mumble supports the x86 one

Does it still? We're nearly 1300 build numbers behind, I've been really slack. I've no idea whether it still works or not (it's possible for it to continue working when the build number changes, as it's not guaranteed the executable changes), just assumed it probably didn't.

has it got different executables

Yeah. wow.exe and wow-64.exe. There's also a Macintosh version, but I don't think we support positional audio on the Mac at all, or maybe just via the Link plugin? I'm completely ignorant of Mumble on the Mac. :(

I suspect World of Warcraft is probably the one game that will support dual architectures for quite some time, as they would be throwing away a substantial part of their audience by dropping support for 32-bit processors.

Edit: I think they can both be supported in the same plugin rather trivially - just need to diverge the codepath at the trylock() function and have alternates for the 64 bit version. I tried writing a 64-bit plugin as a separate file, but couldn't work out why it wouldn't work. Most of the threads on the cheater forums we get our offsets from have 64-bit variants as well, so it should in theory be no more difficult to support once it's working.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2016

Right now I'm more concerned about having all x86 plugins work on x64 as well -- which this new API more easily allows.

It's all internal anyway. And it doesn't touch the plugin ABI at all.

If we determine it's neat to be able to have both architectures in a single plugin DLL, we can add a new header file for use for those plugins, and perhaps deprecate these ones.

We don't have to worry about compatibility.

I'll land this as-is, then we can tackle the dual-arch problem if it becomes a problem in practice...

@mkrautz mkrautz merged commit 6c2cf49 into mumble-voip:master Jun 23, 2016

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