This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Expand the contents of LIBCXX_ENABLE_FILESYSTEM
ClosedPublic

Authored by ldionne on Jun 7 2023, 9:29 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Commits
rGc352fa740712: [libc++] Expand the contents of LIBCXX_ENABLE_FILESYSTEM
Summary

Since LIBCXX_ENABLE_FILESYSTEM now truly represents whether the
platform supports a filesystem (as opposed to whether the <filesystem>
library is provided), we can provide a few additional classes from
the <filesystem> library even when the platform does not have support
for a filesystem. For example, this allows performing path manipulations
using std::filesystem::path even on platforms where there is no actual
filesystem.

rdar://107061236

Diff Detail

Event Timeline

ldionne created this revision.Jun 7 2023, 9:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2023, 9:29 AM
ldionne requested review of this revision.Jun 7 2023, 9:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2023, 9:29 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

This one isn't ready for review yet. I'm uploading it so that folks can see the end goal I'm driving towards with the long stack of patches.

ldionne planned changes to this revision.Jun 7 2023, 9:32 AM
ldionne updated this revision to Diff 532665.Jun 19 2023, 8:09 AM

Rebase onto main and complete patch (to my knowledge, CI will tell).

ldionne updated this revision to Diff 532938.Jun 20 2023, 7:58 AM

Rebase after editing parent revisions to poke CI

ldionne updated this revision to Diff 533033.Jun 20 2023, 1:29 PM

Rebase after fixing format.

ldionne updated this revision to Diff 533222.Jun 21 2023, 5:34 AM

Add missing includes

ldionne updated this revision to Diff 533256.Jun 21 2023, 7:50 AM

Fix modular CI

Thanks for working on this! In general LGTM, but a I have some questions.

libcxx/include/__filesystem/copy_options.h
22

I think it makes sense to mention this in the release notes.

libcxx/include/__filesystem/directory_options.h
24

Why can this be removed here and not in other places?

libcxx/include/fstream
199

Did you see the comments in D152168 and the patch D153327?

ldionne marked 2 inline comments as done.Jun 21 2023, 10:42 AM
ldionne added inline comments.
libcxx/include/__filesystem/directory_options.h
24

It's removed here because it does not fundamentally depend on anything in the dylib. Originally, I had marked everything in <filesystem> with availability macros using this PUSH macro. When <filesystem> was split out, the PUSH was copied to all the sub-headers, even though some parts of <filesystem> did not technically require it because they don't depend on anything in the dylib.

libcxx/include/fstream
199

I did not until now, but thanks for the heads up.

ldionne updated this revision to Diff 533339.Jun 21 2023, 11:03 AM
ldionne marked an inline comment as done.

Rebase and update w/ release note.

ldionne updated this revision to Diff 533552.Jun 22 2023, 4:09 AM

Fix Windows and back-deployment issues.

ldionne updated this revision to Diff 534528.Jun 26 2023, 6:44 AM

Add missing backdeployment annotations and rebase.

ldionne updated this revision to Diff 534649.Jun 26 2023, 10:50 AM

Rebase and fix CI issues in added tests.

ldionne accepted this revision as: Restricted Project.Jun 27 2023, 6:18 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 27 2023, 6:19 AM
This revision was automatically updated to reflect the committed changes.

Hi! This patch seems to break some of the Fuchsia builders. Please find the URL for the failed build below:

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8777129470970901217/+/u/clang/install/stdout

