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

MinGW Support #112

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

nathanvoglsam
Copy link

Currently OptickCore only supports compiling for MSVC for Windows. This pull request extends support of the current code and CMake build system to allow compiling OptickCore with the MinGW toolchain as well.

My motivation for this pull request is to enable using the x86_64-pc-windows-gnu target with rustc and the rust bindings. Debugger support for x86_64-pc-windows-gnu is immensely stronger than on the x86_64-pc-windows-msvc target so it would be ideal for me to still be able to use the gnu target.

Very little changes were needed. Almost every change was related to correctly handling conditional compilation as OPTICK_MSVC as used in many places where a more appropriate name would be OPTICK_WINDOWS as it wasn't guarding MSVC features but windows features.

I have tested the code using the CMake build system with MSVC, MinGW 6.0, MinGW 8.0 (Trunk, from MSYS2) and Linux. To provide a breakdown

  • MSVC: Build unchanged, all changes are behind #if guards so the behavior of the MSVC code should be unchanged.
  • MinGW 8.0: Compiles seamlessly and tested as functional with ConsoleApp
  • Linux: Same as MSVC, all changes are guarded by platform checks
  • MinGW 6.0: Can work but will fail to compile unless WINVER and _WIN32_WINNT are defined globally to _WIN32_WINNT_WIN7 or higher. Some of the ETW functions are only supported on Windows 7 or newer, and older MinGW builds have their headers default to _WIN32_WINNT_WINXP which is for Windows XP. In this case the functions are left undefined and the build will fail. In my preprocessor check I explicitly raise WINVER being too low to be an error with an #error directive. The solution, depending on how old your MinGW is either update the MinGW version being used or define WINVER and _WIN32_WINNT to 0x0601 (the value for _WIN32_WINNT_WIN7) with a compiler flag.
    Manually defining WINVER and _WIN32_WINNT is the intended path for targeting a newer windows version than the default.

The only change which I could see as contentious is for commit 3777e24. The SymLoadModule64 is defined to take char* for the module path parameter on MinGW. I believe this to be an error in the MinGW headers as the official MSVC Windows SDK headers define the parameter to take a const char*. The value for that paramater is taken from std::string.c_str() which always returns a const char* so will cause a compilation error on MinGW.

Under the assumption the MinGW header is wrong and the underlying function will never write through the pointer I cast the const away, which is super unsafe but I believe my assumption to be correct. This could be marked as a workaround and investigated further in the future if needed.

I also fixed a missing include in TestEngine

We need to link these explicitly on MinGW as we'll be compiling with GCC or Clang where support for #pragam comment to link isn't reliable
Functions needed by the library are only supported on Windows 7 or greater, so rather than failing with an error message likely to be "function x undeclared/undefined" explicitly raise an error that offers some solutions. Only very recent versions of MinGW default to building for Windows 7 so this is something people will need to be aware of
Guard out the UE4 stuff on MinGW as UE4 doesn't support MinGW anyway
The module path parameter is marked as char* on MinGW. Considering it's a const char* on MSVC's windows SDK I would expect this to be an error in MinGW's headers and consider this cast safe.
fails to compile on newer versions of MinGW
@nathanvoglsam
Copy link
Author

Solves issue #102

@MattyGroGy
Copy link

Any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants