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

Add support for building with clang-cl #4255

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,27 @@ jobs:
- target: shared
arch: x86_64
host_os: windows-2022
compiler: msvc
- target: static
arch: x86_64
host_os: windows-2022
compiler: msvc
- target: amalgamation
arch: x86_64
host_os: windows-2022
compiler: msvc
- target: shared
arch: x86
host_os: windows-2022
compiler: msvc
- target: shared
arch: x86_64
host_os: windows-2022
compiler: clangcl
- target: static
arch: x86
host_os: windows-2022
compiler: clangcl

runs-on: ${{ matrix.host_os }}

Expand All @@ -52,11 +64,11 @@ jobs:
uses: ./.github/actions/setup-build-agent
with:
target: ${{ matrix.target }}
cache-key: ${{ matrix.host_os }}-msvc-${{ matrix.arch }}-${{ matrix.target }}
cache-key: ${{ matrix.host_os }}-${{ matrix.compiler }}-${{ matrix.arch }}-${{ matrix.target }}
arch: ${{ matrix.arch }}

- name: Build and Test Botan
run: python3 ./src/scripts/ci_build.py --cc='msvc' --make-tool='ninja' --cpu='${{ matrix.arch }}' --test-results-dir=junit_results ${{ matrix.target }}
run: python3 ./src/scripts/ci_build.py --cc='${{ matrix.compiler }}' --make-tool='ninja' --cpu='${{ matrix.arch }}' --test-results-dir=junit_results ${{ matrix.target }}

linux:
name: "Linux"
Expand Down
21 changes: 16 additions & 5 deletions configure.py
Original file line number Diff line number Diff line change
Expand Up @@ -2313,11 +2313,21 @@ def test_exe_extra_ldflags():
variables['includedir'],
'botan-%d' % (Version.major()), 'botan')

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
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

This is relevant to #4424.

# ABI flags are passed to both the compiler and the linker and the compiler flags are also
# passed to the linker(?)
#
# TODO: Extend the build-data/cc/xxx.txt format to allow specifying different CFLAGS for
# different configurations, then /MD etc could be specified there rather than hijacking the ABI
# flags for it and having to special-case their exclusion from the linker command line.

if cc.basename in ('msvc', 'clangcl'):
# Move the "ABI flags" (/MD etc) into the compiler flags to exclude it from linker invocations
variables['cc_compile_flags'] = '%s %s' % (variables['cxx_abi_flags'], variables['cc_compile_flags'])
variables['cxx_abi_flags'] = ''
else:
# Append the compiler flags to the linker flags
variables['ldflags'] = '%s %s' % (variables['ldflags'], variables['cc_compile_flags'])

variables['lib_flags'] = cc.gen_lib_flags(options, variables)

Expand Down Expand Up @@ -3277,11 +3287,11 @@ def validate_options(options, info_os, info_cc, available_module_policies):
raise UserError('Option --with-pdf requires --with-sphinx')

# Warnings
if options.os == 'windows' and options.compiler != 'msvc':
if options.os == 'windows' and options.compiler not in ('msvc', 'clangcl'):
logging.warning('The windows target is oriented towards MSVC; maybe you want --os=cygwin or --os=mingw')

if options.msvc_runtime:
if options.compiler != 'msvc':
if options.compiler not in ('msvc', 'clangcl'):
raise UserError("Makes no sense to specify MSVC runtime for %s" % (options.compiler))

if options.msvc_runtime not in ['MT', 'MD', 'MTd', 'MDd']:
Expand All @@ -3303,6 +3313,7 @@ def calculate_cc_min_version(options, ccinfo, source_paths):
'msvc': r'^ *MSVC ([0-9]{2})([0-9]{2})$',
'gcc': r'^ *GCC ([0-9]+) ([0-9]+)$',
'clang': r'^ *CLANG ([0-9]+) ([0-9]+)$',
'clangcl': r'^ *CLANG ([0-9]+) ([0-9]+)$',
'xcode': r'^ *XCODE ([0-9]+) ([0-9]+)$',
'xlc': r'^ *XLC ([0-9]+) ([0-9]+)$',
'emcc': r'^ *EMCC ([0-9]+) ([0-9]+)$',
Expand Down
102 changes: 102 additions & 0 deletions src/build-data/cc/clangcl.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
macro_name CLANGCL

minimum_supported_version 14.0

binary_name clang-cl
linker_name lld-link

