[libc++] Disable int128_t and ship filesystem on MSVC by default

[ldionne]

Summary

Back in 2020 [1], we went very close to enabling Filesystem on MSVC
by disabling int128_t, but decided to wait because MSVC support
for int128_t was supposed to come shortly after. Since it's not
there yet, I propose turning off int128_t support by default on MSVC.
This will make <filesystem> available by default on MSVC, and most
importantly will open the possibility for changing
LIBCXX_ENABLE_FILESYSTEM to mean "the system doesn't have support
for a filesystem" instead of simply "don't build the std::filesystem
library", which is what I'm really after with this change.

In a way, this is a resurection of D91139.

[1]: https://reviews.llvm.org/D91139#2429595

[ldionne]

I'd like to note that it would be much better to just ship int128_t support on Windows though, because if we do that later it will constitute an ABI break.

[mstorsjo]

Generally, s/Windows/MSVC/ for the whole commit message, since there’s no issue for mingw configurations (I’ve shipped std::filesystem for 1,5 years there already, with the implementation using int128). Other than that, it does look reasonable to me.

One of the arguments back then was that it would be an ABI break when/if we’d enable int128 in the future. At this point, when int128 support isn’t really here yet, I kinda agree that we’ll just have to take that ABI break then (IIRC we generally consider the MSVC/clang-cl port of libc++ not entirely ABI stable anyway?)

[Mordante]

Are you sure it's not available yet? I think 128 bit support has been by @CaseyCarter.
https://github.com/microsoft/STL/blob/main/stl/inc/__msvc_int128.hpp

[mstorsjo]

In D134912#3826133, @Mordante wrote:

Are you sure it's not available yet? I think 128 bit support has been by @CaseyCarter.
https://github.com/microsoft/STL/blob/main/stl/inc/__msvc_int128.hpp

AFAIK that's a different thing. That's a C++ class implementation of operations on 128 integers, based on smaller integers. For libc++'s part, it just uses plain Clang's __int128_t raw data type, which Clang/LLVM generally can generate code for just fine. The problem is that whenever there's a division involved, Clang/LLVM doesn't generate inline code for doing the division, and expects to call the libgcc/compiler-rt builtin __divti3 (and a couple others). For other targets, libgcc or compiler-rt provide the implementation of this function, but for MSVC based environments, it's not available.

There has been discussions of ways of making compiler-rt builtins practically usable in a MSVC environment, and/or Microsoft even shipping those relevant builtins as part of the regular vcruntime libraries that are used in MSVC environments.

[ldionne]

In D134912#3825356, @mstorsjo wrote:

Generally, s/Windows/MSVC/ for the whole commit message,

Right, I'll fix that.

