Bug 86910 - std::filesystem::create_directories doesn't set error code or throw while violating postcondition.
Summary: std::filesystem::create_directories doesn't set error code or throw while vio...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 8.0
: P3 normal
Target Milestone: 8.3
Assignee: Jonathan Wakely
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-10 11:06 UTC by Steffen Schuemann
Modified: 2019-02-08 12:29 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2018-08-10 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Steffen Schuemann 2018-08-10 11:06:43 UTC
std::filesystem::create_directories should create all directories that don't exists in the given path. It is not an error if some of the directories exist. But they must be directories to fulfil the postcondition. 

The current implementation doens't signal an error if it didn't create the directory because a file existed with the same name, so the given postcondition is_directory(p) is violated but no error occurs.

//create_directories.cpp
#include <filesystem>
#include <fstream>
#include <iostream>
#include <system_error>
namespace fs = std::filesystem;
int main()
{
    fs::path p("testxyz");
    if(!fs::exists(p)) {
        std::ofstream file(p);
        file.close();
    }
    std::error_code ec;
    if(fs::create_directories(p, ec))
        std::cout << "created" << std::endl;
    else
        std::cout << "didn't create" << std::endl;
    if(!fs::is_directory(p))
        std::cerr << "postcondition failed!" << std::endl;
    std::cerr << "error code " << (ec?"set":"not set") << std::endl;
    fs::remove(p);
}

Compiled on Ubuntu 18.04 with

g++-8 -std=c++17 -o create_dirs create_directories.cpp -lstdc++fs

The expected output would be:

didn't create
postcondition failed!
error code set

but current code gives:

didn't create
postcondition failed!
error code not set

The interface without ec should throw, but doesn't.
Comment 1 Jonathan Wakely 2018-08-10 11:15:24 UTC
Please provide the output of 'gcc -v' as requested by https://gcc.gnu.org/bugs/
Comment 2 Jonathan Wakely 2018-08-10 11:17:48 UTC
(In reply to Steffen Schuemann from comment #0)
> std::filesystem::create_directories should create all directories that don't
> exists in the given path. It is not an error if some of the directories
> exist. But they must be directories to fulfil the postcondition. 
> 
> The current implementation doens't signal an error if it didn't create the
> directory because a file existed with the same name, so the given
> postcondition is_directory(p) is violated but no error occurs.

That is the correct behaviour:
https://cplusplus.github.io/LWG/lwg-defects.html#2935

I don't think this is a bug.
Comment 3 Steffen Schuemann 2018-08-11 05:41:21 UTC
Sorry, g++-8 -v:

Using built-in specs.
COLLECT_GCC=g++-8
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/8/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 8-20180414-1ubuntu2' --with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs --enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ --prefix=/usr --with-gcc-major-version-only --with-as=/usr/bin/x86_64-linux-gnu-as --with-ld=/usr/bin/x86_64-linux-gnu-ld --program-suffix=-8 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-libmpx --enable-plugin --enable-default-pie --with-system-zlib --with-target-system-zlib --enable-objc-gc=auto --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 8.0.1 20180414 (experimental) [trunk revision 259383] (Ubuntu 8-20180414-1ubuntu2) 

I see, that LWG defect 2935 addresses this question. But first, my understanding is a defect in WP status is still not part of the standard, so it might become part of some C++20 or some TC, but it currently doesn't seem to be part of C++17 (selected by -std=c++17). I might be wrong with that.

And second, I still I don't really see how this is expected to be used by the developers usinf std::fs. Without that postcondition you have to do a check for the directory before using that it, so the "spared" call was simply moved to the user code. Not testing could lead to harder to track down bugs later on in a different domain of the software. 

And third, as testing for existence and type can be done with one call, I don't see the issue raised in the defect about additional system calls in the implementation on Windows and POSIX. I found this issue while implementing a closely C++17 compatible filesystem for C++11/14 level compilers on Windows and POSIX and simply ran my (still growing) test suite against GCCs std::fs where that case failed, so I thought I just report it. ;-)

I didn't need any extra system calls to detect that situation in my (wip) implementation. So I just looked into https://github.com/gcc-mirror/gcc/blob/a2809afdf9560accb06cdb595ce20e32f652a75c/libstdc%2B%2B-v3/src/filesystem/std-ops.cc#L652-L665 and when the fs::file_status result of the fs::status(pp, ec) call tracking the file_type::not_found elements would have been keept, the fs::is_directory(s)-call on that last file_status would have been free of any additional system calls.

Well, this might not be the place to discuss the LWG decision, it just doesn't look like a helpfull one to me, and it feels like beeing taken based on a false assumtion about the implementation, so it hurts and it converts a supposed system call actually not needed to a definitive system call on the user side.

I guess I stick to the current C++17 descritpion and keep the error reporting, as it seems the way to go for me, and it's still good to know I need to take care on other implementations, but as I would use the compiler given std::fs whereever I can (I just don't allways have it), I sure would prefer it to follow the original wording.