output_to_object "/Fo"
output_to_exe "/OUT:"

add_include_dir_option "/I"
add_system_include_dir_option "/external:W0 /external:I"
add_lib_dir_option "/LIBPATH:"
add_compile_definition_option "/D"
add_lib_option "%s.lib"

compile_flags "/nologo /c"

supports_gcc_inline_asm yes

optimization_flags "/O2 /Oi"
size_optimization_flags "/O1 /Os"

# for debug info in the object file (required if using sccache):
#debug_info_flags "/Z7"

# for using a PDB file:
debug_info_flags "/Zi /FS"

preproc_flags "/nologo /EP"

lang_flags "/std:c++20 /EHs /GR"
solemnwarning marked this conversation as resolved.
Show resolved Hide resolved

# 4251: STL types used in DLL interface
# 4275: ???
# 5072: ASan without debug info
warning_flags "/W4 /wd4251 /wd4275 /wd5072"

werror_flags "/WX"

visibility_build_flags "/DBOTAN_DLL=__declspec(dllexport)"
visibility_attribute "__declspec(dllimport)"

# Include dependency tracking for Ninja
# See: https://ninja-build.org/manual.html#ref_headers
ninja_header_deps_style 'msvc'
header_deps_flag '/showIncludes'

ar_command lib
ar_options "/nologo"
ar_output_to "/OUT:"

<sanitizers>
default -> address

iterator -> "/D_ITERATOR_DEBUG_LEVEL=1"
address -> "/fsanitize=address"
</sanitizers>

<isa_flags>
sse2 -> "-msse2"
ssse3 -> "-mssse3"
sse41 -> "-msse4.1"
sse42 -> "-msse4.2"
Copy link
Collaborator

@reneme reneme Sep 2, 2024

Choose a reason for hiding this comment

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

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.

avx2 -> "-mavx2"
avx512 -> "-mavx512f -mavx512bw -mavx512dq -mavx512vbmi -mavx512vbmi2 -mavx512bitalg -mavx512vl -mavx512ifma"

bmi2 -> "-mbmi -mbmi2"
aesni -> "-maes -mpclmul"
rdrand -> "-mrdrnd"
rdseed -> "-mrdseed"
sha -> "-msha"
altivec -> "-maltivec"

arm64:armv8crypto -> "-march=armv8+crypto"
arm64:armv8sha512 -> "-march=armv8.2-a+sha3"

arm32:neon -> "-mfpu=neon"
arm64:neon -> ""
</isa_flags>

<lib_flags>
debug -> "/Fd%{build_dir}/%{libname}.pdb"
</lib_flags>

<so_link_commands>
default -> "{linker} /DLL"
default-debug -> "{linker} /DLL /DEBUG"
</so_link_commands>

<binary_link_commands>
default -> "{linker}"
default-debug -> "{linker} /DEBUG"
</binary_link_commands>

<mach_abi_linking>
all -> "/bigobj"

# These can be overridden with --msvc-runtime option
rt -> "/MD"
rt-debug -> "/MDd"
</mach_abi_linking>
2 changes: 1 addition & 1 deletion src/build-data/detect_version.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* configure.py to determine the compilers version number.
*/

#if defined(_MSC_VER)
#if defined(_MSC_VER) and !defined(__clang__)
Copy link
Owner

Choose a reason for hiding this comment

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

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__

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

  1. it probably doesn't hurt to handle clang-cl explicitly here,
  2. 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.


/*
_MSC_VER Defined as an integer literal that encodes the major and
Expand Down
2 changes: 1 addition & 1 deletion src/build-data/makefile.in
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ LANG_EXE_FLAGS = %{cc_lang_binary_linker_flags}
CXXFLAGS = %{cc_compile_flags}
WARN_FLAGS = %{cc_warning_flags}
LIB_FLAGS = %{lib_flags}
LDFLAGS = %{ldflags} %{cc_compile_flags}
LDFLAGS = %{ldflags}

EXE_LINK_CMD = %{exe_link_cmd}

Expand Down
2 changes: 1 addition & 1 deletion src/build-data/ninja.in
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Copy link
Owner

Choose a reason for hiding this comment

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

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


EXE_LINK_CMD = %{exe_link_cmd}

Expand Down
14 changes: 14 additions & 0 deletions src/lib/math/bigint/big_ops2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,20 @@
#include <botan/internal/mp_core.h>
#include <algorithm>

/* This hack works around an undefined reference to __udivti3() when compiling for 64-bit Windows
* using clang-cl, see:
*
* https://github.com/llvm/llvm-project/issues/25679
* https://stackoverflow.com/questions/68676184/clang-cl-error-lld-link-error-undefined-symbol-divti3
*
* I couldn't come up with a way to embed this into the build info files without extending
* configure.py to allow cpu-specific libs in the compiler info or os+cc+arch-specific libs in the
* module info. Hopefully this can go away when the above Clang issue is fixed anyway.
*/
#if defined(_WIN64) && defined(BOTAN_BUILD_COMPILER_IS_CLANGCL)
#pragma comment(lib, "clang_rt.builtins-x86_64.lib")
#endif
reneme marked this conversation as resolved.
Show resolved Hide resolved

