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

[cryptopp]: Fix compilation with clang-cl #26151

Merged
merged 3 commits into from
Aug 6, 2022

Conversation

raphaelthegreat
Copy link
Contributor

@raphaelthegreat raphaelthegreat commented Aug 3, 2022

Describe the pull request

  • What does your PR fix?

    Fixes compilation on Windows using clang-cl, by performing the following:
    • Add clang-cl 12.0+ to supported configurations
    • Use MSVC path to detect CRYPTOPP_CXX17_UNCAUGHT_EXCEPTIONS

* Add clang-cl 12.0+ to supported configurations
* Use MSVC path to detect CRYPTOPP_CXX17_UNCAUGHT_EXCEPTIONS
@ghost
Copy link

ghost commented Aug 3, 2022

CLA assistant check
All CLA requirements met.

@Neumann-A
Copy link
Contributor

I get:

FAILED: CMakeFiles/cryptopp-object.dir/chacha_avx.cpp.obj 
E:\vcpkg_cache\downloads\tools\clang\clang-14.0.6\bin\clang-cl.exe  /nologo -TP -D_WIN32_WINNT=0x0A00  /nologo /EHsc /GR /DWIN32 /D_WIN64 /D_WIN32_WINNT=0x0A00 /DWINVER=0x0A00 /D_CRT_SECURE_NO_DEPRECATE /D_CRT_SECURE_NO_WARNINGS /D_CRT_NONSTDC_NO_DEPRECATE /D_ATL_SECURE_NO_DEPRECATE /D_SCL_SECURE_NO_WARNINGS /D_CRT_INTERNAL_NONSTDC_NAMES /D_CRT_DECLARE_NONSTDC_NAMES /D_FORCENAMELESSUNION -mcrc32 -msse4.2 /clang:-fasm -fmacro-backtrace-limit=0 /utf-8 /WX- /Od /Ob0 /GS /RTC1 /FC  /MDd /Z7 /Brepro /D_DEBUG /FC  /FIwinapifamily.h /showIncludes /FoCMakeFiles\cryptopp-object.dir\chacha_avx.cpp.obj /FdCMakeFiles\cryptopp-object.dir\ -c -- E:\vcpkg_folders\llvm_test\buildtrees\cryptopp\src\TOPP_8_6_0-ea3517667b\chacha_avx.cpp
E:\vcpkg_folders\llvm_test\buildtrees\cryptopp\src\TOPP_8_6_0-ea3517667b\chacha_avx.cpp(63,8): error: unknown type name '__m256i'
inline __m256i RotateLeft(const __m256i val)
       ^

because the build seems not to be adding the required -mavx2 flag

github-actions[bot]
github-actions bot previously approved these changes Aug 4, 2022
@FrankXie05 FrankXie05 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. labels Aug 4, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for cryptopp have changed but the version was not updated
version: 8.6.0#1
old SHA: 2a1d13e53bda3bc8638f43beb50fe9252e838260
new SHA: b0205883588a92221923d9d68d48fa27feb8b8ac
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/cryptopp/vcpkg.json

Valid values for the license field can be found in the documentation

@raphaelthegreat
Copy link
Contributor Author

raphaelthegreat commented Aug 4, 2022

I tested the build with clang-cl included in the VS 2022 installer and it compiled successfully. I'll try the standalone as well, it probably can't detect the platform correctly

github-actions[bot]
github-actions bot previously approved these changes Aug 4, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/cryptopp/vcpkg.json

Valid values for the license field can be found in the documentation

* Patch CheckCompileLinkOption to use try_compile on all platforms
* Use GNU path when detecing SSE options with clang-cl
@raphaelthegreat
Copy link
Contributor Author

So I performed some tests and the problem seems to be that while CMake enables the MSVC flag with clang-cl, CMAKE_CXX_COMPILER_ID remains equal to "Clang" which breaks the SIMD flag configuration.

@raphaelthegreat
Copy link
Contributor Author

raphaelthegreat commented Aug 4, 2022

Tested build using the latest clang-cl 14.0 with the x64-windows-llvm triplet from this repository.
The only change that was needed was to replace /clang:-fopenmp-simd with /openmp here

@Neumann-A
Copy link
Contributor

Merged this into #25897. Compiles fine now.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/cryptopp/vcpkg.json

Valid values for the license field can be found in the documentation

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Aug 5, 2022
@FrankXie05 FrankXie05 changed the title cryptopp: Fix compilation with clang-cl [cryptopp]: Fix compilation with clang-cl Aug 5, 2022
@BillyONeal
Copy link
Member

Please submit this patch upstream to https://github.com/weidai11/cryptopp

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants