This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Split sources for <filesystem>
ClosedPublic

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

Details

Reviewers
philnik
Mordante
Group Reviewers
Restricted Project
Commits
rGc7d3c84449f4: [libc++] Split sources for <filesystem>
Summary

The operations.cpp file contained the implementation of a ton of
functionality unrelated to just the filesystem operations. Splitting
this up into more files will make it possible in the future to support
parts of <filesystem> (e.g. path) on systems where there is no notion
of a filesystem.

Diff Detail

Event Timeline

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

LGTM with green CI. Do you plant to update this to the current style in a follow-up patch?

This revision is now accepted and ready to land.Jun 7 2023, 9:21 AM

LGTM with green CI. Do you plant to update this to the current style in a follow-up patch?

I didn't plan to, but I could. It looks like there aren't that many changes required to make it comply to our clang-format. I'll look into it later but I'd like to avoid doing this before landing my stack of changes cause I want to avoid rebase nightmares.

ldionne updated this revision to Diff 529388.Jun 7 2023, 11:41 AM

Add files to ignore-format list.

Mordante accepted this revision.Jun 8 2023, 10:46 AM
Mordante added a subscriber: Mordante.

LGTM, when the CI is happy. On small comment.

libcxx/src/filesystem/directory_entry.cpp
10

Please also check the other new files.

ldionne updated this revision to Diff 530595.Jun 12 2023, 10:56 AM
ldionne marked an inline comment as done.

Add -Wunused-function workaround to header files and rebase.

ldionne updated this revision to Diff 530953.Jun 13 2023, 9:52 AM

Try to fix circular header dependency

ldionne updated this revision to Diff 531016.Jun 13 2023, 11:42 AM

It seems like I have to roll up D152379 and D152381 into this patch for anything to work, unfortunately.

ldionne updated this revision to Diff 531064.Jun 13 2023, 2:16 PM

Add missing include.

ldionne updated this revision to Diff 531105.Jun 13 2023, 4:03 PM

Add missing include. This is annoying!

ldionne updated this revision to Diff 531829.Jun 15 2023, 10:47 AM

Remove temporary CI changes and rebase.

ldionne updated this revision to Diff 531994.Jun 15 2023, 11:09 PM

Fix formatting.

ldionne updated this revision to Diff 532130.Jun 16 2023, 6:45 AM

Fix generated files

ldionne updated this revision to Diff 532232.Jun 16 2023, 10:54 AM

Fix GCC issues.

This revision was landed with ongoing or failed builds.Jun 19 2023, 6:07 AM
This revision was automatically updated to reflect the committed changes.

We're seeing some failures in our CI that seem related to this patch. I think either some CMAKE needs adjusting or maybe there is a stale path in the preprocessor directives that cause something to be missed.

Bot: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-xarm64/b8777855578345589777/overview

Error Message:

FAILED: libcxx/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.obj 
/b/s/w/ir/x/w/staging/llvm_build/./bin/clang++ --target=x86_64-unknown-fuchsia --sysroot=/b/s/w/ir/x/w/sdk/arch/x64/sysroot -DLIBCXX_BUILDING_LIBCXXABI -D_GLIBCXX_ASSERTIONS -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_DISABLE_NEW_DELETE_DEFINITIONS -D_LIBCPP_ENABLE_ASSERTIONS -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -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/tools/clang/stage2-bins/include/c++/v1 -I/b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/include/x86_64-unknown-fuchsia/c++/v1 -I/b/s/w/ir/x/w/llvm-llvm-project/libcxxabi/include -resource-dir=/b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/./lib/clang/17 --target=x86_64-unknown-fuchsia -I/b/s/w/ir/x/w/sdk/pkg/sync/include -I/b/s/w/ir/x/w/sdk/pkg/fdio/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -ffunction-sections -fdata-sections -ffile-prefix-map=/b/s/w/ir/x/w/staging/llvm_build/tools/clang/stage2-bins/runtimes/runtimes-x86_64-unknown-fuchsia-bins=../staging/llvm_build/tools/clang/stage2-bins/runtimes/runtimes-x86_64-unknown-fuchsia-bins -ffile-prefix-map=/b/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -O2 -g -DNDEBUG -std=c++20 -fPIC -UNDEBUG -faligned-allocation -nostdinc++ -fvisibility-inlines-hidden -fvisibility=hidden -Wall -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-error -MD -MT libcxx/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.obj -MF libcxx/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.obj.d -o libcxx/src/CMakeFiles/cxx_shared.dir/filesystem/operations.cpp.obj -c /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/operations.cpp
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/operations.cpp:25:
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/time_utils.h:263:16: error: incomplete result type 'TimeVal' (aka 'std::filesystem::detail::(anonymous namespace)::timeval') in function definition
  263 | inline TimeVal make_timeval(TimeSpec const& ts) {
      |                ^
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/posix_compat.h:119:24: note: forward declaration of 'std::filesystem::detail::(anonymous namespace)::timeval'
  119 | using TimeVal = struct timeval;
      |                        ^
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/operations.cpp:25:
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/time_utils.h:266:54: error: member access into incomplete type 'std::filesystem::detail::(anonymous namespace)::timeval'
  266 |     using int_type = decltype(std::declval<TimeVal>().tv_usec);
      |                                                      ^
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/posix_compat.h:119:24: note: forward declaration of 'std::filesystem::detail::(anonymous namespace)::timeval'
  119 | using TimeVal = struct timeval;
      |                        ^
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/operations.cpp:25:
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/time_utils.h:268:24: error: unknown type name 'int_type'
  268 |     return static_cast<int_type>(dur);
      |                        ^
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/time_utils.h:270:11: error: variable has incomplete type 'TimeVal' (aka 'std::filesystem::detail::(anonymous namespace)::timeval')
  270 |   TimeVal TV = {};
      |           ^
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/posix_compat.h:119:24: note: forward declaration of 'std::filesystem::detail::(anonymous namespace)::timeval'
  119 | using TimeVal = struct timeval;
      |                        ^
In file included from /b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/operations.cpp:25:
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/time_utils.h:278:11: error: variable has incomplete type 'TimeVal' (aka 'std::filesystem::detail::(anonymous namespace)::timeval')
  278 |   TimeVal ConvertedTS[2] = {make_timeval(TS[0]), make_timeval(TS[1])};
      |           ^
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/src/filesystem/posix_compat.h:119:24: note: forward declaration of 'std::filesystem::detail::(anonymous namespace)::timeval'
  119 | using TimeVal = struct timeval;
      |                        ^
5 errors generated.

Chatted w/ @phosek offline and he thinks this may require some changes to the Fuchsia headers as the failure is happening in Fuchsia targets. Sorry for the noise on this, as I thought this was happening in the normal Linux part of the toolchain build. If we find that there will be some changes required on the libcxx side we'll let you know, but you can ignore my previous comments for now.

Chatted w/ @phosek offline and he thinks this may require some changes to the Fuchsia headers as the failure is happening in Fuchsia targets. Sorry for the noise on this, as I thought this was happening in the normal Linux part of the toolchain build. If we find that there will be some changes required on the libcxx side we'll let you know, but you can ignore my previous comments for now.

This should be addressed by D153384.

it also caused:
https://github.com/llvm/llvm-project/issues/63613

/usr/include/sys/statvfs.h:47:1: error: unknown type name '__BEGIN_DECLS'
   47 | __BEGIN_DECLS