namespace Botan {

BigInt& BigInt::add(const word y[], size_t y_words, Sign y_sign) {
Expand Down
11 changes: 9 additions & 2 deletions src/lib/prov/pkcs11/p11.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,14 @@
#define CK_DECLARE_FUNCTION(returnType, name) returnType name
#endif

#if defined(_MSC_VER)
/* When building with clang-cl, specifying this with dllimport causes a warning when defining the
* CK_C_XXX function pointer types in pkcs11.h:
*
* 'dllimport' attribute only applies to functions, variables, classes, and Objective-C interfaces
*
* I'm not convinced the dllimport is needed/approproate for typedefs under MSVC either.
*/
#if 0 // defined(_MSC_VER)
#define CK_DECLARE_FUNCTION_POINTER(returnType, name) returnType __declspec(dllimport)(*name)
#else
#define CK_DECLARE_FUNCTION_POINTER(returnType, name) returnType(*name)
reneme marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -40,7 +47,7 @@
#pragma pack(push, cryptoki, 1)
#endif

#include "pkcs11.h"
#include <pkcs11.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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.


#if defined(_MSC_VER)
#pragma pack(pop, cryptoki)
Expand Down
2 changes: 1 addition & 1 deletion src/lib/pubkey/dh/dh.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ BOTAN_DIAGNOSTIC_PUSH
BOTAN_DIAGNOSTIC_IGNORE_INHERITED_VIA_DOMINANCE

class BOTAN_PUBLIC_API(2, 0) DH_PrivateKey final : public DH_PublicKey,
public PK_Key_Agreement_Key,
public virtual PK_Key_Agreement_Key,
public virtual Private_Key {
public:
/**
Expand Down
4 changes: 2 additions & 2 deletions src/lib/pubkey/ecdh/ecdh.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ BOTAN_DIAGNOSTIC_PUSH
BOTAN_DIAGNOSTIC_IGNORE_INHERITED_VIA_DOMINANCE

class BOTAN_PUBLIC_API(2, 0) ECDH_PrivateKey final : public ECDH_PublicKey,
public EC_PrivateKey,
public PK_Key_Agreement_Key {
public virtual EC_PrivateKey,
public virtual PK_Key_Agreement_Key {
public:
/**
* Load a private key.
Expand Down
4 changes: 2 additions & 2 deletions src/lib/pubkey/ecies/ecies.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ namespace {
BOTAN_DIAGNOSTIC_PUSH
BOTAN_DIAGNOSTIC_IGNORE_INHERITED_VIA_DOMINANCE

class ECIES_PrivateKey final : public EC_PrivateKey,
public PK_Key_Agreement_Key {
class ECIES_PrivateKey final : public virtual EC_PrivateKey,
public virtual PK_Key_Agreement_Key {
public:
explicit ECIES_PrivateKey(const ECDH_PrivateKey& private_key) :
EC_PublicKey(private_key), EC_PrivateKey(private_key), PK_Key_Agreement_Key(), m_key(private_key) {}
Expand Down
4 changes: 2 additions & 2 deletions src/lib/pubkey/rsa/rsa.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ class BOTAN_PUBLIC_API(2, 0) RSA_PublicKey : public virtual Public_Key {
BOTAN_DIAGNOSTIC_PUSH
BOTAN_DIAGNOSTIC_IGNORE_INHERITED_VIA_DOMINANCE

class BOTAN_PUBLIC_API(2, 0) RSA_PrivateKey final : public Private_Key,
public RSA_PublicKey {
class BOTAN_PUBLIC_API(2, 0) RSA_PrivateKey final : public virtual Private_Key,
public virtual RSA_PublicKey {
public:
/**
* Load a private key.
Expand Down
4 changes: 2 additions & 2 deletions src/lib/tls/tls13_pqc/hybrid_public_key.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ BOTAN_DIAGNOSTIC_IGNORE_INHERITED_VIA_DOMINANCE
* Composes a number of private keys for hybrid key agreement as defined in this
* IETF draft: https://datatracker.ietf.org/doc/html/draft-ietf-tls-hybrid-design-04
*/
class BOTAN_TEST_API Hybrid_KEM_PrivateKey final : public Private_Key,
public Hybrid_KEM_PublicKey {
class BOTAN_TEST_API Hybrid_KEM_PrivateKey final : public virtual Private_Key,
public virtual Hybrid_KEM_PublicKey {
public:
/**
* Generate a hybrid private key for the given TLS code point.
Expand Down
3 changes: 2 additions & 1 deletion src/lib/utils/os_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,8 @@ bool OS::read_env_variable(std::string& value_out, std::string_view name_view) {
return false;
}

#if defined(BOTAN_TARGET_OS_HAS_WIN32) && defined(BOTAN_BUILD_COMPILER_IS_MSVC)
#if defined(BOTAN_TARGET_OS_HAS_WIN32) && \
(defined(BOTAN_BUILD_COMPILER_IS_MSVC) || defined(BOTAN_BUILD_COMPILER_IS_CLANGCL))
const std::string name(name_view);
char val[128] = {0};
size_t req_size = 0;
Expand Down
3 changes: 1 addition & 2 deletions src/lib/utils/socket/socket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,7 @@ class BSD_Socket final : public OS::Socket {
struct timeval timeout_tv = make_timeout_tv();
fd_set write_set;
FD_ZERO(&write_set);
// Weirdly, Winsock uses a SOCKET type but wants FD_SET to get an int instead
FD_SET(static_cast<int>(m_socket), &write_set);
FD_SET(m_socket, &write_set);

active = ::select(static_cast<int>(m_socket + 1), nullptr, &write_set, nullptr, &timeout_tv);

Expand Down
1 change: 1 addition & 0 deletions src/lib/x509/certstor_flatfile/certstor_flatfile.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#define BOTAN_CERT_STORE_FLATFILE_H_

#include <botan/certstor.h>
#include <botan/pkix_types.h>

#include <map>
#include <memory>
Expand Down
5 changes: 5 additions & 0 deletions src/scripts/ci/setup_gh_actions.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,8 @@ if($identifiers_for_64bit -contains $ARCH ) {
}

echo "SCCACHE_CACHE_SIZE=200M" >> $env:GITHUB_ENV

# Remove standalone LLVM (and clang-cl) from PATH - we want to use the one shipped with VS.
# https://github.com/actions/runner-images/issues/10001#issuecomment-2150541007
$no_llvm_path = ($env:PATH -split ';' | Where-Object { $_ -ne 'C:\Program Files\LLVM\bin' }) -join ';'
echo "PATH=$no_llvm_path" >> $env:GITHUB_ENV
2 changes: 2 additions & 0 deletions src/scripts/ci_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,8 @@ def main(args=None):
options.cc_bin = 'clang++'
elif options.cc == 'msvc':
options.cc_bin = 'cl'
elif options.cc == 'clangcl':
options.cc_bin = 'clang-cl'
elif options.cc == "emcc":
options.cc_bin = "em++"
else:
Expand Down
2 changes: 1 addition & 1 deletion src/scripts/ci_check_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def verify_library(build_config):

major_version = int(build_config["version_major"])

if build_config['compiler'] == 'msvc':
if build_config['compiler'] in ['msvc', 'clangcl']:
expected_lib_format = r'^botan-%d\.(dll|lib)$' % (major_version)
elif build_config['os'] == 'macos':
expected_lib_format = r'^libbotan-%d\.(a|dylib)$' % (major_version)
Expand Down
2 changes: 2 additions & 0 deletions src/tests/runner/test_xml_reporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ std::string full_compiler_name_string() {
return "xcode";
#elif defined(BOTAN_BUILD_COMPILER_IS_CLANG)
return "clang";
#elif defined(BOTAN_BUILD_COMPILER_IS_CLANGCL)
return "clangcl";
#elif defined(BOTAN_BUILD_COMPILER_IS_GCC)
return "gcc";
#elif defined(BOTAN_BUILD_COMPILER_IS_MSVC)
Expand Down
Loading