| Message ID | or1qvh9p3u.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++-v3: check for openat | expand |
On Wed, 22 Jun 2022 at 07:43, Alexandre Oliva via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > > rtems6.0 has fdopendir, and fcntl.h defines AT_FDCWD and declares > openat, but there's no openat in libc. Adjust dir-common.h to not > assume ::openat just because of AT_FDCWD. > > Regstrapped on x86_64-linux-gnu (detects and still uses openat), also > tested with a cross to aarch64-rtems6 (detects openat's absence and > refrains from using it). Ok to install? > > PS: This is the last patch in my rtems6.0 patchset, and the only patch > for the filesystem-related patchset that was written specifically for a > mainline gcc. gcc-11 did not attempt to use openat. This patch enabled > filesystem tests to link when testing mainline on aarch64-rtems6.0. > Alas, several filesystem tests still failed with it, in ways that AFAICT > are not related with the use of openat, or with the other patches I've > posted. However, I'm not able to look into the remaining failures right > now. There are other interactions between AT_CDCWD and ::openat not covered by this patch. I this this also needs to check HAVE_OPENAT: // Return a file descriptor for the directory and current entry's path. // If dirfd is available, use it and return only the filename. // Otherwise, return AT_FDCWD and return the full path. pair<int, const posix::char_type*> dir_and_pathname() const noexcept { const fs::path& p = entry.path(); #if _GLIBCXX_HAVE_DIRFD if (!p.empty()) return {::dirfd(this->dirp), std::prev(p.end())->c_str()}; #endif return {this->fdcwd(), p.c_str()}; } Otherwise, if rterms defines HAVE_DIRFD this function will return a file descriptor and a filename (not a full path) but then _Dir_base::openat doesn't use ::openat and so ignores the file descriptor, and needs a full path. Or maybe instead of changing dir_and_pathname() (because that's used with both openat and unlinkat) we should change _Dir::open_subdir like so: --- a/libstdc++-v3/src/c++17/fs_dir.cc +++ b/libstdc++-v3/src/c++17/fs_dir.cc @@ -136,7 +136,12 @@ struct fs::_Dir : _Dir_base open_subdir(bool skip_permission_denied, bool nofollow, error_code& ec) const noexcept { +#if _GLIBCXX_HAVE_OPENAT auto [dirfd, pathname] = dir_and_pathname(); +#else + int dirfd = -1; + const char* pathname = entry.path().c_str(); +#endif _Dir_base d(dirfd, pathname, skip_permission_denied, nofollow, ec); // If this->path is empty, the new _Dir should have an empty path too. const fs::path& p = this->path.empty() ? this->path : this->entry.path(); This will ensure we pass a full path to _Dir_base::openat when it needs one. I'm a bit sad that this breaks the abstraction, so that the derived class needs to know how the base class is implemented, but that coupling is actually already implicitly present (which is what you're trying to fix). Maybe a cleaner solution is for the _Dir_base to take both a filename *and* a full pathname, and then decide which to use in _Dir_base::openat. So the derived class would provide both, and not care how the base class chooses to use them. > > > for libstdc++-v3/ChangeLog > > * acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check for > openat. > * aclocal.m4, configure, config.h.in: Rebuilt. > * src/filesystem/dir-common.h (openat): Use ::openat if > _GLIBCXX_HAVE_OPENAT. > --- > libstdc++-v3/acinclude.m4 | 12 +++++++ > libstdc++-v3/config.h.in | 3 ++ > libstdc++-v3/configure | 55 ++++++++++++++++++++++++++++++ > libstdc++-v3/src/filesystem/dir-common.h | 2 + > 4 files changed, 71 insertions(+), 1 deletion(-) > > diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4 > index 138bd58d86cb9..e3cc3a8e867d3 100644 > --- a/libstdc++-v3/acinclude.m4 > +++ b/libstdc++-v3/acinclude.m4 > @@ -4772,6 +4772,18 @@ dnl > if test $glibcxx_cv_dirfd = yes; then > AC_DEFINE(HAVE_DIRFD, 1, [Define if dirfd is available in <dirent.h>.]) > fi > +dnl > + AC_CACHE_CHECK([for openat], > + glibcxx_cv_openat, [dnl > + GCC_TRY_COMPILE_OR_LINK( > + [#include <fcntl.h>], > + [int fd = ::openat(AT_FDCWD, "", 0);], > + [glibcxx_cv_openat=yes], > + [glibcxx_cv_openat=no]) > + ]) > + if test $glibcxx_cv_openat = yes; then > + AC_DEFINE(HAVE_OPENAT, 1, [Define if openat is available in <fcntl.h>.]) > + fi > dnl > AC_CACHE_CHECK([for unlinkat], > glibcxx_cv_unlinkat, [dnl > diff --git a/libstdc++-v3/config.h.in b/libstdc++-v3/config.h.in > index f30a8c51c458c..2a3972eef5412 100644 > --- a/libstdc++-v3/config.h.in > +++ b/libstdc++-v3/config.h.in > @@ -292,6 +292,9 @@ > /* Define if <math.h> defines obsolete isnan function. */ > #undef HAVE_OBSOLETE_ISNAN > > +/* Define if openat is available in <fcntl.h>. */ > +#undef HAVE_OPENAT > + > /* Define if poll is available in <poll.h>. */ > #undef HAVE_POLL > > diff --git a/libstdc++-v3/configure b/libstdc++-v3/configure > index 9b94fd71e4248..eac6039212168 100755 > --- a/libstdc++-v3/configure > +++ b/libstdc++-v3/configure > @@ -77177,6 +77177,61 @@ $as_echo "$glibcxx_cv_dirfd" >&6; } > > $as_echo "#define HAVE_DIRFD 1" >>confdefs.h > > + fi > + { $as_echo "$as_me:${as_lineno-$LINENO}: checking for openat" >&5 > +$as_echo_n "checking for openat... " >&6; } > +if ${glibcxx_cv_openat+:} false; then : > + $as_echo_n "(cached) " >&6 > +else > + if test x$gcc_no_link = xyes; then > + cat confdefs.h - <<_ACEOF >conftest.$ac_ext > +/* end confdefs.h. */ > +#include <fcntl.h> > +int > +main () > +{ > +int fd = ::openat(AT_FDCWD, "", 0); > + ; > + return 0; > +} > +_ACEOF > +if ac_fn_cxx_try_compile "$LINENO"; then : > + glibcxx_cv_openat=yes > +else > + glibcxx_cv_openat=no > +fi > +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext > +else > + if test x$gcc_no_link = xyes; then > + as_fn_error $? "Link tests are not allowed after GCC_NO_EXECUTABLES." "$LINENO" 5 > +fi > +cat confdefs.h - <<_ACEOF >conftest.$ac_ext > +/* end confdefs.h. */ > +#include <fcntl.h> > +int > +main () > +{ > +int fd = ::openat(AT_FDCWD, "", 0); > + ; > + return 0; > +} > +_ACEOF > +if ac_fn_cxx_try_link "$LINENO"; then : > + glibcxx_cv_openat=yes > +else > + glibcxx_cv_openat=no > +fi > +rm -f core conftest.err conftest.$ac_objext \ > + conftest$ac_exeext conftest.$ac_ext > +fi > + > +fi > +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $glibcxx_cv_openat" >&5 > +$as_echo "$glibcxx_cv_openat" >&6; } > + if test $glibcxx_cv_openat = yes; then > + > +$as_echo "#define HAVE_OPENAT 1" >>confdefs.h > + > fi > { $as_echo "$as_me:${as_lineno-$LINENO}: checking for unlinkat" >&5 > $as_echo_n "checking for unlinkat... " >&6; } > diff --git a/libstdc++-v3/src/filesystem/dir-common.h b/libstdc++-v3/src/filesystem/dir-common.h > index 365fd527f4d68..669780ea23fe5 100644 > --- a/libstdc++-v3/src/filesystem/dir-common.h > +++ b/libstdc++-v3/src/filesystem/dir-common.h > @@ -199,7 +199,7 @@ struct _Dir_base > #endif > > > -#ifdef AT_FDCWD > +#if _GLIBCXX_HAVE_OPENAT && defined AT_FDCWD > fd = ::openat(fd, pathname, flags); > #else > // If we cannot use openat, there's no benefit to using posix::open unless > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Disinformation flourishes because many people care deeply about injustice > but very few check the facts. Ask me about <https://stallmansupport.org> >
On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > Otherwise, if rterms defines HAVE_DIRFD this function will return a > file descriptor and a filename (not a full path) but then > _Dir_base::openat doesn't use ::openat and so ignores the file > descriptor, and needs a full path. Yuck. It does. This may explain some of the fails I've observed and not looked into yet. How about introducing an internal wrapper class that can hold a ref to base path or an fd for a dir, or an iterator that could resolve to either, and that can be passed around instead of a dirfd that assumes openat, enabling uses of openat where available and falling back to paths otherwise? I don't have much of a sense of all the uses involved, but given the AFAICT impossibility of turning a dirfd into a pathname (even in non-pathological cases of removed or out-of-chroot dirfds), ISTM this wrapper class could demand base paths or CWD in the !HAVE_OPENAT case, and enable both uses otherwise, offering some additional type safety that the current abstraction doesn't. Does that make sense to you?
On Thu, 23 Jun 2022 at 05:41, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > Otherwise, if rterms defines HAVE_DIRFD this function will return a > > file descriptor and a filename (not a full path) but then > > _Dir_base::openat doesn't use ::openat and so ignores the file > > descriptor, and needs a full path. > > Yuck. It does. This may explain some of the fails I've observed and > not looked into yet. Yes, that's what I was thinking too. > How about introducing an internal wrapper class that can hold a ref to > base path or an fd for a dir, or an iterator that could resolve to > either, and that can be passed around instead of a dirfd that assumes > openat, enabling uses of openat where available and falling back to > paths otherwise? I don't have much of a sense of all the uses involved, > but given the AFAICT impossibility of turning a dirfd into a pathname > (even in non-pathological cases of removed or out-of-chroot dirfds), > ISTM this wrapper class could demand base paths or CWD in the > !HAVE_OPENAT case, and enable both uses otherwise, offering some > additional type safety that the current abstraction doesn't. > > Does that make sense to you? I don't think that quite works. I think what we want is more like: struct multipath /* need better name */ { int fd; const char* filename; const char* pathname; }; I already prototyped something like this, let me finish it off and send the patch.
On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > There are other interactions between AT_CDCWD and ::openat not covered > by this patch. I this this also needs to check HAVE_OPENAT: Here's an updated version, tested with this additional change. libstdc++: check for openat From: Alexandre Oliva <oliva@adacore.com> rtems6.0 has fdopendir, and fcntl.h defines AT_FDCWD and declares openat, but there's no openat in libc. Adjust dir-common.h to not assume ::openat just because of AT_FDCWD. for libstdc++-v3/ChangeLog * acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check for openat. * aclocal.m4, configure, config.h.in: Rebuilt. * src/filesystem/dir-common.h (openat): Use ::openat if _GLIBCXX_HAVE_OPENAT. * src/filesystem/dir.cc (dir_and_pathname): Use dirfd if _GLIBCXX_HAVE_OPENAT. --- libstdc++-v3/acinclude.m4 | 12 +++++++ libstdc++-v3/config.h.in | 3 ++ libstdc++-v3/configure | 55 ++++++++++++++++++++++++++++++ libstdc++-v3/src/filesystem/dir-common.h | 2 + libstdc++-v3/src/filesystem/dir.cc | 2 + 5 files changed, 72 insertions(+), 2 deletions(-) diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4 index 138bd58d86cb9..e3cc3a8e867d3 100644 --- a/libstdc++-v3/acinclude.m4 +++ b/libstdc++-v3/acinclude.m4 @@ -4772,6 +4772,18 @@ dnl if test $glibcxx_cv_dirfd = yes; then AC_DEFINE(HAVE_DIRFD, 1, [Define if dirfd is available in <dirent.h>.]) fi +dnl + AC_CACHE_CHECK([for openat], + glibcxx_cv_openat, [dnl + GCC_TRY_COMPILE_OR_LINK( + [#include <fcntl.h>], + [int fd = ::openat(AT_FDCWD, "", 0);], + [glibcxx_cv_openat=yes], + [glibcxx_cv_openat=no]) + ]) + if test $glibcxx_cv_openat = yes; then + AC_DEFINE(HAVE_OPENAT, 1, [Define if openat is available in <fcntl.h>.]) + fi dnl AC_CACHE_CHECK([for unlinkat], glibcxx_cv_unlinkat, [dnl diff --git a/libstdc++-v3/config.h.in b/libstdc++-v3/config.h.in index f30a8c51c458c..2a3972eef5412 100644 --- a/libstdc++-v3/config.h.in +++ b/libstdc++-v3/config.h.in @@ -292,6 +292,9 @@ /* Define if <math.h> defines obsolete isnan function. */ #undef HAVE_OBSOLETE_ISNAN +/* Define if openat is available in <fcntl.h>. */ +#undef HAVE_OPENAT + /* Define if poll is available in <poll.h>. */ #undef HAVE_POLL diff --git a/libstdc++-v3/configure b/libstdc++-v3/configure index 9b94fd71e4248..eac6039212168 100755 --- a/libstdc++-v3/configure +++ b/libstdc++-v3/configure @@ -77177,6 +77177,61 @@ $as_echo "$glibcxx_cv_dirfd" >&6; } $as_echo "#define HAVE_DIRFD 1" >>confdefs.h + fi + { $as_echo "$as_me:${as_lineno-$LINENO}: checking for openat" >&5 +$as_echo_n "checking for openat... " >&6; } +if ${glibcxx_cv_openat+:} false; then : + $as_echo_n "(cached) " >&6 +else + if test x$gcc_no_link = xyes; then + cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include <fcntl.h> +int +main () +{ +int fd = ::openat(AT_FDCWD, "", 0); + ; + return 0; +} +_ACEOF +if ac_fn_cxx_try_compile "$LINENO"; then : + glibcxx_cv_openat=yes +else + glibcxx_cv_openat=no +fi +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext +else + if test x$gcc_no_link = xyes; then + as_fn_error $? "Link tests are not allowed after GCC_NO_EXECUTABLES." "$LINENO" 5 +fi +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include <fcntl.h> +int +main () +{ +int fd = ::openat(AT_FDCWD, "", 0); + ; + return 0; +} +_ACEOF +if ac_fn_cxx_try_link "$LINENO"; then : + glibcxx_cv_openat=yes +else + glibcxx_cv_openat=no +fi +rm -f core conftest.err conftest.$ac_objext \ + conftest$ac_exeext conftest.$ac_ext +fi + +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $glibcxx_cv_openat" >&5 +$as_echo "$glibcxx_cv_openat" >&6; } + if test $glibcxx_cv_openat = yes; then + +$as_echo "#define HAVE_OPENAT 1" >>confdefs.h + fi { $as_echo "$as_me:${as_lineno-$LINENO}: checking for unlinkat" >&5 $as_echo_n "checking for unlinkat... " >&6; } diff --git a/libstdc++-v3/src/filesystem/dir-common.h b/libstdc++-v3/src/filesystem/dir-common.h index 365fd527f4d68..669780ea23fe5 100644 --- a/libstdc++-v3/src/filesystem/dir-common.h +++ b/libstdc++-v3/src/filesystem/dir-common.h @@ -199,7 +199,7 @@ struct _Dir_base #endif -#ifdef AT_FDCWD +#if _GLIBCXX_HAVE_OPENAT && defined AT_FDCWD fd = ::openat(fd, pathname, flags); #else // If we cannot use openat, there's no benefit to using posix::open unless diff --git a/libstdc++-v3/src/filesystem/dir.cc b/libstdc++-v3/src/filesystem/dir.cc index b451902c4a1b1..e64489162e5ff 100644 --- a/libstdc++-v3/src/filesystem/dir.cc +++ b/libstdc++-v3/src/filesystem/dir.cc @@ -120,7 +120,7 @@ struct fs::_Dir : std::filesystem::_Dir_base dir_and_pathname() const noexcept { const fs::path& p = entry.path(); -#if _GLIBCXX_HAVE_DIRFD +#if _GLIBCXX_HAVE_DIRFD && _GLIBCXX_HAVE_OPENAT return {::dirfd(this->dirp), std::prev(p.end())->c_str()}; #endif return {this->fdcwd(), p.c_str()};
On Thu, 23 Jun 2022 at 12:08, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > There are other interactions between AT_CDCWD and ::openat not covered > > by this patch. I this this also needs to check HAVE_OPENAT: > > Here's an updated version, tested with this additional change. Did this improve your test results for directory iterators? In the unlikely even that the target has ::unlinkat but not ::openat this will cause a behaviour regression, because now _Dir::do_unlink will use {AT_FDCWD, "full/path/to/file"} instead of {dirfd(dirp),"file"} but I doubt there are targets that have unlinkat without openat. I'll try to improve this code later as discussed anyway, so this is fine for now. OK for trunk, thanks. > > > libstdc++: check for openat > > From: Alexandre Oliva <oliva@adacore.com> > > rtems6.0 has fdopendir, and fcntl.h defines AT_FDCWD and declares > openat, but there's no openat in libc. Adjust dir-common.h to not > assume ::openat just because of AT_FDCWD. > > > for libstdc++-v3/ChangeLog > > * acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Check for > openat. > * aclocal.m4, configure, config.h.in: Rebuilt. > * src/filesystem/dir-common.h (openat): Use ::openat if > _GLIBCXX_HAVE_OPENAT. > * src/filesystem/dir.cc (dir_and_pathname): Use dirfd if > _GLIBCXX_HAVE_OPENAT. > --- > libstdc++-v3/acinclude.m4 | 12 +++++++ > libstdc++-v3/config.h.in | 3 ++ > libstdc++-v3/configure | 55 ++++++++++++++++++++++++++++++ > libstdc++-v3/src/filesystem/dir-common.h | 2 + > libstdc++-v3/src/filesystem/dir.cc | 2 + > 5 files changed, 72 insertions(+), 2 deletions(-) > > diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4 > index 138bd58d86cb9..e3cc3a8e867d3 100644 > --- a/libstdc++-v3/acinclude.m4 > +++ b/libstdc++-v3/acinclude.m4 > @@ -4772,6 +4772,18 @@ dnl > if test $glibcxx_cv_dirfd = yes; then > AC_DEFINE(HAVE_DIRFD, 1, [Define if dirfd is available in <dirent.h>.]) > fi > +dnl > + AC_CACHE_CHECK([for openat], > + glibcxx_cv_openat, [dnl > + GCC_TRY_COMPILE_OR_LINK( > + [#include <fcntl.h>], > + [int fd = ::openat(AT_FDCWD, "", 0);], > + [glibcxx_cv_openat=yes], > + [glibcxx_cv_openat=no]) > + ]) > + if test $glibcxx_cv_openat = yes; then > + AC_DEFINE(HAVE_OPENAT, 1, [Define if openat is available in <fcntl.h>.]) > + fi > dnl > AC_CACHE_CHECK([for unlinkat], > glibcxx_cv_unlinkat, [dnl > diff --git a/libstdc++-v3/config.h.in b/libstdc++-v3/config.h.in > index f30a8c51c458c..2a3972eef5412 100644 > --- a/libstdc++-v3/config.h.in > +++ b/libstdc++-v3/config.h.in > @@ -292,6 +292,9 @@ > /* Define if <math.h> defines obsolete isnan function. */ > #undef HAVE_OBSOLETE_ISNAN > > +/* Define if openat is available in <fcntl.h>. */ > +#undef HAVE_OPENAT > + > /* Define if poll is available in <poll.h>. */ > #undef HAVE_POLL > > diff --git a/libstdc++-v3/configure b/libstdc++-v3/configure > index 9b94fd71e4248..eac6039212168 100755 > --- a/libstdc++-v3/configure > +++ b/libstdc++-v3/configure > @@ -77177,6 +77177,61 @@ $as_echo "$glibcxx_cv_dirfd" >&6; } > > $as_echo "#define HAVE_DIRFD 1" >>confdefs.h > > + fi > + { $as_echo "$as_me:${as_lineno-$LINENO}: checking for openat" >&5 > +$as_echo_n "checking for openat... " >&6; } > +if ${glibcxx_cv_openat+:} false; then : > + $as_echo_n "(cached) " >&6 > +else > + if test x$gcc_no_link = xyes; then > + cat confdefs.h - <<_ACEOF >conftest.$ac_ext > +/* end confdefs.h. */ > +#include <fcntl.h> > +int > +main () > +{ > +int fd = ::openat(AT_FDCWD, "", 0); > + ; > + return 0; > +} > +_ACEOF > +if ac_fn_cxx_try_compile "$LINENO"; then : > + glibcxx_cv_openat=yes > +else > + glibcxx_cv_openat=no > +fi > +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext > +else > + if test x$gcc_no_link = xyes; then > + as_fn_error $? "Link tests are not allowed after GCC_NO_EXECUTABLES." "$LINENO" 5 > +fi > +cat confdefs.h - <<_ACEOF >conftest.$ac_ext > +/* end confdefs.h. */ > +#include <fcntl.h> > +int > +main () > +{ > +int fd = ::openat(AT_FDCWD, "", 0); > + ; > + return 0; > +} > +_ACEOF > +if ac_fn_cxx_try_link "$LINENO"; then : > + glibcxx_cv_openat=yes > +else > + glibcxx_cv_openat=no > +fi > +rm -f core conftest.err conftest.$ac_objext \ > + conftest$ac_exeext conftest.$ac_ext > +fi > + > +fi > +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $glibcxx_cv_openat" >&5 > +$as_echo "$glibcxx_cv_openat" >&6; } > + if test $glibcxx_cv_openat = yes; then > + > +$as_echo "#define HAVE_OPENAT 1" >>confdefs.h > + > fi > { $as_echo "$as_me:${as_lineno-$LINENO}: checking for unlinkat" >&5 > $as_echo_n "checking for unlinkat... " >&6; } > diff --git a/libstdc++-v3/src/filesystem/dir-common.h b/libstdc++-v3/src/filesystem/dir-common.h > index 365fd527f4d68..669780ea23fe5 100644 > --- a/libstdc++-v3/src/filesystem/dir-common.h > +++ b/libstdc++-v3/src/filesystem/dir-common.h > @@ -199,7 +199,7 @@ struct _Dir_base > #endif > > > -#ifdef AT_FDCWD > +#if _GLIBCXX_HAVE_OPENAT && defined AT_FDCWD > fd = ::openat(fd, pathname, flags); > #else > // If we cannot use openat, there's no benefit to using posix::open unless > diff --git a/libstdc++-v3/src/filesystem/dir.cc b/libstdc++-v3/src/filesystem/dir.cc > index b451902c4a1b1..e64489162e5ff 100644 > --- a/libstdc++-v3/src/filesystem/dir.cc > +++ b/libstdc++-v3/src/filesystem/dir.cc > @@ -120,7 +120,7 @@ struct fs::_Dir : std::filesystem::_Dir_base > dir_and_pathname() const noexcept > { > const fs::path& p = entry.path(); > -#if _GLIBCXX_HAVE_DIRFD > +#if _GLIBCXX_HAVE_DIRFD && _GLIBCXX_HAVE_OPENAT > return {::dirfd(this->dirp), std::prev(p.end())->c_str()}; > #endif > return {this->fdcwd(), p.c_str()}; > > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Disinformation flourishes because many people care deeply about injustice > but very few check the facts. Ask me about <https://stallmansupport.org> >
On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > On Thu, 23 Jun 2022 at 12:08, Alexandre Oliva <oliva@adacore.com> wrote: >> >> On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: >> >> > There are other interactions between AT_CDCWD and ::openat not covered >> > by this patch. I this this also needs to check HAVE_OPENAT: >> >> Here's an updated version, tested with this additional change. > Did this improve your test results for directory iterators? 'fraid the bad results I posted earlier today had this patch in. I can't tell whether it improved anything because I didn't save earlier results to compare. > In the unlikely even that the target has ::unlinkat but not ::openat c++config.h on the target says: /* #undef _GLIBCXX_HAVE_UNLINKAT */ Thanks for the concern,
On Thu, 23 Jun 2022 at 15:05, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > On Thu, 23 Jun 2022 at 12:08, Alexandre Oliva <oliva@adacore.com> wrote: > >> > >> On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > >> > >> > There are other interactions between AT_CDCWD and ::openat not covered > >> > by this patch. I this this also needs to check HAVE_OPENAT: > >> > >> Here's an updated version, tested with this additional change. > > > Did this improve your test results for directory iterators? > > 'fraid the bad results I posted earlier today had this patch in. I > can't tell whether it improved anything because I didn't save earlier > results to compare. > > > In the unlikely even that the target has ::unlinkat but not ::openat > > c++config.h on the target says: > > /* #undef _GLIBCXX_HAVE_UNLINKAT */ > > Thanks for the concern, Here's what I'm thinking of so that the derived _Dir class doesn't need to know whether the base _Dir_base::openat function will actually use ::openat or not. It defines a new _At_path type which contains a {fd, path} pair (to be used together by openat and unlinkat) and a separate path to be used on its own. This allows identifying a file within a directory unambiguously, without being concerned with whether HAVE_OPENAT and HAVE_UNLINKAT are defined. With this change I don't think your patch to make dir_and_pathname() check HAVE_OPENAT is needed. This passes tests on x86_64-linux. diff --git a/libstdc++-v3/src/c++17/fs_dir.cc b/libstdc++-v3/src/c++17/fs_dir.cc index c67fe76bc14..fb752239e1f 100644 --- a/libstdc++-v3/src/c++17/fs_dir.cc +++ b/libstdc++-v3/src/c++17/fs_dir.cc @@ -46,7 +46,7 @@ struct fs::_Dir : _Dir_base { _Dir(const fs::path& p, bool skip_permission_denied, bool nofollow, [[maybe_unused]] bool filename_only, error_code& ec) - : _Dir_base(fdcwd(), p.c_str(), skip_permission_denied, nofollow, ec) + : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec) { #if _GLIBCXX_HAVE_DIRFD if (filename_only) @@ -120,15 +120,15 @@ struct fs::_Dir : _Dir_base // Return a file descriptor for the directory and current entry's path. // If dirfd is available, use it and return only the filename. // Otherwise, return AT_FDCWD and return the full path. - pair<int, const posix::char_type*> + _At_path dir_and_pathname() const noexcept { const fs::path& p = entry.path(); #if _GLIBCXX_HAVE_DIRFD if (!p.empty()) - return {::dirfd(this->dirp), std::prev(p.end())->c_str()}; + return {::dirfd(this->dirp), std::prev(p.end())->c_str(), p.c_str()}; #endif - return {this->fdcwd(), p.c_str()}; + return p.c_str(); } // Create a new _Dir for the directory this->entry.path(). @@ -136,8 +136,7 @@ struct fs::_Dir : _Dir_base open_subdir(bool skip_permission_denied, bool nofollow, error_code& ec) const noexcept { - auto [dirfd, pathname] = dir_and_pathname(); - _Dir_base d(dirfd, pathname, skip_permission_denied, nofollow, ec); + _Dir_base d(dir_and_pathname(), skip_permission_denied, nofollow, ec); // If this->path is empty, the new _Dir should have an empty path too. const fs::path& p = this->path.empty() ? this->path : this->entry.path(); return _Dir(std::move(d), p); @@ -147,7 +146,7 @@ struct fs::_Dir : _Dir_base do_unlink(bool is_directory, error_code& ec) const noexcept { #if _GLIBCXX_HAVE_UNLINKAT - auto [dirfd, pathname] = dir_and_pathname(); + auto [dirfd, pathname, _] = dir_and_pathname(); if (::unlinkat(dirfd, pathname, is_directory ? AT_REMOVEDIR : 0) == -1) { ec.assign(errno, std::generic_category()); diff --git a/libstdc++-v3/src/filesystem/dir-common.h b/libstdc++-v3/src/filesystem/dir-common.h index 365fd527f4d..5f7f4909ad3 100644 --- a/libstdc++-v3/src/filesystem/dir-common.h +++ b/libstdc++-v3/src/filesystem/dir-common.h @@ -91,12 +91,37 @@ is_permission_denied_error(int e) struct _Dir_base { + struct _At_path + { + _At_path(const char* p) noexcept + : dir_fd(fdcwd()), path_from_dir(p), pathname(p) + { } + + _At_path(int fd, const char* p1, const char* p2) + : dir_fd(fd), path_from_dir(p1), pathname(p2) + { } + + int dir_fd; // A directory descriptor (either the parent dir, or AT_FDCWD). + const posix::char_type* path_from_dir; // Path relative to dir_fd. + const posix::char_type* pathname; // Full path relative to CWD. + + static constexpr int + fdcwd() noexcept + { +#ifdef AT_FDCWD + return AT_FDCWD; +#else + return -1; // Use invalid fd if AT_FDCWD isn't supported. +#endif + } + }; + // If no error occurs then dirp is non-null, // otherwise null (even if a permission denied error is ignored). - _Dir_base(int fd, const posix::char_type* pathname, + _Dir_base(const _At_path& atp, bool skip_permission_denied, bool nofollow, error_code& ec) noexcept - : dirp(_Dir_base::openat(fd, pathname, nofollow)) + : dirp(_Dir_base::openat(atp, nofollow)) { if (dirp) ec.clear(); @@ -143,16 +168,6 @@ struct _Dir_base } } - static constexpr int - fdcwd() noexcept - { -#ifdef AT_FDCWD - return AT_FDCWD; -#else - return -1; // Use invalid fd if AT_FDCWD isn't supported. -#endif - } - static bool is_dot_or_dotdot(const char* s) noexcept { return !strcmp(s, ".") || !strcmp(s, ".."); } @@ -174,7 +189,7 @@ struct _Dir_base } static posix::DIR* - openat(int fd, const posix::char_type* pathname, bool nofollow) + openat(const _At_path& atp, bool nofollow) { #if _GLIBCXX_HAVE_FDOPENDIR && defined O_RDONLY && defined O_DIRECTORY \ && ! _GLIBCXX_FILESYSTEM_IS_WINDOWS @@ -198,16 +213,17 @@ struct _Dir_base nofollow = false; #endif + int fd; #ifdef AT_FDCWD - fd = ::openat(fd, pathname, flags); + fd = ::openat(atp.dir_fd, atp.path_from_dir, flags); #else // If we cannot use openat, there's no benefit to using posix::open unless // we will use O_NOFOLLOW, so just use the simpler posix::opendir. if (!nofollow) - return posix::opendir(pathname); + return posix::opendir(atp.pathname); - fd = ::open(pathname, flags); + fd = ::open(atp.pathname, flags); #endif if (fd == -1) @@ -220,7 +236,7 @@ struct _Dir_base errno = err; return nullptr; #else - return posix::opendir(pathname); + return posix::opendir(atp.pathname); #endif } diff --git a/libstdc++-v3/src/filesystem/dir.cc b/libstdc++-v3/src/filesystem/dir.cc index b451902c4a1..c0202849bc4 100644 --- a/libstdc++-v3/src/filesystem/dir.cc +++ b/libstdc++-v3/src/filesystem/dir.cc @@ -53,7 +53,7 @@ struct fs::_Dir : std::filesystem::_Dir_base { _Dir(const fs::path& p, bool skip_permission_denied, bool nofollow, error_code& ec) - : _Dir_base(this->fdcwd(), p.c_str(), skip_permission_denied, nofollow, ec) + : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec) { if (!ec) path = p; @@ -116,14 +116,14 @@ struct fs::_Dir : std::filesystem::_Dir_base // Return a file descriptor for the directory and current entry's path. // If dirfd is available, use it and return only the filename. // Otherwise, return AT_FDCWD and return the full path. - pair<int, const posix::char_type*> + _At_path dir_and_pathname() const noexcept { const fs::path& p = entry.path(); #if _GLIBCXX_HAVE_DIRFD - return {::dirfd(this->dirp), std::prev(p.end())->c_str()}; + return {::dirfd(this->dirp), std::prev(p.end())->c_str(), p.c_str()}; #endif - return {this->fdcwd(), p.c_str()}; + return p.c_str(); } // Create a new _Dir for the directory this->entry.path(). @@ -131,8 +131,7 @@ struct fs::_Dir : std::filesystem::_Dir_base open_subdir(bool skip_permission_denied, bool nofollow, error_code& ec) noexcept { - auto [dirfd, pathname] = dir_and_pathname(); - _Dir_base d(dirfd, pathname, skip_permission_denied, nofollow, ec); + _Dir_base d(dir_and_pathname(), skip_permission_denied, nofollow, ec); return _Dir(std::move(d), entry.path()); }
On Jun 23, 2022, Alexandre Oliva <oliva@adacore.com> wrote:
> * aclocal.m4, configure, config.h.in: Rebuilt.
Dropped aclocal.m4 for the checkin, thanks to the ChangeLog checker.
On Thu, 23 Jun 2022 at 15:05, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > On Thu, 23 Jun 2022 at 12:08, Alexandre Oliva <oliva@adacore.com> wrote: > >> > >> On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > >> > >> > There are other interactions between AT_CDCWD and ::openat not covered > >> > by this patch. I this this also needs to check HAVE_OPENAT: > >> > >> Here's an updated version, tested with this additional change. > > > Did this improve your test results for directory iterators? > > 'fraid the bad results I posted earlier today had this patch in. I Ah, but this patch only added the HAVE_OPENAT check to src/filesystem/dir.cc not the similar code in src/c++17/fs_dir.cc So it only affected std::experimental::filesystem and I would expect the std::filesystem directory iterator tests to fail still. Nevermind, I'm going to make a change that resolves it anyway. > can't tell whether it improved anything because I didn't save earlier > results to compare. > > > In the unlikely even that the target has ::unlinkat but not ::openat > > c++config.h on the target says: > > /* #undef _GLIBCXX_HAVE_UNLINKAT */ > > Thanks for the concern, > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Disinformation flourishes because many people care deeply about injustice > but very few check the facts. Ask me about <https://stallmansupport.org> >
On Jun 24, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > Ah, but this patch only added the HAVE_OPENAT check to > src/filesystem/dir.cc not the similar code in src/c++17/fs_dir.cc Doh! It was such a long and goof-ful day :-( Thanks for catching it. Unless you ask me not to, I'd like to install this to complete the earlier patch. I've given it some testing, along with your _At_path patch, and results are looking promising. There are still a few remaining fails. libstdc++: check for openat with dirfd in std::filesystem From: Alexandre Oliva <oliva@adacore.com> In the recent patch to check for openat, I missed an occurrence of dirfd in std::filesystem. for libstdc++-v3/ChangeLog * src/c++17/fs_dir.cc (dir_and_pathname): Use dirfd if _GLIBCXX_HAVE_OPENAT. --- libstdc++-v3/src/c++17/fs_dir.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libstdc++-v3/src/c++17/fs_dir.cc b/libstdc++-v3/src/c++17/fs_dir.cc index c67fe76bc14f8..25b33baa875fb 100644 --- a/libstdc++-v3/src/c++17/fs_dir.cc +++ b/libstdc++-v3/src/c++17/fs_dir.cc @@ -124,7 +124,7 @@ struct fs::_Dir : _Dir_base dir_and_pathname() const noexcept { const fs::path& p = entry.path(); -#if _GLIBCXX_HAVE_DIRFD +#if _GLIBCXX_HAVE_DIRFD && _GLIBCXX_HAVE_OPENAT if (!p.empty()) return {::dirfd(this->dirp), std::prev(p.end())->c_str()}; #endif
On Mon, 27 Jun 2022 at 10:49, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jun 24, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > Ah, but this patch only added the HAVE_OPENAT check to > > src/filesystem/dir.cc not the similar code in src/c++17/fs_dir.cc > > Doh! It was such a long and goof-ful day :-( > > Thanks for catching it. > > Unless you ask me not to, I'd like to install this to complete the > earlier patch. OK, although I'll just remove it again as part of the _At_path patch that makes this extra check unnecessary. But this part would be a candidate for backporting. > I've given it some testing, along with your _At_path > patch, and results are looking promising. There are still a few > remaining fails. > > > libstdc++: check for openat with dirfd in std::filesystem > > From: Alexandre Oliva <oliva@adacore.com> > > In the recent patch to check for openat, I missed an occurrence of > dirfd in std::filesystem. > > > for libstdc++-v3/ChangeLog > > * src/c++17/fs_dir.cc (dir_and_pathname): Use dirfd if > _GLIBCXX_HAVE_OPENAT. > --- > libstdc++-v3/src/c++17/fs_dir.cc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libstdc++-v3/src/c++17/fs_dir.cc b/libstdc++-v3/src/c++17/fs_dir.cc > index c67fe76bc14f8..25b33baa875fb 100644 > --- a/libstdc++-v3/src/c++17/fs_dir.cc > +++ b/libstdc++-v3/src/c++17/fs_dir.cc > @@ -124,7 +124,7 @@ struct fs::_Dir : _Dir_base > dir_and_pathname() const noexcept > { > const fs::path& p = entry.path(); > -#if _GLIBCXX_HAVE_DIRFD > +#if _GLIBCXX_HAVE_DIRFD && _GLIBCXX_HAVE_OPENAT > if (!p.empty()) > return {::dirfd(this->dirp), std::prev(p.end())->c_str()}; > #endif > > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Disinformation flourishes because many people care deeply about injustice > but very few check the facts. Ask me about <https://stallmansupport.org> >
On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > It defines a new _At_path type which contains a {fd, path} pair (to be > used together by openat and unlinkat) and a separate path to be used > on its own. This allows identifying a file within a directory > unambiguously, without being concerned with whether HAVE_OPENAT and > HAVE_UNLINKAT are defined. > With this change I don't think your patch to make dir_and_pathname() > check HAVE_OPENAT is needed. *nod*, I've amended your patch in my local tree to reverse them both. > This passes tests on x86_64-linux. Have you by any chance tried it while forcing libstdc++ not to use openat? I'm debugging 27_io/.../copy.cc on rtems with a fixed remove_all (there were bugs in the previous version I posted), and hitting an exception within the very first call of dir.__erase() in the remove_all at the end of test_pr99290, after an attempt to open "source" fails. It looks like the atp.pathname is missing the nonexistent_path assigned to variable dir in test_pr99290, so we attempt to open subdirs thereof as if with openat.
On Jun 27, 2022, Alexandre Oliva <oliva@adacore.com> wrote: > It looks like the atp.pathname is missing the nonexistent_path > assigned to variable dir in test_pr99290, so we attempt to open > subdirs thereof as if with openat. This appears to be caused by the early return in fs::_Dir's ctor: _Dir(const fs::path& p, bool skip_permission_denied, bool nofollow, [[maybe_unused]] bool filename_only, error_code& ec) : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec) { #if _GLIBCXX_HAVE_DIRFD // && 0 if (filename_only) return; // Do not store path p when we aren't going to use it. #endif if (!ec) path = p; } but somehow disabling the early return to force the saving of path appears to break copy(): copy.cc's test01() succeeded without the '&& 0' that I've commented-out above, but started failing to create 'to' in the copy at line copy.cc:54 when I put it in to prevent the early return. Does that make any sense to you?
On Mon, 27 Jun 2022 at 14:05, Alexandre Oliva <oliva@adacore.com> wrote: > > On Jun 27, 2022, Alexandre Oliva <oliva@adacore.com> wrote: > > > It looks like the atp.pathname is missing the nonexistent_path > > assigned to variable dir in test_pr99290, so we attempt to open > > subdirs thereof as if with openat. > > This appears to be caused by the early return in fs::_Dir's ctor: > > _Dir(const fs::path& p, bool skip_permission_denied, bool nofollow, > [[maybe_unused]] bool filename_only, error_code& ec) > : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec) > { > #if _GLIBCXX_HAVE_DIRFD // && 0 > if (filename_only) > return; // Do not store path p when we aren't going to use it. > #endif Yes, this needs a fix. If we don't have openat then we always need a full path relative to the CWD, not just a filename relative to a file descriptor for the parent directory. I think we need to store the directory's path if any of dirfd, openat or unlinkat is missing. > > if (!ec) > path = p; > } > > but somehow disabling the early return to force the saving of path > appears to break copy(): copy.cc's test01() succeeded without the '&& 0' > that I've commented-out above, but started failing to create 'to' in the > copy at line copy.cc:54 when I put it in to prevent the early return. > > Does that make any sense to you? No, I'll have to debug the test. I thought that not storing the path was just an optimization (to avoid parsing, decomposing, and allocating a path object that we will never use).
On Mon, 27 Jun 2022 at 14:32, Jonathan Wakely <jwakely@redhat.com> wrote: > > On Mon, 27 Jun 2022 at 14:05, Alexandre Oliva <oliva@adacore.com> wrote: > > > > On Jun 27, 2022, Alexandre Oliva <oliva@adacore.com> wrote: > > > > > It looks like the atp.pathname is missing the nonexistent_path > > > assigned to variable dir in test_pr99290, so we attempt to open > > > subdirs thereof as if with openat. > > > > This appears to be caused by the early return in fs::_Dir's ctor: > > > > _Dir(const fs::path& p, bool skip_permission_denied, bool nofollow, > > [[maybe_unused]] bool filename_only, error_code& ec) > > : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec) > > { > > #if _GLIBCXX_HAVE_DIRFD // && 0 > > if (filename_only) > > return; // Do not store path p when we aren't going to use it. > > #endif > > Yes, this needs a fix. If we don't have openat then we always need a > full path relative to the CWD, not just a filename relative to a file > descriptor for the parent directory. > > I think we need to store the directory's path if any of dirfd, openat > or unlinkat is missing. > > > > > > if (!ec) > > path = p; > > } > > > > but somehow disabling the early return to force the saving of path > > appears to break copy(): copy.cc's test01() succeeded without the '&& 0' > > that I've commented-out above, but started failing to create 'to' in the > > copy at line copy.cc:54 when I put it in to prevent the early return. > > > > Does that make any sense to you? > > No, I'll have to debug the test. I thought that not storing the path > was just an optimization (to avoid parsing, decomposing, and > allocating a path object that we will never use). If I add the && 0 (so we don't store the path) and bodge <bits/c++config.h> so that HAVE_OPENAT and HAVE_UNLINKAT are both undefined, I do not see any new FAIL in the filesystem/operations/copy.cc tests. filesystem::copy doesn't even use recursive_directory_iterator, and the filename_only bool should always be false for non-recursive directory_iterator, and so the early return should never happen anyway. (I do have an uncommitted patch to avoid storing that path in more cases, including for non-recursive directory iterators, but it still wouldn't be true for filesystem::copy).
On Mon, 27 Jun 2022 at 15:00, Jonathan Wakely <jwakely@redhat.com> wrote: > > On Mon, 27 Jun 2022 at 14:32, Jonathan Wakely <jwakely@redhat.com> wrote: > > > > On Mon, 27 Jun 2022 at 14:05, Alexandre Oliva <oliva@adacore.com> wrote: > > > > > > On Jun 27, 2022, Alexandre Oliva <oliva@adacore.com> wrote: > > > > > > > It looks like the atp.pathname is missing the nonexistent_path > > > > assigned to variable dir in test_pr99290, so we attempt to open > > > > subdirs thereof as if with openat. > > > > > > This appears to be caused by the early return in fs::_Dir's ctor: > > > > > > _Dir(const fs::path& p, bool skip_permission_denied, bool nofollow, > > > [[maybe_unused]] bool filename_only, error_code& ec) > > > : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec) > > > { > > > #if _GLIBCXX_HAVE_DIRFD // && 0 > > > if (filename_only) > > > return; // Do not store path p when we aren't going to use it. > > > #endif > > > > Yes, this needs a fix. If we don't have openat then we always need a > > full path relative to the CWD, not just a filename relative to a file > > descriptor for the parent directory. > > > > I think we need to store the directory's path if any of dirfd, openat > > or unlinkat is missing. i.e. --- a/libstdc++-v3/src/c++17/fs_dir.cc +++ b/libstdc++-v3/src/c++17/fs_dir.cc @@ -48,7 +48,7 @@ struct fs::_Dir : _Dir_base [[maybe_unused]] bool filename_only, error_code& ec) : _Dir_base(p.c_str(), skip_permission_denied, nofollow, ec) { -#if _GLIBCXX_HAVE_DIRFD +#if _GLIBCXX_HAVE_DIRFD && _GLIBCXX_HAVE_OPENAT && _GLIBCXX_HAVE_UNLINKAT if (filename_only) return; // Do not store path p when we aren't going to use it. #endif > > > > > > > > > > if (!ec) > > > path = p; > > > } > > > > > > but somehow disabling the early return to force the saving of path > > > appears to break copy(): copy.cc's test01() succeeded without the '&& 0' > > > that I've commented-out above, but started failing to create 'to' in the > > > copy at line copy.cc:54 when I put it in to prevent the early return. N.B. it's not supposed to create 'to' there, that line is checking that it fails and reports an error. Could the FAIL simply be that rtems gives a different error, not EISDIR as the test expects? VERIFY( ec == std::make_error_code(std::errc::is_a_directory) ); Although that shouldn't depend on anything target-specific, because the libstdc++ code itself reports that error: else if (is_directory(f) && create_symlinks) ec = std::make_error_code(errc::is_a_directory);
On Jun 27, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > -#if _GLIBCXX_HAVE_DIRFD > +#if _GLIBCXX_HAVE_DIRFD && _GLIBCXX_HAVE_OPENAT && _GLIBCXX_HAVE_UNLINKAT Thanks, I had this bit in the WIP _At_path patch, as well as my updated remove_all patch, in the latest round of tests. I confirm that I don't get any errors with it on an x86_64-linux-gnu native built with openat and unlinkat forced disabled in libstdc++, but on aarch64-rtems6, there are some fails that are not making much sense to me. I haven't looked into any but 27_io/.../copy.cc, so I can't tell whether they've regressed, but copy.cc's test01 did when I put this (or variants thereof) in. Tomorrow I'll look into building libstdc++ with -Og or something else with less optimization than the default, to help me make sense of what's going on that causes copy() to return without as much as creating the destination. FAILED: default@libstdc++,27_io,filesystem,operations,copy_cc FAILED: default@libstdc++,experimental,filesystem,operations,copy_cc .../27_io/filesystem/operations/copy.cc:5[67]: void test01(): Assertion '!exists(to)' failed. FAILED: default@libstdc++,27_io,filesystem,operations,copy_file_cc FAILED: default@libstdc++,experimental,filesystem,operations,copy_file_cc terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error' what(): filesystem error: cannot copy file: File exists [filesystem-test.000001-copy_file] [filesystem-test.000001-copy_file] FAILED: default@libstdc++,27_io,filesystem,operations,equivalent_cc FAILED: default@libstdc++,experimental,filesystem,operations,equivalent_cc .../27_io/filesystem/operations/equivalent.cc:4[45]: void test01(): Assertion '!result' failed. FAILED: default@libstdc++,27_io,filesystem,operations,relative_cc .../27_io/filesystem/operations/relative.cc:32: void test01(): Assertion 'r == ".."/p' failed. Also still present: .../20_util/from_chars/4.cc:304: error: 'log2' is not a member of 'std'; did you mean 'log'? (IIRC we used to have two of these, we're now down to one)
On Mon, 27 Jun 2022 at 23:04, Alexandre Oliva via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > On Jun 27, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > -#if _GLIBCXX_HAVE_DIRFD > > +#if _GLIBCXX_HAVE_DIRFD && _GLIBCXX_HAVE_OPENAT && _GLIBCXX_HAVE_UNLINKAT I'll push this today. > Thanks, I had this bit in the WIP _At_path patch, as well as my updated > remove_all patch, in the latest round of tests. > > I confirm that I don't get any errors with it on an x86_64-linux-gnu > native built with openat and unlinkat forced disabled in libstdc++, but > on aarch64-rtems6, there are some fails that are not making much sense > to me. I haven't looked into any but 27_io/.../copy.cc, so I can't tell > whether they've regressed, but copy.cc's test01 did when I put this (or > variants thereof) in. > > Tomorrow I'll look into building libstdc++ with -Og or something else > with less optimization than the default, to help me make sense of what's You can just use --enable-libstdcxx-debug and then link to the libs in $libdir/debug instead of $libdir (e.g. with LD_LIBRARY_PATH). > going on that causes copy() to return without as much as creating the > destination. Again, that test is *supposed* to return without creating the destination. It's testing the failure case. > > > FAILED: default@libstdc++,27_io,filesystem,operations,copy_cc > FAILED: default@libstdc++,experimental,filesystem,operations,copy_cc > > .../27_io/filesystem/operations/copy.cc:5[67]: void test01(): Assertion '!exists(to)' failed. I don't know what 5[67] means, so I don't know if that's the first or second !exists(to) assertion, but this is not failing because copy returned without creating something. copy is not supposed to create anything here, and it didn't, because the previous assertion (ec == std::make_error_code(std::errc::is_a_directory)) didn't fail. So the copy function behaves correctly, but the 'to' file exists anyway. Which suggests to me another problem with mkstemp / nonexistent_path. Before rebuilding the library I'd just add an extra assertion into the test. --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/copy.cc @@ -49,6 +49,7 @@ test01() VERIFY( ec ); auto to = __gnu_test::nonexistent_path(); + VERIFY( !exists(to) ); ec.clear(); auto opts = fs::copy_options::create_symlinks; fs::copy("/", to, opts, ec); I suspect this is going to fail, because 'to' already exists before you ever call copy("/", to, opts, ec). And that would happen is nonexistent_path is still broken. Do you have the commit r13-1289-g30a8f67295f412 in your tree? > FAILED: default@libstdc++,27_io,filesystem,operations,copy_file_cc > FAILED: default@libstdc++,experimental,filesystem,operations,copy_file_cc > > terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error' > what(): filesystem error: cannot copy file: File exists [filesystem-test.000001-copy_file] [filesystem-test.000001-copy_file] Again, this looks like a problem with nonexistent_path, the 'from' and 'to' paths are the same: auto from = __gnu_test::nonexistent_path(); auto to = __gnu_test::nonexistent_path(); > FAILED: default@libstdc++,27_io,filesystem,operations,equivalent_cc > FAILED: default@libstdc++,experimental,filesystem,operations,equivalent_cc > > .../27_io/filesystem/operations/equivalent.cc:4[45]: void test01(): Assertion '!result' failed. This would also happen if nonexistent_path still returns the same value twice. > > FAILED: default@libstdc++,27_io,filesystem,operations,relative_cc > > .../27_io/filesystem/operations/relative.cc:32: void test01(): Assertion 'r == ".."/p' failed. Ditto. > > Also still present: > > .../20_util/from_chars/4.cc:304: error: 'log2' is not a member of 'std'; did you mean 'log'? > > (IIRC we used to have two of these, we're now down to one) That should be fixed at r13-1315-g30aea28bd30027 now.
On Jun 28, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > I'll push this today. Thanks! > You can just use --enable-libstdcxx-debug Thanks again ;-) > Again, that test is *supposed* to return without creating the > destination. It's testing the failure case. Aha, and that's why one shouldn't debug something without looking at the code to see what it's *supposed* to do ;-) >> FAILED: default@libstdc++,27_io,filesystem,operations,copy_cc >> FAILED: default@libstdc++,experimental,filesystem,operations,copy_cc >> >> .../27_io/filesystem/operations/copy.cc:5[67]: void test01(): >> Assertion '!exists(to)' failed. > I don't know what 5[67] means Sorry for being unclear, it's just that the corresponding failing asserts are at different lines in the two mentioned testcases, and I tried to convey that fact with regexp notation. > Which suggests to me another problem with mkstemp / nonexistent_path. *lightbulb powers up* Now it all makes sense. It isn't *another* problem, that probably regressed when the mkstemp patch went in and so it got out of my radar and thus out of the patchset I used in subsequent test runs, but because of the way I use the testing system, the baseline on top of which the patchset was installed was still was still that of the previous nightly build, so I effectively dropped the mkstemp fix. And since when I joined this project this bug had already been fixed, I didn't associate the regressions with the patch. Apologies for the noise. Today's baseline, plus your _At_path patch and my remove_all patch, is all clear. Yay!
On Tue, 28 Jun 2022 at 13:04, Alexandre Oliva <oliva@gnu.org> wrote: > > On Jun 28, 2022, Jonathan Wakely <jwakely@redhat.com> wrote: > > > I'll push this today. > > Thanks! > > > You can just use --enable-libstdcxx-debug > > Thanks again ;-) > > > Again, that test is *supposed* to return without creating the > > destination. It's testing the failure case. > > Aha, and that's why one shouldn't debug something without looking at the > code to see what it's *supposed* to do ;-) > > >> FAILED: default@libstdc++,27_io,filesystem,operations,copy_cc > >> FAILED: default@libstdc++,experimental,filesystem,operations,copy_cc > >> > >> .../27_io/filesystem/operations/copy.cc:5[67]: void test01(): > >> Assertion '!exists(to)' failed. > > > I don't know what 5[67] means > > Sorry for being unclear, it's just that the corresponding failing > asserts are at different lines in the two mentioned testcases, and I > tried to convey that fact with regexp notation. Doh! Of course. I thought it was some rtems thing. /facepalm > > > Which suggests to me another problem with mkstemp / nonexistent_path. > > *lightbulb powers up* > > Now it all makes sense. > > It isn't *another* problem, that probably regressed when the mkstemp > patch went in and so it got out of my radar and thus out of the patchset > I used in subsequent test runs, but because of the way I use the testing > system, the baseline on top of which the patchset was installed was > still was still that of the previous nightly build, so I effectively > dropped the mkstemp fix. And since when I joined this project this bug > had already been fixed, I didn't associate the regressions with the > patch. Makes sense. > Apologies for the noise. Today's baseline, plus your _At_path patch and > my remove_all patch, is all clear. Yay! Great!
diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4 index 138bd58d86cb9..e3cc3a8e867d3 100644 --- a/libstdc++-v3/acinclude.m4 +++ b/libstdc++-v3/acinclude.m4 @@ -4772,6 +4772,18 @@ dnl if test $glibcxx_cv_dirfd = yes; then AC_DEFINE(HAVE_DIRFD, 1, [Define if dirfd is available in <dirent.h>.]) fi +dnl + AC_CACHE_CHECK([for openat], + glibcxx_cv_openat, [dnl + GCC_TRY_COMPILE_OR_LINK( + [#include <fcntl.h>], + [int fd = ::openat(AT_FDCWD, "", 0);], + [glibcxx_cv_openat=yes], + [glibcxx_cv_openat=no]) + ]) + if test $glibcxx_cv_openat = yes; then + AC_DEFINE(HAVE_OPENAT, 1, [Define if openat is available in <fcntl.h>.]) + fi dnl AC_CACHE_CHECK([for unlinkat], glibcxx_cv_unlinkat, [dnl diff --git a/libstdc++-v3/config.h.in b/libstdc++-v3/config.h.in index f30a8c51c458c..2a3972eef5412 100644 --- a/libstdc++-v3/config.h.in +++ b/libstdc++-v3/config.h.in @@ -292,6 +292,9 @@ /* Define if <math.h> defines obsolete isnan function. */ #undef HAVE_OBSOLETE_ISNAN +/* Define if openat is available in <fcntl.h>. */ +#undef HAVE_OPENAT + /* Define if poll is available in <poll.h>. */ #undef HAVE_POLL diff --git a/libstdc++-v3/configure b/libstdc++-v3/configure index 9b94fd71e4248..eac6039212168 100755 --- a/libstdc++-v3/configure +++ b/libstdc++-v3/configure @@ -77177,6 +77177,61 @@ $as_echo "$glibcxx_cv_dirfd" >&6; } $as_echo "#define HAVE_DIRFD 1" >>confdefs.h + fi + { $as_echo "$as_me:${as_lineno-$LINENO}: checking for openat" >&5 +$as_echo_n "checking for openat... " >&6; } +if ${glibcxx_cv_openat+:} false; then : + $as_echo_n "(cached) " >&6 +else + if test x$gcc_no_link = xyes; then + cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include <fcntl.h> +int +main () +{ +int fd = ::openat(AT_FDCWD, "", 0); + ; + return 0; +} +_ACEOF +if ac_fn_cxx_try_compile "$LINENO"; then : + glibcxx_cv_openat=yes +else + glibcxx_cv_openat=no +fi +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext +else + if test x$gcc_no_link = xyes; then + as_fn_error $? "Link tests are not allowed after GCC_NO_EXECUTABLES." "$LINENO" 5 +fi +cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include <fcntl.h> +int +main () +{ +int fd = ::openat(AT_FDCWD, "", 0); + ; + return 0; +} +_ACEOF +if ac_fn_cxx_try_link "$LINENO"; then : + glibcxx_cv_openat=yes +else + glibcxx_cv_openat=no +fi +rm -f core conftest.err conftest.$ac_objext \ + conftest$ac_exeext conftest.$ac_ext +fi + +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $glibcxx_cv_openat" >&5 +$as_echo "$glibcxx_cv_openat" >&6; } + if test $glibcxx_cv_openat = yes; then + +$as_echo "#define HAVE_OPENAT 1" >>confdefs.h + fi { $as_echo "$as_me:${as_lineno-$LINENO}: checking for unlinkat" >&5 $as_echo_n "checking for unlinkat... " >&6; } diff --git a/libstdc++-v3/src/filesystem/dir-common.h b/libstdc++-v3/src/filesystem/dir-common.h index 365fd527f4d68..669780ea23fe5 100644 --- a/libstdc++-v3/src/filesystem/dir-common.h +++ b/libstdc++-v3/src/filesystem/dir-common.h @@ -199,7 +199,7 @@ struct _Dir_base #endif -#ifdef AT_FDCWD +#if _GLIBCXX_HAVE_OPENAT && defined AT_FDCWD fd = ::openat(fd, pathname, flags); #else // If we cannot use openat, there's no benefit to using posix::open unless