Add support for building with clang-cl #4255

solemnwarning · 2024-07-24T21:06:18Z

Both x86_32 and x86_64 build using the appropriate toolchain from VS2022, all tests pass except for the following in both cases:

System Certificate Store - Find Certificate by UTF8 subject DN ran 1 tests in 1.04 msec 1 FAILED
Failure 1: found certificate was nullopt (at src/tests/test_certstor_system.cpp:306)

coveralls · 2024-07-24T21:34:37Z

coverage: 91.275% (+0.001%) from 91.274%
when pulling cce2b6d on solemnwarning:clang-cl
into 1499274 on randombit:master.

solemnwarning · 2024-07-27T09:35:17Z

It turns out the aforementioned test failure isn't specific to the clang-cl build - it happens with MSVC too, I think its failing because my Windows VM doesn't have the "D-TRUST Root Class 3 CA 2 EV 2009" certificate the tests are looking for.

reneme · 2024-07-27T16:14:11Z

That's certainly possible. If I recall correctly, the certstore tests are looking for more than one cert. so if it fails for just that one, you're probably right.

reneme

Regarding the failing test: Let's try to assume the existence of a different certificate. Apparently the D-Trust certificate isn't as wide-spread.

See: #4280

src/build-data/cc/clangcl.txt

reneme · 2024-07-31T16:56:33Z

@solemnwarning could you rebase to master and see if the test succeeds on your platform now?

solemnwarning · 2024-08-01T15:50:34Z

@reneme

I rebased onto master and the tests still failed... they do pass if I import the "D-TRUST Root Class 3 CA 2 EV 2009" certificate into the trust store though, so clearly my test VM is lacking certificates that everyone else apparently has.

Regarding /Zc:preprocessor, it still builds okay, so I assume its fine without?

reneme · 2024-08-01T15:53:52Z

Regarding /Zc:preprocessor, it still builds okay, so I assume its fine without?

I added that for a change in the tests. So if the unit tests (make tests) still build fine, I'd also conclude that it's not needed.

they do pass if I import the "D-TRUST Root Class 3 CA 2 EV 2009"

👍

reneme

I tested on my windows environment and unfortunately the linker workaround doesn't seem to work for me. Or am I doing something wrong?

Also: I added a few commits that, I think, are worthwhile to add here: https://github.com/reneme/botan/commits/clang-cl/

a CI job on Windows, and
dedicated detection of 'clang-cl' and its version.

src/lib/math/bigint/big_ops2.cpp

src/build-data/cc/clangcl.txt

src/lib/math/bigint/big_ops2.cpp

src/lib/tls/tls13_pqc/hybrid_public_key.h

src/lib/tls/tls13_pqc/hybrid_public_key.cpp

src/lib/prov/pkcs11/p11.h

src/lib/utils/socket/socket.cpp

This commit allows building with the Visual Studio LLVM toolchain by passing --cc=clangcl to the configure.py script.

reneme

Looks good to me, apart from the two open discussions in p11.h.

reneme · 2024-08-13T09:29:29Z

src/lib/prov/pkcs11/p11.h

@@ -40,7 +47,7 @@
   #pragma pack(push, cryptoki, 1)
 #endif

-#include "pkcs11.h"
+#include <pkcs11.h>


I can imagine the quote-based include might have been on purpose here. We do ship the upstream pkcs11 headers in the same include directory. There might be a case for preferring those over some system-provided ones. @randombit?

I do not recall the reasoning on this one. I guess if we end up picking up a system provided header that's ok as long as it's recent enough.

randombit · 2024-08-13T10:55:38Z

src/build-data/detect_version.cpp

@@ -3,7 +3,7 @@
 * configure.py to determine the compilers version number.
 */

-#if defined(_MSC_VER)
+#if defined(_MSC_VER) and !defined(__clang__)


Does this cause us to detect the compiler as clang (vs clangcl)? I would think what we'd want is an additional check

#if defined(_MSC_VER) && defined(__clang__) CLANGCL __clang_major__ __clang_minor__

This detects the version only. Guessing the compiler happens before even "running" the detect_version.cpp procedure.. That said:

it probably doesn't hurt to handle clang-cl explicitly here,

we should look into 'guessing' clang-cl correctly.

Right now, I wouldn't be surprised if the configure script would only pick up clang-cl when it is asked to do so explicitly.

randombit · 2024-08-13T11:01:15Z

src/lib/prov/pkcs11/p11.h

@@ -40,7 +47,7 @@
   #pragma pack(push, cryptoki, 1)
 #endif

-#include "pkcs11.h"
+#include <pkcs11.h>


I do not recall the reasoning on this one. I guess if we end up picking up a system provided header that's ok as long as it's recent enough.

src/lib/prov/pkcs11/p11.h

randombit · 2024-08-13T11:12:23Z

src/build-data/ninja.in

@@ -10,7 +10,7 @@ LANG_EXE_FLAGS = %{cc_lang_binary_linker_flags}
 CXXFLAGS       = %{cc_compile_flags}
 WARN_FLAGS     = %{cc_warning_flags}

-LDFLAGS        = %{ldflags} %{cc_compile_flags}
+LDFLAGS        = %{ldflags}


This behavior (passing the compiler flags also to the linker) was just added in #4206 which I guess is kind of inelegant but we'd need to figure out some alternate solution for #4196

randombit · 2024-08-13T11:15:05Z

configure.py

-    if cc.basename == 'msvc' and variables['cxx_abi_flags'] != '':
-        # MSVC linker doesn't support/need the ABI options,
-        # just transfer them over to just the compiler invocations
+    # On MSVC, the "ABI flags" should be passed to the compiler only, on other platforms, the


