search

discord / discord-rpc

https://discordapp.com/developers
MIT License
1.06k stars 331 forks source link

[Suggestion] Patch for MinGW and possibly WinXP support #102

Closed k1-801 closed 6 years ago

k1-801 commented 6 years ago

There were several questions about MinGW and WinXP support in discord-rpc (#88, #83, #34), all being closed as "not supported".

Inserting this piece of code into discord_register_win.cpp (instead of old preprocessor blocks) makes it compilable with MinGW as well.

UPD: Wrong solution, causes segfaults on MinGW builds, see comments below and PR #105

// Patch for discord_register_win.cpp
#include "discord-rpc.h"

#define WIN32_LEAN_AND_MEAN
#define NOMCX
#define NOSERVICE
#define NOIME
#include <windows.h>
#include <psapi.h>
#include <cwchar>
#include <stdio.h>

/**
 * Updated fixes for MinGW and WinXP
 * This block is written the way it does not involve changing the rest of the code
 * Checked to be compiling
 * 1) strsafe.h belongs to Windows SDK and cannot be added to MinGW
 * #include guarded, functions redirected to <string.h> substitutes
 * 2) RegSetKeyValueW and LSTATUS are not declared in <winreg.h>
 * The entire function is rewritten
 */
#ifdef __MINGW32__
/// strsafe.h fixes
#define StringCbPrintfW snwprintf
LPWSTR StringCbCopyW(LPWSTR a, size_t l, LPCWSTR b)
{
    a[l-1] = 0;
    return wcsncpy(a, b, l - 1); // does not set the last byte to 0 on overflow, so it's set to 0 above
}
#else
#include <strsafe.h>
#endif // __MINGW32__

/// winreg.h fixes
#ifndef LSTATUS
#define LSTATUS LONG
#endif
#ifdef RegSetKeyValueW
#undefine RegSetKeyValueW
#endif
#define RegSetKeyValueW regset
LSTATUS regset(HKEY hkey, LPCWSTR subkey, LPCWSTR name, DWORD type, const void *data, DWORD len)
{
    HKEY hsubkey = NULL;
    LSTATUS ret;
    if (subkey && subkey[0])  /* need to create the subkey */
    {
        if ((ret = RegCreateKeyW( hkey, subkey, &hsubkey )) != ERROR_SUCCESS) return ret;
        hkey = hsubkey;
    }
    ret = RegSetValueExW( hkey, name, 0, type, (const BYTE*)data, len );
    if (hsubkey) RegCloseKey( hsubkey );
    return ret;
}

// Discord_RegisterW and the rest of the file

It may be not the best solution, but it definetely makes the code compilable with MinGW (not tested to be working though, just finished rewriting it - will test as soon as I write some code to test it with).

The idea is:

Also, serialization.h contains #pragma declarations meant to supress warnings in MSVS, but they cause warnings in MinGW, so I wrapped them in #ifndefs, like this:

// Patch for serialization.h
#ifndef __MINGW32__
#pragma warning(push)
#pragma warning(disable : 4061) // enum is not explicitly handled by a case label
#pragma warning(disable : 4365) // signed/unsigned mismatch
#pragma warning(disable : 4464) // relative include path contains
#pragma warning(disable : 4668) // is not defined as a preprocessor macro
#pragma warning(disable : 6313) // Incorrect operator
#endif // __MINGW32__

#include "rapidjson/document.h"
#include "rapidjson/stringbuffer.h"
#include "rapidjson/writer.h"

#ifndef __MINGW32__
#pragma warning(pop)
#endif // __MINGW32__

What should I do next with these patches?

k1-801 commented 6 years ago

Made a pull request with these patches in hope it helps.

oldmud0 commented 6 years ago

What compile options make this work? Here is my attempt:

MINGW32 /r/discord-rpc/build
$ cmake ..
-- The C compiler identification is GNU 7.2.0
-- The CXX compiler identification is GNU 7.2.0
-- Check for working C compiler: /mingw32/bin/cc.exe
-- Check for working C compiler: /mingw32/bin/cc.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /mingw32/bin/c++.exe
-- Check for working CXX compiler: /mingw32/bin/c++.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
no rapidjson, download
-- Configuring done
-- Generating done
-- Build files have been written to: /r/discord-rpc/build

MINGW32 /r/discord-rpc/build
$ cmake --build . --target install
Scanning dependencies of target discord-rpc
[ 12%] Building CXX object src/CMakeFiles/discord-rpc.dir/discord-rpc.cpp.o
[ 25%] Building CXX object src/CMakeFiles/discord-rpc.dir/rpc_connection.cpp.o
[ 37%] Building CXX object src/CMakeFiles/discord-rpc.dir/serialization.cpp.o
[ 50%] Building CXX object src/CMakeFiles/discord-rpc.dir/connection_unix.cpp.o
R:/discord-rpc/src/connection_unix.cpp:7:10: fatal error: sys/socket.h: No such file or directory
 #include <sys/socket.h>
          ^~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [src/CMakeFiles/discord-rpc.dir/build.make:135: src/CMakeFiles/discord-rpc.dir/connection_unix.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:86: src/CMakeFiles/discord-rpc.dir/all] Error 2
make: *** [Makefile:130: all] Error 2

It seems it thinks my target is Unix and not Windows. Looks like I will have to hack up the CMakeLists to give me GCC compile flags for a Windows target.

k1-801 commented 6 years ago

I used codeblocks to compile this, so I'm not sure about cmake. When this problem appeared, I simply removed unix and linux files from compilation lists and it worked. Also, mingw segfaults on empty {} in initialization list sometimes, probably a weird mingw/gcc bug.

oldmud0 commented 6 years ago

I made this quick change:

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index c3dab48..9fa12f6 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -59,15 +59,18 @@ if(WIN32)
 endif(WIN32)

 if(UNIX)
-    set(BASE_RPC_SRC ${BASE_RPC_SRC} connection_unix.cpp)
-
-    if (APPLE)
-        add_definitions(-DDISCORD_OSX)
-        set(BASE_RPC_SRC ${BASE_RPC_SRC} discord_register_osx.m)
-    else (APPLE)
-        add_definitions(-DDISCORD_LINUX)
-        set(BASE_RPC_SRC ${BASE_RPC_SRC} discord_register_linux.cpp)
-    endif(APPLE)
+   if(MINGW)
+       set(BASE_RPC_SRC ${BASE_RPC_SRC} connection_win.cpp discord_register_win.cpp)
+   else(MINGW)
+       set(BASE_RPC_SRC ${BASE_RPC_SRC} connection_unix.cpp)
+       if (APPLE)
+           add_definitions(-DDISCORD_OSX)
+           set(BASE_RPC_SRC ${BASE_RPC_SRC} discord_register_osx.m)
+       else (APPLE)
+           add_definitions(-DDISCORD_LINUX)
+           set(BASE_RPC_SRC ${BASE_RPC_SRC} discord_register_linux.cpp)
+       endif(APPLE)
+   endif(MINGW)

     add_library(discord-rpc ${BASE_RPC_SRC})
     target_link_libraries(discord-rpc PUBLIC pthread)

Now it seems to compile successfully.

k1-801 commented 6 years ago

It may be a good idea to make a pull request for this if it works.

oldmud0 commented 6 years ago

I will consider it. Thanks.

oldmud0 commented 6 years ago

Well, turns out it didn't work. On Discord_Initialize the stack frame seems to jump to 0x00000000 and a segfault occurs. Tried it on someone else's project and the same thing happens.

$ gcc -v
Using built-in specs.
COLLECT_GCC=D:\Programs\msys64\mingw32\bin\gcc.exe
COLLECT_LTO_WRAPPER=D:/Programs/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.2.0/lto-wrapper.exe
Target: i686-w64-mingw32
Configured with: ../gcc-7.2.0/configure --prefix=/mingw32 --with-local-prefix=/mingw32/local --build=i686-w64-mingw32 --host=i686-w64-mingw32 --target=i686-w64-mingw32 --with-native-system-header-dir=/mingw32/i686-w64-mingw32/include --libexecdir=/mingw32/lib --enable-bootstrap --with-arch=i686 --with-tune=generic --enable-languages=c,lto,c++,objc,obj-c++,fortran,ada --enable-shared --enable-static --enable-libatomic --enable-threads=posix --enable-graphite --enable-fully-dynamic-string --enable-libstdcxx-time=yes --disable-libstdcxx-pch --disable-libstdcxx-debug --disable-isl-version-check --enable-lto --enable-libgomp --disable-multilib --enable-checking=release --disable-rpath --disable-win32-registry --disable-nls --disable-werror --disable-symvers --with-libiconv --with-system-zlib --with-gmp=/mingw32 --with-mpfr=/mingw32 --with-mpc=/mingw32 --with-isl=/mingw32 --with-pkgversion='Rev1, Built by MSYS2 project' --with-bugurl=https://sourceforge.net/projects/msys2 --with-gnu-as --with-gnu-ld --disable-sjlj-exceptions --with-dwarf2
Thread model: posix
gcc version 7.2.0 (Rev1, Built by MSYS2 project)

Disassembly right at the call:

        26 [1]    Discord_Initialize(APPLICATION_ID, &handlers, 1, nullptr);
0x429125  <+0x00af>        8b 45 b4                 mov    -0x4c(%ebp),%eax
0x429128  <+0x00b2>        8b 00                    mov    (%eax),%eax
0x42912a  <+0x00b4>        c7 44 24 0c 00 00 00 00  movl   $0x0,0xc(%esp)
0x429132  <+0x00bc>        c7 44 24 08 01 00 00 00  movl   $0x1,0x8(%esp)
0x42913a  <+0x00c4>        8d 55 c4                 lea    -0x3c(%ebp),%edx
0x42913d  <+0x00c7>        89 54 24 04              mov    %edx,0x4(%esp)
0x429141  <+0x00cb>        89 04 24                 mov    %eax,(%esp)
0x429144  <+0x00ce>        e8 17 3a 00 00           call   0x42cb60 <Discord_Initialize(char const*, DiscordEventHandlers*, int, char const*)>

Here is the body of that function:

Discord::Discord()
{
  DiscordEventHandlers handlers;
  std::memset(&handlers, 0, sizeof(handlers));
  handlers = {};
  handlers.ready = [] {
    qInfo() << "Discord RPC ready";
  };
  /*
  handlers.disconnected = [](int errorCode, const char* message) {
    qInfo() << "Discord RPC disconnected! " << message;
  };
  handlers.errored = [](int errorCode, const char* message) {
    qWarning() << "Discord RPC errored out! " << message;
  };
  */
  qInfo() << "Are things working out all right?";
  Discord_Initialize(APPLICATION_ID, &handlers, 1, nullptr);
}

The string Are things working out all right? gets printed and the breakpoint at that point gets hit, so the problem obviously lies in Discord_Initialize. I was able to step through discord_register_win.cpp and found that the segfault was reached right at RegCloseKey(key) in Discord_RegisterW. So the problem is either in closing the registry key (unlikely) (I modified it to step over a few lines after the closing the key) or a compiler problem in returning from the function (somewhat likely but still bizarre) (confirmed; segfault occurs at leave instruction before ret in Discord_RegisterW, so the stack was corrupted somehow).

I'm completely dumbfounded at what the issue could be.

For now, I'm going to stick to the pre-built DLLs.

k1-801 commented 6 years ago

I still have nothing to test the library with. But, checking some documentation for WinAPI shows that RegCreateKey[W] function is obsolete and may even be unsupported in newer versions in favor of RegCreateKeyEx[W]. However, this is almost the same code as used in ReactOS, so it should have worked. If this is the reason for stack corruption, replacing it as manual suggests should work. Does changing the line that uses it to

if ((ret = RegCreateKeyExW(hkey, subkey, 0, 0, 0, KEY_ALL_ACCESS, 0, &hsubkey, 0)) != ERROR_SUCCESS) return ret;

fix the issue? Also, what is the Windows version you are using for tests?

oldmud0 commented 6 years ago

Hi, thanks for getting back. I am using Windows 7 64-bit with the latest updates.

The code you provided unfortunately seems to have no effect. The stack corruption still occurs.

k1-801 commented 6 years ago

Found another detail about RegCreateKey[Ex][W]:

If hKey is one of the predefined keys, lpSubKey may be NULL. In that case, phkResult receives the same hKey handle passed in to the function.

That being said, if the second argument is null, hsubkey in regset may be equal to hkey, so it gets closed when regset returns, but hkey may be still in use. I'm surprised I didn't see it or even think about it when I was readin it first time, although this behaviour seems logical. The first thing that comes to my mind is to check them to be equal before closing, so regset would look like that:

LSTATUS regset(HKEY hkey, LPCWSTR subkey, LPCWSTR name, DWORD type, const void *data, DWORD len)
{
    HKEY htkey = hkey, hsubkey = nullptr;
    LSTATUS ret;
    if (subkey && subkey[0])
    {
        if((ret = RegCreateKeyExW(hkey, subkey, 0, 0, 0, KEY_ALL_ACCESS, 0, &hsubkey, 0)) != ERROR_SUCCESS) return ret;
        htkey = hsubkey;
    }
    ret = RegSetValueExW(htkey, name, 0, type, (const BYTE*)data, len);
    if (hsubkey && hsubkey != hkey) RegCloseKey(hsubkey);
    return ret;
}

Which is more complicated than the original one, but this is the most obvious solution that comes to my mind. Although, I don't know why this code works in ReactOS without consequences, unless it doesn't really close key handlers, which should be fixed as well. I'm pretty sure this one should fix the issue. Also, I'm not sure about KEY_ALL_ACCESS. It is said to be the default one, but I have no idea how this thing works, so a more specific constant may be needed. P.S. This is the reference function used: https://doxygen.reactos.org/d2/da2/RegSetKeyValue_8c_source.html

oldmud0 commented 6 years ago

Tried it again, yet the stack corruption persists. What's going on?

k1-801 commented 6 years ago

It took a while to do some debugging. Turns out the problem wasn't even in the registry interactions - the stack gets destroyed after StringCbCopyW call, which in my case was replaced with my own function, and by all means it is my fault. In this case, MSVS builds were not spoiled by my changes. The reason for this code behavior is simple: strsafe.h functions take the size in bytes (and it is retrieved by sizeof()), while string.h ones take it in wide characters, and some conversion must be done first. I've already finished adapting my replacement code to the standards, currently testing it. So, things that had to be done: 1) This dumb substitute function

HRESULT StringCbPrintfW(LPWSTR pszDest, size_t cbDest, LPCWSTR pszFormat, ...)
{
    HRESULT ret;
    va_list va;
    va_start(va, pszFormat);
    cbDest /= 2; // Size is divided by 2 to convert from bytes to wide characters - causes segfault othervise
    ret = vsnwprintf(pszDest, cbDest, pszFormat, va);
    pszDest[cbDest - 1] = 0; // Terminate the string in case a buffer overflow; -1 will be returned
    va_end(va);
    return ret;
}

instead of a simple #define. This allows size conversion and a guaranteed 0-terminator. 2) The only call of StringCbCopyW was replaced with StringCbPrintfW with a L"%s" format string. It was a commonly used style in the rest of this file anyway.

        //StringCbCopyW(openCommand, oc_len, exeFilePath);
        StringCbPrintfW(openCommand, sizeof(openCommand), L"%s", exeFilePath);

The function itself was removed.

UPD: Tested 4 times, seems to be working by now. Please confirm this solution to be working or inform me if it's not. Made a PR #105 with fixes.

msciotti commented 6 years ago

@k1-801 can we close this with the merge of #105 ?