I know @rnk had discussed shipping int128_t support a while back, I'd like to hear from him whether there are plans to do this in the short term (the next few weeks). Otherwise, we'll ship this and take a break if we want to use int128_t support in filesystem in the future (which I guess we don't have to). We have other similar situations w.r.t. ranges already FWIW (with no_unique_address), so there is precedent for not being fully ABI stable on Windows.

[mstorsjo]

In D134912#3826856, @ldionne wrote:

I know @rnk had discussed shipping int128_t support a while back, I'd like to hear from him whether there are plans to do this in the short term (the next few weeks). Otherwise, we'll ship this and take a break if we want to use int128_t support in filesystem in the future

IIRC there's a couple other places in libc++ where int128_t is used too, if available, so there might be changes in a few other spots than only std::filesystem.

[rnk]

In D134912#3826856, @ldionne wrote:

In D134912#3825356, @mstorsjo wrote:

Generally, s/Windows/MSVC/ for the whole commit message,

Right, I'll fix that.

I know @rnk had discussed shipping int128_t support a while back, I'd like to hear from him whether there are plans to do this in the short term (the next few weeks). Otherwise, we'll ship this and take a break if we want to use int128_t support in filesystem in the future (which I guess we don't have to). We have other similar situations w.r.t. ranges already FWIW (with no_unique_address), so there is precedent for not being fully ABI stable on Windows.

Sorry, I think this task was assigned to someone who has since left our team, and it's not likely to happen soon.

I think the way forward here is to add /DEFAULTLIB:clang_rt.builtins-${arch}.lib to LLVM objects when i128 is present. I think we have some prior art for this around the _fltused symbol.

[mstorsjo]

In D134912#3828421, @rnk wrote:

In D134912#3826856, @ldionne wrote:

In D134912#3825356, @mstorsjo wrote:

Generally, s/Windows/MSVC/ for the whole commit message,

Right, I'll fix that.

I know @rnk had discussed shipping int128_t support a while back, I'd like to hear from him whether there are plans to do this in the short term (the next few weeks). Otherwise, we'll ship this and take a break if we want to use int128_t support in filesystem in the future (which I guess we don't have to). We have other similar situations w.r.t. ranges already FWIW (with no_unique_address), so there is precedent for not being fully ABI stable on Windows.

Sorry, I think this task was assigned to someone who has since left our team, and it's not likely to happen soon.

I think the way forward here is to add /DEFAULTLIB:clang_rt.builtins-${arch}.lib to LLVM objects when i128 is present. I think we have some prior art for this around the _fltused symbol.

I think that might work. MSVC does ship those libraries (in VC/Tools/MSVC/<version>/lib/<arch> today - however, they only ship them for x86 and x64 - they're missing for arm and arm64, so we'd need to file a request for them to start shipping those files (at least builtins, the sanitizers aren't equally essential) for all architectures.

If considering libraries shipped with Clang as opposed to ones shipped with MSVC, we already have prior art saying that the user may need to specify the path to the Clang runtime library directory manually to the linker, since b8000c0ce84541c5b5535419234fb65ce77d6756. Previously that only extended to when the user knowingly was using sanitizers, now it would also happen when unwittingly using int128_t.

There's a potential issue ahead, if we're switching the library naming due to LLVM_ENABLE_PER_TARGET_RUNTIME_DIR (CC @phosek @MaskRay), since we probably don't know which library naming is the preferred one at the point where we're injecting the defaultlib directive.

[tschuett]

IIRC, the LLVM libc had similar issues. They ship their own 128-bit integer where needed.

[ldionne]

So my understanding is that there's a way forward to enable int128_t on Windows by linking the appropriate support from the LLVM side. Is anyone opposing to this patch?

[mstorsjo]

In D134912#3882738, @ldionne wrote:

So my understanding is that there's a way forward to enable int128_t on Windows by linking the appropriate support from the LLVM side. Is anyone opposing to this patch?

No objection from me (but remember to do s/Windows/MSVC/, or s/Windows/clang-cl/ on the commit message before committing), but please wait for acks from others who actually use it in this configurations before proceeding (I don't use it in this configuration, only use it for some reference testing).

[Mordante]

No objections from me either.

[mstorsjo]

D146190 finally formalized that we expect the libc++ ABI to be not entirely stable for MSVC targets, so I think that should clear any blockers for moving forward with this patch.

[ldionne]

Rebase and update commit message.

I will ship this once the CI is green.

[ldionne]

Accepting per discussion above.

libcxx/CMakeLists.txt

 
 # Basic options ---------------------------------------------------------------
 option(LIBCXX_ENABLE_ASSERTIONS
   "Enable assertions inside the compiled library, and at the same time make it the
    default when compiling user code. Note that assertions can be enabled or disabled
    by users in their own code regardless of this option." OFF)
 option(LIBCXX_ENABLE_SHARED "Build libc++ as a shared library." ON)
 option(LIBCXX_ENABLE_STATIC "Build libc++ as a static library." ON)
-set(ENABLE_FILESYSTEM_DEFAULT ON)
-if (WIN32 AND NOT MINGW)
-  # Filesystem is buildable for windows, but it requires __int128 helper
-  # functions, that currently are provided by libgcc or compiler_rt builtins.
-  # These are available in MinGW environments, but not currently in MSVC
-  # environments.
-  set(ENABLE_FILESYSTEM_DEFAULT OFF)
-endif()
-option(LIBCXX_ENABLE_FILESYSTEM "Build filesystem as part of the main libc++ library"
-    ${ENABLE_FILESYSTEM_DEFAULT})
+option(LIBCXX_ENABLE_FILESYSTEM "Build filesystem as part of the main libc++ library" ON)
 option(LIBCXX_INCLUDE_TESTS "Build the libc++ tests." ${LLVM_INCLUDE_TESTS})
 option(LIBCXX_ENABLE_DEBUG_MODE
   "Whether to build libc++ with the debug mode enabled.
    By default, this is turned off. Turning it on results in a different ABI (additional
    symbols but also potentially different layouts of types), and one should not mix code
    built against a dylib that has debug mode and code built against a regular dylib." OFF)
 option(LIBCXX_ENABLE_RANDOM_DEVICE
   "Whether to include support for std::random_device in the library. Disabling

libcxx/include/__config

 // clang-format on
 
 #  define _VSTD_FS std::__fs::filesystem
 
 #  if __has_attribute(__enable_if__)
 #    define _LIBCPP_PREFERRED_OVERLOAD __attribute__((__enable_if__(true, "")))
 #  endif
 
-#  ifndef __SIZEOF_INT128__
+#if !defined(__SIZEOF_INT128__) || defined(_MSC_VER)
 #    define _LIBCPP_HAS_NO_INT128
 #  endif
 
 #  if __has_attribute(__malloc__)
 #    define _LIBCPP_NOALIAS __attribute__((__malloc__))
 #  else
 #    define _LIBCPP_NOALIAS
 #  endif

libcxx/utils/ci/run-buildbot

 function generate-cmake() {
     generate-cmake-base \
           -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind" \
           -DLIBCXX_CXX_ABI=libcxxabi \
           "${@}"
 }
 
 function generate-cmake-libcxx-win() {
-    # TODO: Clang-cl in MSVC configurations don't have access to compiler_rt
-    # builtins helpers for int128 division. See
-    # https://reviews.llvm.org/D91139#2429595 for a comment about longterm
-    # intent for handling the issue. In the meantime, define
-    # -D_LIBCPP_HAS_NO_INT128 (both when building the library itself and
-    # when building tests) to allow enabling filesystem for running tests,
-    # even if it uses a non-permanent ABI.
     generate-cmake-base \
           -DLLVM_ENABLE_RUNTIMES="libcxx" \
           -DCMAKE_C_COMPILER=clang-cl \
           -DCMAKE_CXX_COMPILER=clang-cl \
-          -DLIBCXX_ENABLE_FILESYSTEM=YES \
-          -DLIBCXX_EXTRA_SITE_DEFINES="_LIBCPP_HAS_NO_INT128" \
           "${@}"
 }
 
 function check-runtimes() {
     echo "--- Installing libc++, libc++abi and libunwind to a fake location"
     ${NINJA} -vC "${BUILD_DIR}" install-cxx install-cxxabi install-unwind
 
     echo "+++ Running the libc++ tests"