These shenanigans seem to suggest that we might end up with something that builds in CI but not on a developers machine, which isn't helpful. Is the issue here that Clang really dislikes /MD/etc during the link step?

If I'm recalling correctly, link.exe ignores (and warns on) those flags, while lld-link.exe errors on them.

This is relevant to #4424.

src/lib/math/bigint/big_ops2.cpp

solemnwarning · 2024-09-01T23:50:42Z

@randombit @reneme

Are either of you anticipating me making any changes on this PR? The remaining points seem to be a bit "maybe do something".

reneme · 2024-09-02T09:53:53Z

src/build-data/cc/clangcl.txt

+sse2   -> "-msse2"
+ssse3  -> "-mssse3"
+sse41  -> "-msse4.1"
+sse42  -> "-msse4.2"


Note that there's a pull request that removes explicit (but unused) support for SSE 4.2. This will likely cause issues on master. Please rebase your patch onto master ~~once the linked PR is merged~~ and simply remove this line.

reneme · 2024-09-02T09:55:50Z

Are either of you anticipating me making any changes on this PR?

This is awaiting approval by @randombit

solemnwarning force-pushed the clang-cl branch 2 times, most recently from 6b32b1e to 630d6f1 Compare July 25, 2024 22:34

reneme reviewed Jul 31, 2024

View reviewed changes

src/build-data/cc/clangcl.txt Show resolved Hide resolved

reneme reviewed Aug 2, 2024

View reviewed changes

src/lib/math/bigint/big_ops2.cpp Outdated Show resolved Hide resolved

src/build-data/cc/clangcl.txt Outdated Show resolved Hide resolved

solemnwarning force-pushed the clang-cl branch from 630d6f1 to dd022b3 Compare August 2, 2024 22:33

reneme reviewed Aug 5, 2024

View reviewed changes

solemnwarning force-pushed the clang-cl branch 2 times, most recently from f3f45bc to 6171bd3 Compare August 5, 2024 18:43

reneme reviewed Aug 12, 2024

View reviewed changes

src/lib/prov/pkcs11/p11.h Show resolved Hide resolved

src/lib/utils/socket/socket.cpp Outdated Show resolved Hide resolved

solemnwarning and others added 10 commits August 12, 2024 23:51

Add support for building with clang-cl

628f915

This commit allows building with the Visual Studio LLVM toolchain by passing --cc=clangcl to the configure.py script.

Add a CI job for clang-cl

6729c44

Add dedicated version detection for clang-cl

dbfd7f9

Provide a botan-specific preproc macro for clang-cl

472c6fd

Fix 32-bit clang-cl build

e594267

Fix clang-cl CI build(?)

48aaba9

Fix some warnings when building with clang-cl

ce51e6e

Fix CI

dbaf2a6

Add 32-bit clang-cl CI job

390a93c

FIX: multiple c'tor calls

cce2b6d

solemnwarning force-pushed the clang-cl branch from 6171bd3 to cce2b6d Compare August 12, 2024 22:51

reneme approved these changes Aug 13, 2024

View reviewed changes

randombit self-requested a review August 13, 2024 10:50

randombit reviewed Aug 13, 2024

View reviewed changes

reneme reviewed Sep 2, 2024

View reviewed changes

reneme mentioned this pull request Oct 7, 2024

Certstore system tests fail if certificates are not installed. #3984

Closed

reneme added this to the Botan 3.6.0 milestone Oct 17, 2024

randombit modified the milestones: Botan 3.6.0, Botan 3.7.0 Oct 21, 2024

randombit self-requested a review October 21, 2024 20:26

reneme mentioned this pull request Nov 5, 2024

MSVC Compiler Flags Incorrectly Passed to Linker (Regression) #4424

Open

WangWeiLin-MV mentioned this pull request Nov 13, 2024

[botan] Update to 3.6.1 microsoft/vcpkg#42007

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for building with clang-cl #4255

Add support for building with clang-cl #4255

solemnwarning commented Jul 24, 2024

coveralls commented Jul 24, 2024 •

edited

Loading

solemnwarning commented Jul 27, 2024

reneme commented Jul 27, 2024

reneme left a comment

reneme commented Jul 31, 2024

solemnwarning commented Aug 1, 2024

reneme commented Aug 1, 2024

reneme left a comment

reneme left a comment

reneme Aug 13, 2024

randombit Aug 13, 2024

randombit Aug 13, 2024

reneme Aug 13, 2024

randombit Aug 13, 2024

randombit Aug 13, 2024

randombit Aug 13, 2024

solemnwarning Aug 13, 2024

TriceHelix Nov 5, 2024

solemnwarning commented Sep 1, 2024

reneme Sep 2, 2024 •

edited

Loading

reneme commented Sep 2, 2024

Add support for building with clang-cl #4255

Are you sure you want to change the base?

Add support for building with clang-cl #4255

Conversation

solemnwarning commented Jul 24, 2024

coveralls commented Jul 24, 2024 • edited Loading

solemnwarning commented Jul 27, 2024

reneme commented Jul 27, 2024

reneme left a comment

Choose a reason for hiding this comment

reneme commented Jul 31, 2024

solemnwarning commented Aug 1, 2024

reneme commented Aug 1, 2024

reneme left a comment

Choose a reason for hiding this comment

reneme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solemnwarning commented Sep 1, 2024

reneme Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

reneme commented Sep 2, 2024

coveralls commented Jul 24, 2024 •

edited

Loading

reneme Sep 2, 2024 •

edited

Loading