From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 1DBE438344D7 for ; Wed, 22 Jun 2022 10:36:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1DBE438344D7 Received: from mail-ej1-f69.google.com (mail-ej1-f69.google.com [209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-329-QknQiSBZOuCs4q_qKzPkBQ-1; Wed, 22 Jun 2022 06:36:30 -0400 X-MC-Unique: QknQiSBZOuCs4q_qKzPkBQ-1 Received: by mail-ej1-f69.google.com with SMTP id ne36-20020a1709077ba400b00722d5f547d8so2994375ejc.19 for ; Wed, 22 Jun 2022 03:36:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=6QAyKkeJoVqUMRg9vi9IU3kH64McwPGHDxdzi/SQjrA=; b=rGy0Z2abMzuRaJ4iQLt8Mrf55l9RdbAjesMuKYVoUDeT1ofoK/YQMDuO53XKUcRyBD r93GwCStecn8YG++qq9Hvm5x0G2hfFHAPMEAKhmuMPDs6iO4veKF7CntF4Fhdk1bFNNY +ywNsg26NJWVVvYR3A/8qSBBi7he2Yw2KQEr/0L31xWwkphENtZgf1IVORql2axauvQg NBa1SWCRvU94T4Al9qQ/Q2CY4PcmcOgkv2kVDN10BVNzx0OiID1MAuZNP2OVUYbq/Skh 5nPuUOjyx9/vOjOWQ4KUBI3vZn8a5lisB4pdNmVjEBqXJDRELOOfLq2vc0BxL82xFyPF pH+A== X-Gm-Message-State: AJIora8kTuGmbNO5St38hZj35ve81qB/F1EbdVxT/xCGW9N28v4LT2Ef Mab6B1noW7/WUUSijOqbZtJngxd/HP+p1cG/M6KpDV734SibiZNHTqa6cCaOpQh+iwRumCcnDku NTcednYrSwoT2RWtS0PRhnM5bfRQ2o+E= X-Received: by 2002:a17:907:6294:b0:6e1:ea4:74a3 with SMTP id nd20-20020a170907629400b006e10ea474a3mr2480083ejc.168.1655894189227; Wed, 22 Jun 2022 03:36:29 -0700 (PDT) X-Google-Smtp-Source: AGRyM1u+sN0UqjM07iY1FPbjbvSN967OpVQjk1Eph7gog5fVdDihCtLtK70fztmAf1fqM1yh3odcT29TcihEJxJxjvY= X-Received: by 2002:a17:907:6294:b0:6e1:ea4:74a3 with SMTP id nd20-20020a170907629400b006e10ea474a3mr2480055ejc.168.1655894188823; Wed, 22 Jun 2022 03:36:28 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Jonathan Wakely Date: Wed, 22 Jun 2022 11:36:17 +0100 Message-ID: Subject: Re: [PATCH] libstdc++-v3: check for openat To: Alexandre Oliva Cc: gcc Patches , "libstdc++" X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-13.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Jun 2022 10:36:34 -0000 On Wed, 22 Jun 2022 at 07:43, Alexandre Oliva via Libstdc++ 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 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 .]) > fi > +dnl > + AC_CACHE_CHECK([for openat], > + glibcxx_cv_openat, [dnl > + GCC_TRY_COMPILE_OR_LINK( > + [#include ], > + [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 .]) > + 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 defines obsolete isnan function. */ > #undef HAVE_OBSOLETE_ISNAN > > +/* Define if openat is available in . */ > +#undef HAVE_OPENAT > + > /* Define if poll is available in . */ > #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 > +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 > +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 >