[PATCH] libstdc++-v3: check for openat

[PATCH] libstdc++-v3: check for openat

[Alexandre Oliva]

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.


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>

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

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>
>

Re: [PATCH] libstdc++-v3: check for openat

[Alexandre Oliva]

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?

-- 
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>

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

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.

Re: [PATCH] libstdc++-v3: check for openat

[Alexandre Oliva]

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()};


-- 
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>

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

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>
>

Re: [PATCH] libstdc++-v3: check for openat

[Alexandre Oliva]

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,

-- 
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>

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]

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.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 6629 bytes --]

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());
   }
 

Re: [PATCH] libstdc++-v3: check for openat

[Alexandre Oliva]

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.

-- 
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>

Re: [PATCH] libstdc++-v3: check for openat

[Alexandre Oliva]

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?

-- 
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>

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

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).

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

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).

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

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);

Re: [PATCH] libstdc++-v3: check for openat

[Alexandre Oliva]

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)

-- 
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>

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

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.

Re: [PATCH] libstdc++-v3: check for openat

[Alexandre Oliva]

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!

-- 
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>

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

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!

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

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>
>

Re: [PATCH] libstdc++-v3: check for openat

[Alexandre Oliva]

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


-- 
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>

Re: [PATCH] libstdc++-v3: check for openat

[Jonathan Wakely]

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>
>

Re: [PATCH] libstdc++-v3: check for openat

[Alexandre Oliva]

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.

-- 
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>