Page MenuHomePhabricator

[libcxx] Filesystem TS -- Complete
ClosedPublic

Authored by EricWF on Feb 6 2016, 12:45 PM.

Details

Summary

This review contains the complete filesystem TS implementation.

NOTE: DO NOT APPLY THIS PATCH FROM PHABRICATOR! IT WILL NOT WORK!

Please clone https://github.com/efcs/libcxx/tree/filesystem-ts instead.

The test suite has symlinks checked in that are needed for a large portion of the tests. Using arc patch or downloading the diff directly will not
create these symlinks.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Light review below, looks like great work.

I noticed the tests seem somewhat inconsistent about whether to
glue & to the type or use two spaces (i.e., & ). Which one is
preferred in this codebase? Or is it laissez-faire?

The rest of my comments are inline.

EricWF added a comment.Feb 6 2016, 3:11 PM

Light review below, looks like great work.

I noticed the tests seem somewhat inconsistent about whether to
glue & to the type or use two spaces (i.e., & ). Which one is
preferred in this codebase? Or is it laissez-faire?

I prefer it glued to the type *i think* but there is no set style currently. I'll try and make it more consistent as I see them.

The rest of my comments are inline.

Thanks! If I could bother you to put them on phab next time instead of email it really helps me track them.

EricWF updated this revision to Diff 47103.Feb 6 2016, 3:42 PM

Address @dexonsmith's review comments. Thanks Duncan!

EricWF updated this revision to Diff 57723.May 18 2016, 6:54 PM
EricWF retitled this revision from [libcxx] Filesystem TS Part 1 -- path to [libcxx] Filesystem TS -- Complete.
EricWF updated this object.

Update the patch to contain the complete filesystem TS.

There still some junk in this patch (test/CMakelists.txt for example) but I thought I would update this review to show the current implementation status.

EricWF updated this revision to Diff 57725.May 18 2016, 7:12 PM

Remove a bunch of duplicate tests that have since been rewritten.

For reference here's the current test coverage: http://efcs.ca/filesystem-coverage/

Looks like I've completely missed this patch somehow. Will try to find some time (or someone) to have a look at it from an embedded-systems / ARM point of view asap.

Great work!!!

Looks like I've completely missed this patch somehow. Will try to find some time (or someone) to have a look at it from an embedded-systems / ARM point of view asap.

Great work!!!

Thanks! Even just attempting to run the tests is greatly appreciated. Writing portable tests is next to impossible so there are going to be kinks to iron out for each platform.

EricWF updated this revision to Diff 58896.May 28 2016, 11:26 AM

Add more tests and improve temporary file removal at end of test-suite run.

EricWF updated this revision to Diff 58924.May 29 2016, 5:05 PM

Added the remaining tests.

I have tested it on:

  • Ubuntu 16.04
  • Ubuntu 14.04
  • FreeBSD 10.3
  • OS X 10.11
EricWF updated this revision to Diff 59007.May 30 2016, 5:03 PM

The build now works in -fno-exceptions mode and the test suite passes.

I'm seeing a few failures on my Fedora 20 system. Will go through these tomorrow.

I'm seeing a few failures on my Fedora 20 system. Will go through these tomorrow.

If you want to just send me the raw ouput I'll go over them tonight.

I'm seeing a few failures on my Fedora 20 system. Will go through these tomorrow.

If you want to just send me the raw ouput I'll go over them tonight.

This is from a default (without extra cmake options) build: https://dl.dropboxusercontent.com/u/12212624/Other/filesystem_libcxx_results_fedora20.log

Some of them look obvious. For the rest, let me know if I can get you more info, I can debug tomorrow evening.

/ Asiri

EricWF updated this revision to Diff 59011.May 30 2016, 6:26 PM

Add missing include in min_allocator.h.

Thanks. I fixed the missing include causing most of the failures. Please update me when you can with new results.

majnemer added inline comments.
include/experimental/filesystem
611

Do we typically put names in TODOs?

673

TODO?

