-
-
Notifications
You must be signed in to change notification settings - Fork 296
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
nathanvoglsam
wants to merge
10
commits into
bombomby:master
Choose a base branch
from
nathanvoglsam:mingw-compilation-support
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
MinGW Support #112
nathanvoglsam
wants to merge
10
commits into
bombomby:master
from
nathanvoglsam:mingw-compilation-support
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Solves issue #102 |
aminya
approved these changes
Jan 16, 2021
Any updates on this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 forx86_64-pc-windows-gnu
is immensely stronger than on thex86_64-pc-windows-msvc
target so it would be ideal for me to still be able to use thegnu
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 beOPTICK_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
#if
guards so the behavior of the MSVC code should be unchanged.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 raiseWINVER
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 defineWINVER
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
. TheSymLoadModule64
is defined to takechar*
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 aconst char*
. The value for that paramater is taken fromstd::string.c_str()
which always returns aconst 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