[16/25](2) Building CXX object libcxx/src/CMakeFiles/cxx_static.dir/filesystem/filesystem_clock.cpp.obj
FAILED: libcxx/src/CMakeFiles/cxx_static.dir/filesystem/filesystem_clock.cpp.obj 
/b/s/w/ir/x/w/staging/llvm_build/./bin/clang-cl --target=x86_64-pc-windows-msvc  /nologo -TP -DUNICODE -D_ALLOW_ITERATOR_DEBUG_LEVEL_MISMATCH -D_ALLOW_MSC_VER_MISMATCH -D_CRTBLD -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -D_GLIBCXX_ASSERTIONS -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS="" -D_LIBCPP_ENABLE_ASSERTIONS -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src -I/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1 -I/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-pc-windows-msvc/c++/v1 -Xclang -ivfsoverlay -Xclang /b/s/w/ir/cache/windows_sdk/llvm-vfsoverlay.yaml /winsysroot /b/s/w/ir/cache/windows_sdk /Zc:inline /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw -no-canonical-prefixes /MD /Zi /O2 /Ob1 -std:c++20 -UNDEBUG -W4 -Wextra -Wnewline-eof -Wshadow -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wundef -Wunused-template -Wformat-nonliteral -Wno-user-defined-literals -Wno-covered-switch-default -Wno-suggest-override -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-c++11-compat -Wno-undef -Wno-reserved-id-macro -Wno-gnu-include-next -Wno-gcc-compat -Wno-zero-as-null-pointer-constant -Wno-deprecated-dynamic-exception-spec -Wno-sign-conversion -Wno-old-style-cast -Wno-deprecated -Wno-shift-sign-overflow -Wno-double-promotion -Wno-error -EHsc /showIncludes /Folibcxx/src/CMakeFiles/cxx_static.dir/filesystem/filesystem_clock.cpp.obj /Fdlibcxx/src/CMakeFiles/cxx_static.dir/cxx_static.pdb -c -- /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/filesystem_clock.cpp
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/filesystem_clock.cpp:14:
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/posix_compat.h(362,28): error: use of undeclared identifier 'status'
  362 |     const file_status st = status(dir, local_ec);
      |                            ^
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/posix_compat.h(363,10): error: use of undeclared identifier 'exists'; did you mean 'exit'?
  363 |     if (!exists(st) || is_directory(st))
      |          ^~~~~~
      |          exit
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/cstdlib(126,9): note: 'exit' declared here
  126 | using ::exit _LIBCPP_USING_IF_EXISTS;
      |         ^
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/filesystem_clock.cpp:14:
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/posix_compat.h(363,17): error: no viable conversion from 'const file_status' to 'int'
  363 |     if (!exists(st) || is_directory(st))
      |                 ^~
/b/s/w/ir/cache/windows_sdk/Windows Kits/10/Include/10.0.19041.0/ucrt/stdlib.h(56,62): note: passing argument to parameter '_Code' here
   56 |     _ACRTIMP __declspec(noreturn) void __cdecl exit(_In_ int _Code);
      |                                                              ^
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/filesystem_clock.cpp:14:
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/posix_compat.h(363,24): error: use of undeclared identifier 'is_directory'
  363 |     if (!exists(st) || is_directory(st))
      |                        ^
4 errors generated.
[17/25](1) Building CXX object compiler-rt/lib/fuzzer/CMakeFiles/RTfuzzer.x86_64.dir/FuzzerUtilWindows.cpp.obj
[18/25](0) Building CXX object compiler-rt/lib/orc/CMakeFiles/RTOrc.x86_64.dir/coff_platform.cpp.obj
In file included from /b/s/w/ir/x/w/llvm-llvm-project/compiler-rt/lib/orc/coff_platform.cpp:20:

We are investigating this from our end as well.

phosek added a subscriber: phosek.Jun 27 2023, 4:48 PM

We reproduced the issue. We currently disable filesystem support on Windows by setting LIBCXX_ENABLE_FILESYSTEM to OFF, see https://github.com/llvm/llvm-project/blob/7075f9d926c93efefacb72caf17e93ae00b3703b/clang/cmake/caches/Fuchsia-stage2.cmake#L122. Even when filesystem is disabled, libc++ still builds filesystem_clock.cpp (https://github.com/llvm/llvm-project/blob/be02f912d6b9b515a1d1b30cf06dfff1c085ea34/libcxx/src/CMakeLists.txt#L15) and filesystem_clock.cpp includes posix_compat.h (https://github.com/llvm/llvm-project/blob/be02f912d6b9b515a1d1b30cf06dfff1c085ea34/libcxx/src/filesystem/filesystem_clock.cpp#L14) which on Windows calls functions like status (https://github.com/llvm/llvm-project/blob/be02f912d6b9b515a1d1b30cf06dfff1c085ea34/libcxx/src/filesystem/posix_compat.h#L362), but that function is declared in __filesystem/operations.h (https://github.com/llvm/llvm-project/blob/be02f912d6b9b515a1d1b30cf06dfff1c085ea34/libcxx/include/__filesystem/operations.h#L187) but after this change, all those declarations aren't omitted when LIBCXX_ENABLE_FILESYSTEM is disabled (https://github.com/llvm/llvm-project/blob/be02f912d6b9b515a1d1b30cf06dfff1c085ea34/libcxx/include/__filesystem/operations.h#L31).

Can we either revert this change, or at least undo the change to __filesystem/operations.h?

This also affects wasi for the same reason (filesystem_clock.cpp still being compiled), but a different cause: wasi doesn't have sys/statvfs.h, so the failure is

libcxx/src/filesystem/posix_compat.h:42:11: fatal error: 'sys/statvfs.h' file not found

This also affects wasi for the same reason (filesystem_clock.cpp still being compiled), but a different cause: wasi doesn't have sys/statvfs.h, so the failure is

libcxx/src/filesystem/posix_compat.h:42:11: fatal error: 'sys/statvfs.h' file not found

Same issue for me.

