Allow building with clang-cl

[MisterDA]

clang-cl is an alternative command-line interface to Clang, designed for compatibility with the Visual C/C++ compiler, cl.exe.

clang-cl is ABI and (mostly) command-line compatible with cl, providing a drop-in replacement. Users may prefer it to MSVC as it's based on LLVM/clang, a free software compiler toolchain,

which aims to deliver amazingly fast compiles, extremely useful error and warning messages and to provide a platform for building great source level tools.

I've added a job to the CI matrix to test OCaml with clang-cl on Windows x86_64. To test this PR locally, follow Clang/LLVM support in Visual Studio projects, then configure OCaml with:

./configure --host=x86_64-pc-windows CC=clang-cl

Having another compilers' opinion on Windows and MSVC (by opposition to MinGW-w64) specific code paths can only benefit the quality of the C code in OCaml. In the future, this could also allow cross-compiling OCaml for native Windows from any host running clang, after installing Windows headers.

This PR accounts for some (in)compatibility quirks of cl and clang-cl CLI, fixes little configure.ac bugs introduced in the MSVC restoration effort, adds the required macros to ignore (innocuous) deprecation warnings raised by clang, and adds the only C patch required to make it build out of the box.

[MisterDA]

Out of curiosity, do you have a sense of how much work it would be/how feasible it would be to support clang (not clang-cl) to build on Windows? Thanks!

I tried it once, I think the blockers are the same as Upgrade from msvcrt.lib to ucrt.lib #122 , and it would require probably require a new FlexDLL chain.

[MisterDA]

I've rebased on top of trunk to get the C fix I extracted from this PR.

Another advantage of clang-cl is that it supports a lot of GCC extensions, such as attributes. clang-cl doesn't masquerade as GNU C (doesn't define __GNUC__), but if we switch to detecting attributes support (with __has_attribute or C23 __has_c_attribute) instead of detecting them based on compiler-specific macros (_MSC_VER, __clang__, __GNUC__, …) we can leverage them on Windows too.

[MisterDA]

This looks good to me! Thank you for cleaning the duplicate flags in MSVC cl calls while you were at it 😄

Thanks for the review!

I’m a bit confused by the Autoconf tests won't pick up… paragraph in the commit message for Reorganize MSVC atomics cflags: I read it as "The flags must be in CFLAGS otherwise the test fails, but we don’t want them to be in CFLAGS".

Yes, that's exactly what's happening. The Autoconf tests uses CFLAGS for the compiler invocation, but we don't modify this variable in the configure script. If we did, we'd need to be careful not to overwrite user-specified flags (as in ./configure CFLAGS=-myflags) or end up with duplicated flags here:

https://github.com/ocaml/ocaml/blob/e719a1f78c825c1fff4c10c2c47e044149da4caf/configure.ac#L2597-L2598

but @shindere has a PR removing this behavior: Don't use configured CFLAGS & CPPFLAGS to compile third-party C sources #12589.

I do think I've found the sweet spot by passing new CFLAGS to OCAML_CC_SUPPORTS_ATOMIC. This allows to discover what flags are really needed without modifying the global CFLAGS variable; and as no other configure test use C11 atomics or include OCaml headers, they're no risk of a test failing because the compiler invocation would be missing a flag.

I can try to rewrite the commit message if it's not clear enough.

[shym]

It was just the phrasing that got me confused then. Would the following sound good?

Autoconf tests won't pick up the flags if they're not also in CFLAGS,
otherwise failing atomics support detection. So the necessary flags are
added to CFLAGS but only for the OCAML_CC_SUPPORTS_ATOMIC test,
otherwise they would end up duplicated in the final configuration.

[nojb]

@shym: are you happy with the current state of the PR? I am happy to approve it on your behalf based on your quite thorough review.

[shym]

@nojb: I just did a last range-diff and I have no further suggestion, I’m happy with the current state of the PR.

[shindere]

Many thanks to all for the hard work both in authoring and reviewing htis PR.

I intend to go through it and take care of its merge, which should preferably happen after the merge of #13095, which I intend to deal with, too.

[dra27]

I had a closer look at things surrounding https://github.com/ocaml/ocaml/pull/13093/commits/55916b0b7d39694ff5fc92c21f8a0d9d7bebe586 and I have a fix for winpthreads in https://github.com/dra27/ocaml/commit/01d9ee555f781a242c35c0d07d74cfce3975e97b and then an overall simplification of all the autoconf-make meta-programming in https://github.com/dra27/ocaml/commit/b88157c1724b9b46a53f61ef3b144c676b8bc631 but then a much simpler fix for the problem with ocamltest/ocamltest_config.ml occurred to me... it doesn't need to be using $internal_cppflags at all... it's testing the compiler, it both morally and technically ought to be using $common_cppflags.

I tried this in https://github.com/dra27/ocaml/commit/12238ffd542f45485f3b1b8bed4a31aa57b89c9e which, when combined with your https://github.com/ocaml/ocaml/pull/13093/commits/2367ceb6e6c43eb85589ced78e70669efd4d77d3 means that ocamltest_config.ml now uses both $common_cflags and $common_cppflags. There are only two tests which use those variables - one of them is simply using the preprocessor for actual pre-processing (it actually doesn't care about any special flags at all) and the other is the -output-complete-obj test which doesn't need either of the -I or the -DWINDOWS_UNICODE flags at all.

So, I think I'd like to suggest dropping https://github.com/ocaml/ocaml/pull/13093/commits/55916b0b7d39694ff5fc92c21f8a0d9d7bebe586 and, if you're happy, then cherry-picking just https://github.com/dra27/ocaml/commit/12238ffd542f45485f3b1b8bed4a31aa57b89c9e. Everything else in this PR looks great - I particularly like the cleanups around windows_unicode and the flags detection, thank you!

Once this is in, I'll open a separate PR with the other two commits, because they're orthogonal to this PR, once ocamltest_config.ml is being correctly generated.

[MisterDA]

I had a closer look at things […]

Thanks for the review and the help!

So, I think I'd like to suggest dropping 55916b0 and, if you're happy, then cherry-picking just dra27@12238ff.

Done.

[MisterDA]

Are there sufficiently few places that it could be done with #define _WINSOCK_DEPRECATED_NO_WARNINGS /* Remove when gethostbyname replaced with getaddrinfo */ in the files? (I'm not 100% convinced this would be better)

Yes, the problem happens only in runtime/debugger.c. I'll move the define there. I already have written the patches to update this code, and reverting this simple commit will be even simpler.

EDIT: my bad, it's also needed in libunix, of course. We'll need to keep the macro there because we still want to provide bindings for deprecated functions.

[MisterDA]

Many thanks to all the reviewers! I think I've taken everything into account, and I've rebased the PR on trunk. All set?

[shindere]

Antonin Décimo (2024/04/22 10:40 -0700):

Many thanks to all the reviewers! I think I've taken everything into account, and I've rebased the PR on trunk. All set?

I think we are (almost?) there!

I have a few neits about the unicode commit but they may be worth left for another day, after all.

Many thanks to you, @MisterDA, for the super work you have done here.

I also thank the other reviewers for their very nice reviews!

[shindere]

Merging, as thit opens the way for a few already-planned subsequent improvements.