src/experimental/directory_iterator.cpp
47 ↗(On Diff #59011)

readdir_r is deprecated, please use readdir.

100 ↗(On Diff #59011)

Why is this commented out?

src/experimental/operations.cpp
127–128 ↗(On Diff #59011)

What sort of logic is this trying to determine?

354–355 ↗(On Diff #59011)

Any reason why mkdir isn't wrapped like the other fs calls?

381 ↗(On Diff #59011)

Hmm, why compare against zero instead of -1?

403 ↗(On Diff #59011)

Hmm, SUSv4 says:

The value returned for the variable {PATH_MAX} indicates the longest relative pathname that could be given if the specified directory is the current working directory of the process.

However, you want an absolute path relative to the root directory right?

506–532 ↗(On Diff #59011)

I'd suggest you swap these two blocks around. This way we don't need to add more conjunctions if we add more special cases.

528 ↗(On Diff #59011)

Isn't utime deprecated? I'd suggest using utimes.

659 ↗(On Diff #59011)

Formatting?

663 ↗(On Diff #59011)

Why not use uintmax_t instead of decltype(si.free) ?

667–669 ↗(On Diff #59011)

Does the standard give any guidance if this calculation overflows?

687 ↗(On Diff #59011)

Why all these environment variables?

705–707 ↗(On Diff #59011)

Formatting?

EricWF marked 6 inline comments as done.May 30 2016, 10:51 PM

@majnemer Thanks for looking at this!

include/experimental/filesystem
611

IDK. I normally do?

673

I'll clarify that. I have no idea how to implement the locale conversions. I was going to wait until most of filesystem landed before worrying about them.

src/experimental/directory_iterator.cpp
100 ↗(On Diff #59011)

It should be removed. After the stream is closed the __entry_ member is never referenced again.

src/experimental/operations.cpp
127–128 ↗(On Diff #59011)
354–355 ↗(On Diff #59011)

There are plenty of other fs calls that aren't wrapped. The wrapped ones are because they are used in a bunch of places.

381 ↗(On Diff #59011)

Probably an oversight.

403 ↗(On Diff #59011)

Yes, but that language confuses me. If I'm not mistaken the longest relative pathname for the CWD is going to be longer than the absolute one, since it's easy to take an absolute path and add "./././././././" and still refer to the same directory.

FYI this code also appears in tho opergroup spec for getcwd.
http://pubs.opengroup.org/onlinepubs/009695399/functions/getcwd.html

506–532 ↗(On Diff #59011)

Ack.

528 ↗(On Diff #59011)

If I'm not mistaken utimes is legacy, not utime. Also note that utimensat is used on all platforms except for apple, for which this is the fallback implementation.

659 ↗(On Diff #59011)

Not ever sure what I was saying there. Removing it.

663 ↗(On Diff #59011)

Not sure what I was thinking. Changing it.

667–669 ↗(On Diff #59011)

It says:

Any members for which the value cannot be determined shall be set to static_cast<uintmax_t>(-1).

I overlooked overflow checking. I'll add it.

No idea how I'll test it though.

687 ↗(On Diff #59011)
EricWF updated this revision to Diff 59013.May 30 2016, 10:55 PM
EricWF marked 5 inline comments as done.

Address @majnemers review comments:

  • Use readdir instead of readdir_r.
  • Remove or clean up poor comments.
  • reshuffle last_write_time implementation
  • detect overflows in space calls.

I'm seeing a few failures on my Fedora 20 system. Will go through these tomorrow.

If you want to just send me the raw ouput I'll go over them tonight.

This is from a default (without extra cmake options) build: https://dl.dropboxusercontent.com/u/12212624/Other/filesystem_libcxx_results_fedora20.log

Some of them look obvious. For the rest, let me know if I can get you more info, I can debug tomorrow evening.

I installed a Fedora 23 system and ran the test suite and I didn't see any of the same failures. It must be something other than the distro.
Are you using an odd filesystem? Or maybe it could be your system locale?

I installed a Fedora 23 system and ran the test suite and I didn't see any of the same failures. It must be something other than the distro.
Are you using an odd filesystem? Or maybe it could be your system locale?

I suspect so. Ran the latest patch just now and I'm still seeing the remaining failures.

Btw, when do you plan to land this? I'm a bit tight as we are on the run-up to a release. I will investigate my failures offline, but I don't think it should be blocking you (given that you've got them working on fedora 23). Mind you fedora 20 is a bit old.

/ Asiri

I installed a Fedora 23 system and ran the test suite and I didn't see any of the same failures. It must be something other than the distro.
Are you using an odd filesystem? Or maybe it could be your system locale?

I suspect so. Ran the latest patch just now and I'm still seeing the remaining failures.

Btw, when do you plan to land this? I'm a bit tight as we are on the run-up to a release. I will investigate my failures offline, but I don't think it should be blocking you (given that you've got them working on fedora 23). Mind you fedora 20 is a bit old.

/ Asiri

I don't have an odd filesystem btw, I've let fedora manage the partitioning etc. (LVM stuff - I don't know much). Other than that, I have an SSD (but don't think it can affect these tests).

/ Asiri

I installed a Fedora 23 system and ran the test suite and I didn't see any of the same failures. It must be something other than the distro.
Are you using an odd filesystem? Or maybe it could be your system locale?

I suspect so. Ran the latest patch just now and I'm still seeing the remaining failures.

Btw, when do you plan to land this? I'm a bit tight as we are on the run-up to a release. I will investigate my failures offline, but I don't think it should be blocking you (given that you've got them working on fedora 23). Mind you fedora 20 is a bit old.

/ Asiri

I have no immediate plans to land it. I'm going to try and get some eyeballs on it and some other people to try it first. If it lands and starts causing large amounts of failures it's going to be a headache and likely get reverted.

Would you / community be open to the idea of hiding the os syscalls behind an API? (like we are doing for pthreads)?

I think this is the only way we could get at least some of this functionality working on bare-metal ARM (it should work on arm-linux without much trouble - I think). How much work do you think this refactoring would need? I'm happy to do that after you land the patch.

I will see if I can try this out on arm-linux, as soon as I resolve this fedora20 mystery of mine :)

Cheers,

/ Asiri

Ah I figured it out! The test suite has symlinks within it. Git handles them properly but they obviously don't survive the round trip to phabricator and back. So all of the tests that depend on them fail (with is evidently a lot).

Ah I figured it out! The test suite has symlinks within it. Git handles them properly but they obviously don't survive the round trip to phabricator and back. So all of the tests that depend on them fail (with is evidently a lot).

Perhaps you can put up a tarball somewhere?

/ Asiri

Would you / community be open to the idea of hiding the os syscalls behind an API? (like we are doing for pthreads)?

Yes I would be very open to that. Then I could also have test shims in order to test truly exceptional cases.

I think this is the only way we could get at least some of this functionality working on bare-metal ARM (it should work on arm-linux without much trouble - I think). How much work do you think this refactoring would need? I'm happy to do that after you land the patch.

It shouldn't be that much work to get path, directory_iterator and recursive_directory_iterator working. They have like 4 system calls in there implementation.

The rest of the TS is just a set of free functions. Each uses a separate system call for the most part. However the API is very simple and you can pick and choose which functions you want to support.

I will see if I can try this out on arm-linux, as soon as I resolve this fedora20 mystery of mine :)

Cheers,

/ Asiri

The git branch I develop on is public: https://github.com/efcs/libcxx/tree/filesystem-ts

You can download or clone from there (and it will always be up to date!).

EricWF updated this object.May 31 2016, 12:14 AM

The git branch I develop on is public: https://github.com/efcs/libcxx/tree/filesystem-ts

You can download or clone from there (and it will always be up to date!).

All tests pass! :-)

I'll try to find an arm-linux setup soon and run this there as well.

/ Asiri

majnemer added inline comments.May 31 2016, 8:34 AM
src/experimental/operations.cpp
529 ↗(On Diff #59013)

SUSv4 says:

The utime() function is marked obsolescent.

However, utimes was made legacy in SUSv3 and removed from legacy in SUSv4
http://pubs.opengroup.org/onlinepubs/9699919799/functions/utimes.html

majnemer added inline comments.May 31 2016, 8:48 AM
src/experimental/operations.cpp
128–129 ↗(On Diff #59013)

It is possible for st_ino to wrap around while the machine is still running.
I'd mix st_gen into the comparison if we are running under one of the BSDs or Darwin.

Linux has a FS_IOC_GETVERSION ioctl but it requires a file descriptor. Maybe such heroics are not worth it.

ahatanak added inline comments.
test/std/utilities/meta/meta.rel/is_nothrow_callable.pass.cpp
57 ↗(On Diff #59013)

Can you explain why this change was needed? I'm trying to figure out why the two static_asserts in the #else part fail.

EricWF marked an inline comment as done.Jun 1 2016, 1:41 PM
EricWF added inline comments.
src/experimental/operations.cpp
128–129 ↗(On Diff #59013)

It is possible for st_ino to wrap around while the machine is still running.

Can you clarify this point? Maybe some docs I could read.

I'd mix st_gen into the comparison if we are running under one of the BSDs or Darwin.

According to the darwin docs st_gen is only available as a super user and even then my tests show it's always zero.
https://developer.apple.com/library/ios/documentation/System/Conceptual/ManPages_iPhoneOS/man2/stat.2.html

How does mixing that in help if st_ino wraps?

test/std/utilities/meta/meta.rel/is_nothrow_callable.pass.cpp
57 ↗(On Diff #59013)

Woops that sneaked in. I was investigating some GCC behavior unrelated to filesystem but forgot to revert the changes.

EricWF updated this revision to Diff 59271.Jun 1 2016, 1:42 PM
EricWF marked an inline comment as done.

Address review comments:

  • Make last_write_time(path, time) use utimes instead of utime on OS X.
  • Remove unintended changes to is_nothrow_callabletest.

@mclow.lists Just a reminder that 3.9 branches for release on July 19th.

EricWF updated this revision to Diff 60616.Jun 13 2016, 2:59 PM
  • Mark the filesystem tests as UNSUPPORTED when libc++experimental.a is not present. This uses lit.local.cfg files as opposed to //UNSUPPORTED: comments in every file.
  • Add missing trailing newlines in the tests.

Unless there are any objections I plan to commit this change on Friday.

That will give it about a month of in-tree time before the 3.9 release branch.

Unless there are any objections I plan to commit this change on Friday.

That will give it about a month of in-tree time before the 3.9 release branch.

Fine by me.

I didn't have much time to go through the patch, is there a way to disable this module from building? It won't compile for our targets until we put in some sort of a porting layer (which I need to start thinking of). Would be nice if there is some CMake switch to turn this off until such time.

Thanks!

/ Asiri

Unless there are any objections I plan to commit this change on Friday.

That will give it about a month of in-tree time before the 3.9 release branch.

Fine by me.

I didn't have much time to go through the patch, is there a way to disable this module from building? It won't compile for our targets until we put in some sort of a porting layer (which I need to start thinking of). Would be nice if there is some CMake switch to turn this off until such time.

Currently you can switch it off using the CMake option -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF. This turns off all of the experimental libraries, including filesystem and polymorphic memory resources.

Would this be sufficient for your use case? Otherwise I can create an option to specifically disable filesystem.

Fine by me.

I didn't have much time to go through the patch, is there a way to disable this module from building? It won't compile for our targets until we put in some sort of a porting layer (which I need to start thinking of). Would be nice if there is some CMake switch to turn this off until such time.

Currently you can switch it off using the CMake option -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF. This turns off all of the experimental libraries, including filesystem and polymorphic memory resources.

Would this be sufficient for your use case? Otherwise I can create an option to specifically disable filesystem.

We can currently build the rest of the experimental library (polymorphic memory resource), so it would be great if we can selectively disable just the filesystem module.

Thanks.

(sorry for the late response - went to bed)

/ Asiri

EricWF updated this revision to Diff 60653.Jun 13 2016, 11:59 PM

Add LIBCXX_ENABLE_FILESYSTEM CMake option to turn off only the filesystem parts of libc++experimental.a.

Add LIBCXX_ENABLE_FILESYSTEM CMake option to turn off only the filesystem parts of libc++experimental.a.

Thanks!

EricWF updated this revision to Diff 61122.Jun 17 2016, 12:39 PM

Final cleanup before commit. Filesystem incoming!

EricWF accepted this revision.Jun 17 2016, 12:40 PM
EricWF added a reviewer: EricWF.

Accepting before committing. Speak now or forever hold your peace. (Or just speak post commit)

This revision is now accepted and ready to land.Jun 17 2016, 12:40 PM
EricWF closed this revision.Jun 17 2016, 12:57 PM

Committed as r273034.