| Summary: | std::filesystem::create_directories doesn't set error code or throw while violating postcondition. | ||
|---|---|---|---|
| Product: | gcc | Reporter: | Steffen Schuemann <ssh> |
| Component: | libstdc++ | Assignee: | Jonathan Wakely <redi> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | webrown.cpp |
| Priority: | P3 | ||
| Version: | 8.0 | ||
| Target Milestone: | 8.3 | ||
| Host: | Target: | ||
| Build: | Known to work: | ||
| Known to fail: | Last reconfirmed: | 2018-08-10 00:00:00 | |
Please provide the output of 'gcc -v' as requested by https://gcc.gnu.org/bugs/ (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. 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! 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). (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. 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! (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++. 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. 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 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 Fixed for GCC 8.3 |
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.