| Message ID | 03e5f4cf-7955-76fc-f94e-11695857f0f1@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Fix file descriptor existence of MinGW. | expand |
On 8/6/19 6:04 AM, Martin Liška wrote: > Hi. > > The patch is about proper checking of file descriptors on Windows. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Is there a way to share the definition of the new function so it doesn't have to be duplicated? Other than that, I'm wondering if the guard for F_GETFD is necessary and when (the original code didn't guard its use). Martin > > @Pekka: Can you please test it on Windows? > > Ready to be installed? > Thanks, > Martin > > gcc/ChangeLog: > > 2019-08-06 Martin Liska <mliska@suse.cz> > > PR bootstrap/91352 > * gcc.c (fd_exists): New. > (driver::detect_jobserver): Use fd_exists. > * lto-wrapper.c (fd_exists): New. > (jobserver_active_p): Use fd_exists. > --- > gcc/gcc.c | 19 +++++++++++++++++-- > gcc/lto-wrapper.c | 19 +++++++++++++++++-- > 2 files changed, 34 insertions(+), 4 deletions(-) > >
On 8/6/19 5:35 PM, Martin Sebor wrote: > On 8/6/19 6:04 AM, Martin Liška wrote: >> Hi. >> >> The patch is about proper checking of file descriptors on Windows. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Is there a way to share the definition of the new function so > it doesn't have to be duplicated? I would like to, but I'm wondering which header and source file should I use for it? > > Other than that, I'm wondering if the guard for F_GETFD is > necessary and when (the original code didn't guard its use). Because the value is not defined on MinGW targets where _get_osfhandle call must be used ;) Martin > > Martin > > >> >> @Pekka: Can you please test it on Windows? >> >> Ready to be installed? >> Thanks, >> Martin >> >> gcc/ChangeLog: >> >> 2019-08-06 Martin Liska <mliska@suse.cz> >> >> PR bootstrap/91352 >> * gcc.c (fd_exists): New. >> (driver::detect_jobserver): Use fd_exists. >> * lto-wrapper.c (fd_exists): New. >> (jobserver_active_p): Use fd_exists. >> --- >> gcc/gcc.c | 19 +++++++++++++++++-- >> gcc/lto-wrapper.c | 19 +++++++++++++++++-- >> 2 files changed, 34 insertions(+), 4 deletions(-) >> >> >
On 8/6/19 9:55 AM, Martin Liška wrote: > On 8/6/19 5:35 PM, Martin Sebor wrote: >> On 8/6/19 6:04 AM, Martin Liška wrote: >>> Hi. >>> >>> The patch is about proper checking of file descriptors on Windows. >>> >>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Is there a way to share the definition of the new function so >> it doesn't have to be duplicated? > > I would like to, but I'm wondering which header and source file > should I use for it? I'm not sure. It's a general purpose function that could go in some utility library like libiberty but I don't know what the process for enhancing it is to tell if that's appropriate. >> Other than that, I'm wondering if the guard for F_GETFD is >> necessary and when (the original code didn't guard its use). > > Because the value is not defined on MinGW targets where _get_osfhandle > call must be used ;) I was unclear -- sorry. What I meant was: since there is a check for MinGW, is the check for F_GETFD necessary, or would this be sufficient and preferable? #if defined(_WIN32) return _get_osfhandle (fd) != -1; else return fcntl (fd, F_GETFD) >= 0; #endif That way it's clear that only two possibilities are expected. Martin > > Martin > >> >> Martin >> >> >>> >>> @Pekka: Can you please test it on Windows? >>> >>> Ready to be installed? >>> Thanks, >>> Martin >>> >>> gcc/ChangeLog: >>> >>> 2019-08-06 Martin Liska <mliska@suse.cz> >>> >>> PR bootstrap/91352 >>> * gcc.c (fd_exists): New. >>> (driver::detect_jobserver): Use fd_exists. >>> * lto-wrapper.c (fd_exists): New. >>> (jobserver_active_p): Use fd_exists. >>> --- >>> gcc/gcc.c | 19 +++++++++++++++++-- >>> gcc/lto-wrapper.c | 19 +++++++++++++++++-- >>> 2 files changed, 34 insertions(+), 4 deletions(-) >>> >>> >> >
On 8/6/19 6:18 PM, Martin Sebor wrote: > On 8/6/19 9:55 AM, Martin Liška wrote: >> On 8/6/19 5:35 PM, Martin Sebor wrote: >>> On 8/6/19 6:04 AM, Martin Liška wrote: >>>> Hi. >>>> >>>> The patch is about proper checking of file descriptors on Windows. >>>> >>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >>> >>> Is there a way to share the definition of the new function so >>> it doesn't have to be duplicated? >> >> I would like to, but I'm wondering which header and source file >> should I use for it? > > I'm not sure. It's a general purpose function that could go > in some utility library like libiberty but I don't know what > the process for enhancing it is to tell if that's appropriate. Yep, I like the suggested approach. I've done that in the updated patch. > >>> Other than that, I'm wondering if the guard for F_GETFD is >>> necessary and when (the original code didn't guard its use). >> >> Because the value is not defined on MinGW targets where _get_osfhandle >> call must be used ;) > > I was unclear -- sorry. What I meant was: since there is > a check for MinGW, is the check for F_GETFD necessary, or > would this be sufficient and preferable? > > #if defined(_WIN32) > return _get_osfhandle (fd) != -1; > else > return fcntl (fd, F_GETFD) >= 0; > #endif > > That way it's clear that only two possibilities are expected. Ah yes, included. Martin > > Martin > >> >> Martin >> >>> >>> Martin >>> >>> >>>> >>>> @Pekka: Can you please test it on Windows? >>>> >>>> Ready to be installed? >>>> Thanks, >>>> Martin >>>> >>>> gcc/ChangeLog: >>>> >>>> 2019-08-06 Martin Liska <mliska@suse.cz> >>>> >>>> PR bootstrap/91352 >>>> * gcc.c (fd_exists): New. >>>> (driver::detect_jobserver): Use fd_exists. >>>> * lto-wrapper.c (fd_exists): New. >>>> (jobserver_active_p): Use fd_exists. >>>> --- >>>> gcc/gcc.c | 19 +++++++++++++++++-- >>>> gcc/lto-wrapper.c | 19 +++++++++++++++++-- >>>> 2 files changed, 34 insertions(+), 4 deletions(-) >>>> >>>> >>> >> >
On Wed, Aug 07, 2019 at 10:45:08AM +0200, Martin Liška wrote: > @@ -155,3 +156,16 @@ lrealpath (const char *filename) > /* This system is a lost cause, just duplicate the filename. */ > return strdup (filename); > } > + > +/* Return true when FD file descriptor exists. */ > + > +int > +fd_exists (int fd) > +{ > +#if defined(_WIN32) > + HANDLE h = (HANDLE) _get_osfhandle (fd); > + return h != (HANDLE) -1; > +#else > + return fcntl (fd, F_GETFD) >= 0; > +#endif > +} Judging from gnulib http://erislabs.net/gitweb/?p=gnulib.git;a=commitdiff_plain;h=021c8619190757f535c72ad5cdb1d624e19620d6 the #if condition for Windows should be probably different and it would be worth to have a fallback for when F_GETFD isn't defined e.g. through dup2 (fd, fd) < 0. And for the libiberty changes you want Ian to review them. Jakub
On 8/7/19 10:56 AM, Jakub Jelinek wrote: > On Wed, Aug 07, 2019 at 10:45:08AM +0200, Martin Liška wrote: >> @@ -155,3 +156,16 @@ lrealpath (const char *filename) >> /* This system is a lost cause, just duplicate the filename. */ >> return strdup (filename); >> } >> + >> +/* Return true when FD file descriptor exists. */ >> + >> +int >> +fd_exists (int fd) >> +{ >> +#if defined(_WIN32) >> + HANDLE h = (HANDLE) _get_osfhandle (fd); >> + return h != (HANDLE) -1; >> +#else >> + return fcntl (fd, F_GETFD) >= 0; >> +#endif >> +} > > Judging from gnulib > http://erislabs.net/gitweb/?p=gnulib.git;a=commitdiff_plain;h=021c8619190757f535c72ad5cdb1d624e19620d6 > the #if condition for Windows should be probably different > and it would be worth to have a fallback for when F_GETFD isn't defined > e.g. through dup2 (fd, fd) < 0. Thank you for the review. I updated the patch accordingly. Martin > > And for the libiberty changes you want Ian to review them. > > Jakub >
Hi. There's one enhanced version where I added HAVE_FCNTL_H. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin
On 7.8.2019 15:09, Martin Liška wrote: > Hi. > > There's one enhanced version where I added HAVE_FCNTL_H. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? > Thanks, > Martin Hi, tested this one against the latest trunk and it seems to be working -- as in it's possible to bootstrap a working (cross) compiler that is hosted on mingw-w64. -- Pekka
On Wed, Aug 7, 2019 at 5:09 AM Martin Liška <mliska@suse.cz> wrote: > > There's one enhanced version where I added HAVE_FCNTL_H. > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > Ready to be installed? I think a better name than fd_exists would be is_valid_fd. If that's OK with you, the libiberty part of this patch seems fine with that change. Thanks. (Although I'm not quite sure what problem this is trying to solve. What goes wrong if the driver just doesn't bother to check whether the file descriptors listed in --jobserver-auth are valid? Why would that ever happen, and why would it matter if it did? The ChangeLog talks about the linker using the same file descriptors, but in that case won't this test see the descriptors as valid?) Ian
On 8/7/19 4:35 PM, Ian Lance Taylor wrote: > On Wed, Aug 7, 2019 at 5:09 AM Martin Liška <mliska@suse.cz> wrote: >> >> There's one enhanced version where I added HAVE_FCNTL_H. >> >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. >> >> Ready to be installed? > > I think a better name than fd_exists would be is_valid_fd. If that's > OK with you, the libiberty part of this patch seems fine with that > change. Thanks. Thank you, I installed the patch with changed name. > > (Although I'm not quite sure what problem this is trying to solve. > What goes wrong if the driver just doesn't bother to check whether the > file descriptors listed in --jobserver-auth are valid? Because make provides a silly jobserver API. When you forget to add '+' to your make target, then make will pass --jobserver-auth=x,y options, but the file descriptors are not opened. And so that one can't use job server. That's why we do the dance. Martin > Why would that > ever happen, and why would it matter if it did? The ChangeLog talks > about the linker using the same file descriptors, but in that case > won't this test see the descriptors as valid?) > > Ian >
diff --git a/gcc/gcc.c b/gcc/gcc.c index 18a07426290..6bcd989ee30 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -8359,6 +8359,21 @@ driver::final_actions () const } } +/* Return true when FD file descriptor exists. */ + +static bool +fd_exists (int fd) +{ +#if defined (F_GETFD) + return fcntl (fd, F_GETFD) >= 0; +#elif defined(_WIN32) + HANDLE h = (HANDLE) _get_osfhandle (fd); + return h != (HANDLE) -1; +#else + return false; +#endif +} + /* Detect whether jobserver is active and working. If not drop --jobserver-auth from MAKEFLAGS. */ @@ -8380,8 +8395,8 @@ driver::detect_jobserver () const = (sscanf (n + strlen (needle), "%d,%d", &rfd, &wfd) == 2 && rfd > 0 && wfd > 0 - && fcntl (rfd, F_GETFD) >= 0 - && fcntl (wfd, F_GETFD) >= 0); + && fd_exists (rfd) + && fd_exists (wfd)); /* Drop the jobserver if it's not working now. */ if (!jobserver) diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c index 3414adedd26..065a1d0ad02 100644 --- a/gcc/lto-wrapper.c +++ b/gcc/lto-wrapper.c @@ -1217,6 +1217,21 @@ init_num_threads (void) /* FIXME: once using -std=c11, we can use std::thread::hardware_concurrency. */ +/* Return true when FD file descriptor exists. */ + +static bool +fd_exists (int fd) +{ +#if defined (F_GETFD) + return fcntl (fd, F_GETFD) >= 0; +#elif defined(_WIN32) + HANDLE h = (HANDLE) _get_osfhandle (fd); + return h != (HANDLE) -1; +#else + return false; +#endif +} + /* Return true when a jobserver is running and can accept a job. */ static bool @@ -1237,8 +1252,8 @@ jobserver_active_p (void) return (sscanf (n + strlen (needle), "%d,%d", &rfd, &wfd) == 2 && rfd > 0 && wfd > 0 - && fcntl (rfd, F_GETFD) >= 0 - && fcntl (wfd, F_GETFD) >= 0); + && fd_exists (rfd) + && fd_exists (wfd)); } /* Execute gcc. ARGC is the number of arguments. ARGV contains the arguments. */