Thank you for pointing me on that LWG defect!
Comment 4 Steffen Schuemann 2018-08-11 08:26:18 UTC
I just realised, if I'm all wrong, and not handling this as an error is the way to go, as described in LWG-defect-#2935, create_directory() would violate the new behaviour described in 2935, as it passes my unit tests so it throws an exception if a file with that name exists. One way or the other, something doesn't match (also verified on g++ 8.1.0 on macOS and by looking into the code).
Comment 5 Jonathan Wakely 2018-08-11 20:33:37 UTC
(In reply to Steffen Schuemann from comment #3)
> gcc version 8.0.1 20180414 (experimental) [trunk revision 259383] (Ubuntu
> 8-20180414-1ubuntu2) 

Why are you using a pre-release snapshot from 4 months ago when the 8.1 and 8.2 releases have been made since then?

> I see, that LWG defect 2935 addresses this question. But first, my
> understanding is a defect in WP status is still not part of the standard, so
> it might become part of some C++20 or some TC, but it currently doesn't seem
> to be part of C++17 (selected by -std=c++17). I might be wrong with that.

It's a defect in C++17. We implement the fixes, not implement a standard that is known to be defective. Why would you want a pedantic adherence to a defective standard? Do you also want compiler bugs to go unfixed, because they were present in past releases? :-)

Anyway, the manual says what -std=c++17 means:

"The 2017 ISO C++ standard plus amendments."

https://gcc.gnu.org/onlinedocs/gcc/C-Dialect-Options.html
 
> And second, I still I don't really see how this is expected to be used by
> the developers usinf std::fs. Without that postcondition you have to do a
> check for the directory before using that it, so the "spared" call was
> simply moved to the user code. Not testing could lead to harder to track
> down bugs later on in a different domain of the software. 

Yes, I agree. I don't think pushing the is_directory check onto users is helpful, but that's what the Defect Report resolution says. I was discussing it recently with the developer of libc++'s std::filesystem implementation and I think we both fail to meet the spec for create_directory (as you noted in comment 4). I'm not sure what libc++ does for create_directories.

I haven't changed create_directory, because I like its current behaviour. I haven't changed create_directories because it was accidentally already doing what 2935 wants, and intentionally changing to disagree with the spec seems wrong. i.e. inertia has won.

> And third, as testing for existence and type can be done with one call,

That's true for my implementation of create_directories, but not for create_directory, where you don't need to test for existence. You call mkdir and if it returns EEXIST you know it already exists. To see if it's a directory requires another system call (which I currently do).

I've now asked the standard committee to reconsider whether we made the right call on 2935.

I'll confirm this, because whatever happens create_directory and create_directories should be consistent, so I'll need to change one of them.
Comment 6 Steffen Schuemann 2018-08-12 15:02:28 UTC
Yeah, I totally understand, that if the behaviour of create_directories was in conformance to the resolution to that defect, you don't feel like changing anything. And I definitely don't want to be pedantic. In the end, if this is, what is wanted, I have to accept the solution. Well, I might even feel pressed to adapt the behaviour of my own implementation to that. I guess, if I had seen the defect report in advance, I might not even have bothered you with this, even if I would have felt bad about that decision. But I'm really grateful, that you did forward that question, thank you for trying.

The GCC version I was using was simply the GCC8 that my Ubuntu 18.04 virtual machine had available. I didn't want to build my own GCC on that vm as I have some disk space issues. Before filing the issue, I double checked against the head of the source, to make sure it is nothing changed in a later version. I additionally tested against the g++ on my macOS that is the 8.1.0 from homebrew.

And just for the fun of it, I used some time to double check the source of libc++ and compiled myself a clang-8 from HEAD to run my tests with that one too, and it is just the other way around: create_directories fails/throws when a file with that name exists, and create_directory doesn't. :-)
I guess there's some inconsistency too, but this is the wrong issue tracker for that.

In the end, I vote for consistency, either the more useful way with the postcondition, or the fixed way without. Whichever docs you use when calling them, they should behave consistently.

Thanks again, for the help and work!
Comment 7 Jonathan Wakely 2018-08-12 22:35:15 UTC
(In reply to Steffen Schuemann from comment #6)
> And just for the fun of it, I used some time to double check the source of
> libc++ and compiled myself a clang-8 from HEAD to run my tests with that one
> too, and it is just the other way around: create_directories fails/throws
> when a file with that name exists, and create_directory doesn't. :-)

Ha!

> In the end, I vote for consistency, either the more useful way with the
> postcondition, or the fixed way without. Whichever docs you use when calling
> them, they should behave consistently.

Yes, I definitely agree.

Once the committee finishes discussing it and decides to either revisit the DR, or reconfirm it's definitely what we want, then I'll change libstdc++.
Comment 8 Jonathan Wakely 2018-11-28 16:07:51 UTC
The proposed new direction is:

In case mkdir() fails because p already exists, if p resolves to an existing directory, no error is reported, otherwise an error is reported.

And consistently for create_directory and create_directories, of course.
Comment 9 Jonathan Wakely 2018-11-29 00:40:09 UTC
Author: redi
Date: Thu Nov 29 00:39:37 2018
New Revision: 266598

URL: https://gcc.gnu.org/viewcvs?rev=266598&root=gcc&view=rev
Log:
PR libstdc++/86910 fix filesystem::create_directories

Implement the proposed semantics from P1164R0, which reverts the changes
of LWG 2935. This means that failure to create a directory because a
non-directory already exists with that name will be reported as an
error.

While rewriting the function, also fix PR 87846, which is a result of
the C++17 changes to how a trailing slash on a path affects the last
component of a path.

	PR libstdc++/86910
	PR libstdc++/87846
	* src/filesystem/ops.cc (experimental::create_directories): Report
	an error when the path resolves to an existing non-directory (P1164).
	* src/filesystem/std-ops.cc (create_directories): Likewise. Handle
	empty filenames due to trailing slashes.
	* testsuite/27_io/filesystem/operations/create_directories.cc: Test
	when some component of the path exists and is not a directory. Test
	trailing slashes.
	* testsuite/experimental/filesystem/operations/create_directories.cc:
	Likewise.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/src/filesystem/ops.cc
    trunk/libstdc++-v3/src/filesystem/std-ops.cc
    trunk/libstdc++-v3/testsuite/27_io/filesystem/operations/create_directories.cc
    trunk/libstdc++-v3/testsuite/experimental/filesystem/operations/create_directories.cc
Comment 10 Jonathan Wakely 2019-02-08 12:20:54 UTC
Author: redi
Date: Fri Feb  8 12:20:22 2019
New Revision: 268685

URL: https://gcc.gnu.org/viewcvs?rev=268685&root=gcc&view=rev
Log:
PR libstdc++/86910 fix filesystem::create_directories

Implement the proposed semantics from P1164R0, which reverts the changes
of LWG 2935. This means that failure to create a directory because a
non-directory already exists with that name will be reported as an
error.

While rewriting the function, also fix PR 87846, which is a result of
the C++17 changes to how a trailing slash on a path affects the last
component of a path.

Backport from mainline
2018-11-29  Jonathan Wakely  <jwakely@redhat.com>

	PR libstdc++/86910
	PR libstdc++/87846
	* src/filesystem/ops.cc (experimental::create_directories): Report
	an error when the path resolves to an existing non-directory (P1164).
	* src/filesystem/std-ops.cc (create_directories): Likewise. Handle
	empty filenames due to trailing slashes.
	* testsuite/27_io/filesystem/operations/create_directories.cc: Test
	when some component of the path exists and is not a directory. Test
	trailing slashes.
	* testsuite/experimental/filesystem/operations/create_directories.cc:
	Likewise.

Modified:
    branches/gcc-8-branch/libstdc++-v3/ChangeLog
    branches/gcc-8-branch/libstdc++-v3/src/filesystem/ops.cc
    branches/gcc-8-branch/libstdc++-v3/src/filesystem/std-ops.cc
    branches/gcc-8-branch/libstdc++-v3/testsuite/27_io/filesystem/operations/create_directories.cc
    branches/gcc-8-branch/libstdc++-v3/testsuite/experimental/filesystem/operations/create_directories.cc
Comment 11 Jonathan Wakely 2019-02-08 12:29:00 UTC
Fixed for GCC 8.3