| Message ID | 20220207201058.250114-1-dimitar@dinux.eu |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: Decouple HAVE_FCNTL_H from HAVE_DIRENT_H check | expand |
On Mon, 7 Feb 2022 at 20:12, Dimitar Dimitrov <dimitar@dinux.eu> wrote: > On PRU target with newlib, we have the following combination in config.h: > /* #undef HAVE_DIRENT_H */ > #define HAVE_FCNTL_H 1 > #define HAVE_UNLINKAT 1 > > In newlib, targets which do not define dirent.h, get a build error when > including <dirent.h>: > > https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/sys/dirent.h;hb=HEAD > > While fs_dir.cc correctly checks for HAVE_FCNTL_H, dir-common.h doesn't, > and instead uses HAVE_DIRENT_H. This results in unlinkat() function call > in fs_dir.cc without the needed <fcntl.h> include in dir-common.h. Thus > a build failure: > .../gcc/libstdc++-v3/src/c++17/fs_dir.cc:151:11: error: ‘::unlinkat’ has > not been declared; did you mean ‘unlink’? > > Fix by encapsulating <fcntl.h> include with the correct check. > But there's no point doing anything in that file if we don't have <dirent.h>, the whole thing is unusable. There's no point making the members using unlinkat compile if you can't ever construct the type. So I think we want a different fix. > Regtested x86_64-pc-linux-gnu and no new failures detected. > > Fixes commit r12-7062-gebf61754647689 (libstdc++: Fix > filesystem::remove_all races). > > libstdc++-v3/ChangeLog: > > * src/filesystem/dir-common.h (_GLIBCXX_HAVE_FCNTL_H): Move the > check outside the HAVE_DIRENT_H check. > > Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu> > --- > libstdc++-v3/src/filesystem/dir-common.h | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/libstdc++-v3/src/filesystem/dir-common.h > b/libstdc++-v3/src/filesystem/dir-common.h > index 0b7665a3f70..cfce4fae9a4 100644 > --- a/libstdc++-v3/src/filesystem/dir-common.h > +++ b/libstdc++-v3/src/filesystem/dir-common.h > @@ -35,10 +35,11 @@ > # include <sys/types.h> > # endif > # include <dirent.h> // opendir, readdir, fdopendir, dirfd > -# ifdef _GLIBCXX_HAVE_FCNTL_H > -# include <fcntl.h> // open, openat, fcntl, AT_FDCWD, O_NOFOLLOW etc. > -# include <unistd.h> // close, unlinkat > -# endif > +#endif > + > +#ifdef _GLIBCXX_HAVE_FCNTL_H > +# include <fcntl.h> // open, openat, fcntl, AT_FDCWD, O_NOFOLLOW etc. > +# include <unistd.h> // close, unlinkat > #endif > > namespace std _GLIBCXX_VISIBILITY(default) > -- > 2.34.1 > >
On Mon, 7 Feb 2022 at 21:01, Jonathan Wakely <jwakely.gcc@gmail.com> wrote: > > > On Mon, 7 Feb 2022 at 20:12, Dimitar Dimitrov <dimitar@dinux.eu> wrote: > >> On PRU target with newlib, we have the following combination in config.h: >> /* #undef HAVE_DIRENT_H */ >> #define HAVE_FCNTL_H 1 >> #define HAVE_UNLINKAT 1 >> >> In newlib, targets which do not define dirent.h, get a build error when >> including <dirent.h>: >> >> https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/sys/dirent.h;hb=HEAD >> >> While fs_dir.cc correctly checks for HAVE_FCNTL_H, dir-common.h doesn't, >> and instead uses HAVE_DIRENT_H. This results in unlinkat() function call >> in fs_dir.cc without the needed <fcntl.h> include in dir-common.h. Thus >> a build failure: >> .../gcc/libstdc++-v3/src/c++17/fs_dir.cc:151:11: error: ‘::unlinkat’ >> has not been declared; did you mean ‘unlink’? >> >> Fix by encapsulating <fcntl.h> include with the correct check. >> > > But there's no point doing anything in that file if we don't have > <dirent.h>, the whole thing is unusable. There's no point making the > members using unlinkat compile if you can't ever construct the type. > > So I think we want a different fix. > Maybe something like: --- a/libstdc++-v3/src/filesystem/dir-common.h +++ b/libstdc++-v3/src/filesystem/dir-common.h @@ -70,6 +70,8 @@ struct DIR { }; inline DIR* opendir(const char*) { return nullptr; } inline dirent* readdir(DIR*) { return nullptr; } inline int closedir(DIR*) { return -1; } +#undef _GLIBCXX_HAVE_DIRFD +#undef _GLIBCXX_HAVE_UNLINKAT #endif } // namespace __gnu_posix Or adding wrappers for those in namespace posix.
On Mon, Feb 07, 2022 at 09:05:45PM +0000, Jonathan Wakely wrote: > On Mon, 7 Feb 2022 at 21:01, Jonathan Wakely <jwakely.gcc@gmail.com> wrote: > > > > > > > On Mon, 7 Feb 2022 at 20:12, Dimitar Dimitrov <dimitar@dinux.eu> wrote: > > > >> On PRU target with newlib, we have the following combination in config.h: > >> /* #undef HAVE_DIRENT_H */ > >> #define HAVE_FCNTL_H 1 > >> #define HAVE_UNLINKAT 1 > >> > >> In newlib, targets which do not define dirent.h, get a build error when > >> including <dirent.h>: > >> > >> https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/sys/dirent.h;hb=HEAD > >> > >> While fs_dir.cc correctly checks for HAVE_FCNTL_H, dir-common.h doesn't, > >> and instead uses HAVE_DIRENT_H. This results in unlinkat() function call > >> in fs_dir.cc without the needed <fcntl.h> include in dir-common.h. Thus > >> a build failure: > >> .../gcc/libstdc++-v3/src/c++17/fs_dir.cc:151:11: error: ‘::unlinkat’ > >> has not been declared; did you mean ‘unlink’? > >> > >> Fix by encapsulating <fcntl.h> include with the correct check. > >> > > > > But there's no point doing anything in that file if we don't have > > <dirent.h>, the whole thing is unusable. There's no point making the > > members using unlinkat compile if you can't ever construct the type. > > > > So I think we want a different fix. > > > > > Maybe something like: > > --- a/libstdc++-v3/src/filesystem/dir-common.h > +++ b/libstdc++-v3/src/filesystem/dir-common.h > @@ -70,6 +70,8 @@ struct DIR { }; > inline DIR* opendir(const char*) { return nullptr; } > inline dirent* readdir(DIR*) { return nullptr; } > inline int closedir(DIR*) { return -1; } > +#undef _GLIBCXX_HAVE_DIRFD > +#undef _GLIBCXX_HAVE_UNLINKAT > #endif > } // namespace __gnu_posix Yes, this fixes the PRU target, and does not regress x86_64-pc-linux-gnu. Regards, Dimitar
On Tue, 8 Feb 2022 at 21:02, Dimitar Dimitrov <dimitar@dinux.eu> wrote: > On Mon, Feb 07, 2022 at 09:05:45PM +0000, Jonathan Wakely wrote: > > On Mon, 7 Feb 2022 at 21:01, Jonathan Wakely <jwakely.gcc@gmail.com> > wrote: > > > > > > > > > > > On Mon, 7 Feb 2022 at 20:12, Dimitar Dimitrov <dimitar@dinux.eu> > wrote: > > > > > >> On PRU target with newlib, we have the following combination in > config.h: > > >> /* #undef HAVE_DIRENT_H */ > > >> #define HAVE_FCNTL_H 1 > > >> #define HAVE_UNLINKAT 1 > > >> > > >> In newlib, targets which do not define dirent.h, get a build error > when > > >> including <dirent.h>: > > >> > > >> > https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/sys/dirent.h;hb=HEAD > > >> > > >> While fs_dir.cc correctly checks for HAVE_FCNTL_H, dir-common.h > doesn't, > > >> and instead uses HAVE_DIRENT_H. This results in unlinkat() function > call > > >> in fs_dir.cc without the needed <fcntl.h> include in dir-common.h. > Thus > > >> a build failure: > > >> .../gcc/libstdc++-v3/src/c++17/fs_dir.cc:151:11: error: ‘::unlinkat’ > > >> has not been declared; did you mean ‘unlink’? > > >> > > >> Fix by encapsulating <fcntl.h> include with the correct check. > > >> > > > > > > But there's no point doing anything in that file if we don't have > > > <dirent.h>, the whole thing is unusable. There's no point making the > > > members using unlinkat compile if you can't ever construct the type. > > > > > > So I think we want a different fix. > > > > > > > > > Maybe something like: > > > > --- a/libstdc++-v3/src/filesystem/dir-common.h > > +++ b/libstdc++-v3/src/filesystem/dir-common.h > > @@ -70,6 +70,8 @@ struct DIR { }; > > inline DIR* opendir(const char*) { return nullptr; } > > inline dirent* readdir(DIR*) { return nullptr; } > > inline int closedir(DIR*) { return -1; } > > +#undef _GLIBCXX_HAVE_DIRFD > > +#undef _GLIBCXX_HAVE_UNLINKAT > > #endif > > } // namespace __gnu_posix > Yes, this fixes the PRU target, and does not regress > x86_64-pc-linux-gnu. > Thanks for checking it. I'm just testing it myself on powerpc64le-linux-gnu and will push when it finishes.
diff --git a/libstdc++-v3/src/filesystem/dir-common.h b/libstdc++-v3/src/filesystem/dir-common.h index 0b7665a3f70..cfce4fae9a4 100644 --- a/libstdc++-v3/src/filesystem/dir-common.h +++ b/libstdc++-v3/src/filesystem/dir-common.h @@ -35,10 +35,11 @@ # include <sys/types.h> # endif # include <dirent.h> // opendir, readdir, fdopendir, dirfd -# ifdef _GLIBCXX_HAVE_FCNTL_H -# include <fcntl.h> // open, openat, fcntl, AT_FDCWD, O_NOFOLLOW etc. -# include <unistd.h> // close, unlinkat -# endif +#endif + +#ifdef _GLIBCXX_HAVE_FCNTL_H +# include <fcntl.h> // open, openat, fcntl, AT_FDCWD, O_NOFOLLOW etc. +# include <unistd.h> // close, unlinkat #endif namespace std _GLIBCXX_VISIBILITY(default)
On PRU target with newlib, we have the following combination in config.h: /* #undef HAVE_DIRENT_H */ #define HAVE_FCNTL_H 1 #define HAVE_UNLINKAT 1 In newlib, targets which do not define dirent.h, get a build error when including <dirent.h>: https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/sys/dirent.h;hb=HEAD While fs_dir.cc correctly checks for HAVE_FCNTL_H, dir-common.h doesn't, and instead uses HAVE_DIRENT_H. This results in unlinkat() function call in fs_dir.cc without the needed <fcntl.h> include in dir-common.h. Thus a build failure: .../gcc/libstdc++-v3/src/c++17/fs_dir.cc:151:11: error: ‘::unlinkat’ has not been declared; did you mean ‘unlink’? Fix by encapsulating <fcntl.h> include with the correct check. Regtested x86_64-pc-linux-gnu and no new failures detected. Fixes commit r12-7062-gebf61754647689 (libstdc++: Fix filesystem::remove_all races). libstdc++-v3/ChangeLog: * src/filesystem/dir-common.h (_GLIBCXX_HAVE_FCNTL_H): Move the check outside the HAVE_DIRENT_H check. Signed-off-by: Dimitar Dimitrov <dimitar@dinux.eu> --- libstdc++-v3/src/filesystem/dir-common.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)