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

[Jonathan Wakely <jwakely@redhat.com>]

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