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

fix MinGW #137

Merged
merged 2 commits into from
Mar 9, 2020
Merged

fix MinGW #137

merged 2 commits into from
Mar 9, 2020

Conversation

SchrodingerZhu
Copy link
Collaborator

  • update CMakeLists.txt to detect "AMD64"
  • update macros
    • define NOMINMAX only when not defined
    • define USE_CLZLL for mingw
    • include <cstdio> for mingw
  • update clz(size_t)
    • __builtin_clzll for mingw

@SchrodingerZhu SchrodingerZhu mentioned this pull request Mar 8, 2020
@SchrodingerZhu
Copy link
Collaborator Author

I checked that with mingw-w64-i686-toolchain, the project cannot be compiled. The -mcx16 cannot work somehow. Maybe we can just drop support for MinGW with i686 toolchains.

@mjp41
Copy link
Member

mjp41 commented Mar 9, 2020

I wonder if there are compatibility issues with linking in the mincore.lib. If you configure cmake with

-DWIN8COMPAT=TRUE

It won't try to use VirtualAlloc2, or pull in mincore.lib.

Also, I am a little concerned the changes you are making will affect the Windows-clang target.

@mjp41
Copy link
Member

mjp41 commented Mar 9, 2020

Sorry, just reread your mcx16 comment. That is pretty crucial to one of the datastructures, so I don't think we can support this.

@SchrodingerZhu
Copy link
Collaborator Author

Sorry, just reread your mcx16 comment. That is pretty crucial to one of the datastructures, so I don't think we can support this.

For MinGW x86_64, mcx16 works fine; but for MinGW i686, the mcx16 feature cannot be enabled even with -mcx16. Therefore, I just suggest not supporting MinGW i686

@SchrodingerZhu
Copy link
Collaborator Author

SchrodingerZhu commented Mar 9, 2020

I wonder if there are compatibility issues with linking in the mincore.lib. If you configure cmake with

-DWIN8COMPAT=TRUE

It won't try to use VirtualAlloc2, or pull in mincore.lib.

Also, I am a little concerned the changes you are making will affect the Windows-clang target.

thanks for reminding me of clang. I am not quite sure about it either; I remember on windows, clang will provide compatibility with MSVC rather than GNU, but it may still define __GNUC__? MSYS2 environment also provides a clang toolchain, I have not checked the library with that.

I do not quite understand the issue of mincore.lib, can you provide more details?

@SchrodingerZhu
Copy link
Collaborator Author

tried to compile with -DWIN8COMPAT=TRUE and MinGW x86-64 on Windows 10 Build 19577.rs_prerelease.200228-1439, everything works fine. Unfortunately, I do not have a Win 8 machine for further tests.

@mjp41
Copy link
Member

mjp41 commented Mar 9, 2020

For MinGW x86_64, mcx16 works fine; but for MinGW i686, the mcx16 feature cannot be enabled even with -mcx16. Therefore, I just suggest not supporting MinGW i686

This makes complete sense. We are only supporting 32bit on Windows with MSVC at the moment. We don't have any 32bit Linux support.

I think it it works without WIN8COMPAT that is fine. I had thought it was failing at runtime, and this might fix it. But if it is running, then it is not needed.

I'll try to find some time to see if Windows Clang is setting GNUC.

Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I confirmed Windows' Clang unaffected.

@mjp41 mjp41 merged commit 31da639 into microsoft:master Mar 9, 2020
Copy link
Collaborator

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be missing a CI job, so we have no way of knowing if this will break in the future.

@@ -72,6 +72,8 @@ if(NOT MSVC)
target_compile_options(snmalloc_lib INTERFACE -mcx16)
elseif(${CMAKE_SYSTEM_PROCESSOR} STREQUAL "amd64")
target_compile_options(snmalloc_lib INTERFACE -mcx16)
elseif(${CMAKE_SYSTEM_PROCESSOR} STREQUAL "AMD64")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this not be a case-insensitive string compare (e.g. use TOLOWER on the processor and then compare against amd64)?

@SchrodingerZhu
Copy link
Collaborator Author

@davidchisnall

This rust crate can be referred as a test, it also provides freebsd and mingw environment.

MSVC/MinGW/Linux/MacOS: travis ci

FreeBSD: Build Status

However, this is not a good solution. It would be better to have an extra pipeline here.

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.

3 participants