Page MenuHomePhabricator

[libc++] Disable int128_t and ship filesystem on Windows by default
Needs ReviewPublic

Authored by ldionne on Sep 29 2022, 2:50 PM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

Back in 2020 [1], we went very close to enabling Filesystem on Windows
by disabling int128_t, but decided to wait because Windows 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
Windows. This will make <filesystem> available by default on Windows,
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

Diff Detail

Event Timeline

ldionne created this revision.Sep 29 2022, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 2:50 PM
ldionne requested review of this revision.Sep 29 2022, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 2:50 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a subscriber: Restricted Project.Sep 29 2022, 2:51 PM

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.

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?)

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

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.

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.

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 added a comment.Sep 30 2022, 2:49 PM

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.

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.

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

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?

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).

No objections from me either.