The fix here would probably be to split TimeSpec out of posix_compat.h so that it can be included from filesystem_clock.cpp without pulling in all of posix_compat.h. Or we could also conditionalize the definition of some things inside posix_compat.h based on _LIBCPP_HAS_NO_FILESYSTEM.

TBH, it's kind of hard for me to formulate a patch without seeing exactly what the issue is and being able to reproduce, given that those are not part of our CI configurations. I think it would be a lot easier if one of the affected folks came up with that patch -- I'll be happy to review it, it's just that I'll be shooting blind if I try to fix this issue for you folks without being able to see it first hand.

The fix here would probably be to split TimeSpec out of posix_compat.h so that it can be included from filesystem_clock.cpp without pulling in all of posix_compat.h. Or we could also conditionalize the definition of some things inside posix_compat.h based on _LIBCPP_HAS_NO_FILESYSTEM.

TBH, it's kind of hard for me to formulate a patch without seeing exactly what the issue is and being able to reproduce, given that those are not part of our CI configurations. I think it would be a lot easier if one of the affected folks came up with that patch -- I'll be happy to review it, it's just that I'll be shooting blind if I try to fix this issue for you folks without being able to see it first hand.

If that helps, our setup is baremetal newlib (custom target). Newlib provides some sys headers, but apparently not this one https://cygwin.com/git/?p=newlib-cygwin.git;a=tree;f=newlib/libc/include/sys.
I guess it would be the same with picolibc https://github.com/picolibc/picolibc/tree/main/newlib/libc/include/sys.

If that helps, our setup is baremetal newlib (custom target). Newlib provides some sys headers, but apparently not this one https://cygwin.com/git/?p=newlib-cygwin.git;a=tree;f=newlib/libc/include/sys.
I guess it would be the same with picolibc https://github.com/picolibc/picolibc/tree/main/newlib/libc/include/sys.

https://reviews.llvm.org/D154246 doesn't seem to have run into the same issues (so far).

Anyway, I'm trying something out in D154390 but it's a bit of a blind attempt.

Anyway, I'm trying something out in D154390 but it's a bit of a blind attempt.

FWIW, that fixes the issue for wasi.

If that helps, our setup is baremetal newlib (custom target). Newlib provides some sys headers, but apparently not this one https://cygwin.com/git/?p=newlib-cygwin.git;a=tree;f=newlib/libc/include/sys.
I guess it would be the same with picolibc https://github.com/picolibc/picolibc/tree/main/newlib/libc/include/sys.

https://reviews.llvm.org/D154246 doesn't seem to have run into the same issues (so far).

https://reviews.llvm.org/D154246#4468049

Anyway, I'm trying something out in D154390 but it's a bit of a blind attempt.

Thanks!

Anyway, I'm trying something out in D154390

This doesn't yet fix the issue when building with picolibc. It fails with:

filesystem_clock.cpp:42:12: error: use of undeclared identifier 'clock_gettime'

I think an option is needed in libcxx/CMakeLists.txt to disable building filesystem_clock.cpp.

it's a bit of a blind attempt.

To help with this you can apply the patch from D154246 and run CXX=clang++ CC=clang libcxx/utils/ci/run-buildbot armv6m-picolibc.

Anyway, I'm trying something out in D154390

This doesn't yet fix the issue when building with picolibc. It fails with:

filesystem_clock.cpp:42:12: error: use of undeclared identifier 'clock_gettime'

I think an option is needed in libcxx/CMakeLists.txt to disable building filesystem_clock.cpp.

Does that mean the platform doesn't provide a way to check the time, or is it that the API it provides is just different? How is e.g. std::system_clock implemented on that platform?

FWIW this looks related: D88825

michaelplatings added a comment.EditedJul 4 2023, 8:39 AM

Does that mean the platform doesn't provide a way to check the time, or is it that the API it provides is just different? How is e.g. std::system_clock implemented on that platform?

From chrono.cpp:

#if !defined(__APPLE__) && defined(_POSIX_TIMERS) && _POSIX_TIMERS > 0
# define _LIBCPP_USE_CLOCK_GETTIME
#endif

...

#elif defined(CLOCK_REALTIME) && defined(_LIBCPP_USE_CLOCK_GETTIME)

...

#else

static system_clock::time_point __libcpp_system_clock_now() {
    timeval tv;
    gettimeofday(&tv, 0);
    return system_clock::time_point(seconds(tv.tv_sec) + microseconds(tv.tv_usec));
}

#endif

Picolibc does not define _POSIX_TIMERS so it looks like gettimeofday is used. Perhaps the same could be done in filesystem_clock.cpp?

libcxx/test/libcxx/input.output/filesystems/class.path/path.member/path.native.obs/string_alloc.pass.cpp