Re: [PATCH] Properly detect working jobserver in gcc driver.

[Jan Hubicka <hubicka@ucw.cz>]

> On Fri, Aug 2, 2019 at 10:50 AM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Fri, Aug 02, 2019 at 10:47:10AM +0200, Martin Liška wrote:
> > > > Can you strace if other fds are opened and not closed in the spot you had it
> > > > before?  Advantage of doing it there is that it will not be done for all the
> > > > -E/-S/-c compilations when the linker is not spawned.
> > >
> > > I've used the same trick which you used and I'm attaching the output.
> > > I believe it's fine, I can't see any opened fd by GCC.
> >
> > LGTM.
> 
> Btw, we discussed yesterday on the phone and the conclusion was to
> make -flto auto-detect a job-server (but not fall back to # of threads)
> and add -flto=auto to auto-detect a job-server and fall back to # of threads.
> That basically makes -flto=jobserver the default behavior which means
> we should document -flto=1 as a way to override jobserver detection.
> 
> We also discussed carrying distribution-local patches to make GNU make
> expose the jobserver FD to all jobs, not just those marked to make the
> new default effective.  Does anybody have an idea if there's another
> common enough make that would benefit from support?  That is,
> do other "jobserver" implementations/APIs exist we could support
> or is another make used in more than 1% of build systems?

Ideal solution would be to provide a simple jobserver library for
multithreaded tools to use (GCC is not the only one - llvm, compression
programs etc) and patch make and its replacements to use it.
I think Ninja is used by some larger projects now.

Honza
> 
> Richard.
> 
> > > gcc/ChangeLog:
> > >
> > > 2019-08-02  Martin Liska  <mliska@suse.cz>
> > >
> > >       * gcc.c (driver::maybe_run_linker): Call detect_jobserver
> > >       to detect working job server.
> > >       (driver::detect_jobserver): Test whether jobserver
> > >       is active from GCC driver. That will prevent situation where
> > >       GCC is invoked from a LD plugin and the linker already uses
> > >       file descriptors suggested by make.  That leads to a wrong
> > >       detection.
> > >       * gcc.h (driver): Add detect_jobserver.
> > >       * lto-wrapper.c (jobserver_active_p): Simplify sscanf by
> > >       not scanning for --jobserver-auth prefix.
> > > ---
> > >  gcc/gcc.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  gcc/gcc.h         |  1 +
> > >  gcc/lto-wrapper.c |  2 +-
> > >  3 files changed, 44 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/gcc/gcc.c b/gcc/gcc.c
> > > index a4323eb146e..18a07426290 100644
> > > --- a/gcc/gcc.c
> > > +++ b/gcc/gcc.c
> > > @@ -8268,6 +8268,8 @@ driver::maybe_run_linker (const char *argv0) const
> > >      {
> > >        int tmp = execution_count;
> > >
> > > +      detect_jobserver ();
> > > +
> > >        if (! have_c)
> > >       {
> > >  #if HAVE_LTO_PLUGIN > 0
> > > @@ -8357,6 +8359,46 @@ driver::final_actions () const
> > >      }
> > >  }
> > >
> > > +/* Detect whether jobserver is active and working.  If not drop
> > > +   --jobserver-auth from MAKEFLAGS.  */
> > > +
> > > +void
> > > +driver::detect_jobserver () const
> > > +{
> > > +  /* Detect jobserver and drop it if it's not working.  */
> > > +  const char *makeflags = env.get ("MAKEFLAGS");
> > > +  if (makeflags != NULL)
> > > +    {
> > > +      const char *needle = "--jobserver-auth=";
> > > +      const char *n = strstr (makeflags, needle);
> > > +      if (n != NULL)
> > > +     {
> > > +       int rfd = -1;
> > > +       int wfd = -1;
> > > +
> > > +       bool jobserver
> > > +         = (sscanf (n + strlen (needle), "%d,%d", &rfd, &wfd) == 2
> > > +            && rfd > 0
> > > +            && wfd > 0
> > > +            && fcntl (rfd, F_GETFD) >= 0
> > > +            && fcntl (wfd, F_GETFD) >= 0);
> > > +
> > > +       /* Drop the jobserver if it's not working now.  */
> > > +       if (!jobserver)
> > > +         {
> > > +           unsigned offset = n - makeflags;
> > > +           char *dup = xstrdup (makeflags);
> > > +           dup[offset] = '\0';
> > > +
> > > +           const char *space = strchr (makeflags + offset, ' ');
> > > +           if (space != NULL)
> > > +             strcpy (dup + offset, space);
> > > +           xputenv (concat ("MAKEFLAGS=", dup, NULL));
> > > +         }
> > > +     }
> > > +    }
> > > +}
> > > +
> > >  /* Determine what the exit code of the driver should be.  */
> > >
> > >  int
> > > diff --git a/gcc/gcc.h b/gcc/gcc.h
> > > index a0a1d94c6e6..dc77dba67fb 100644
> > > --- a/gcc/gcc.h
> > > +++ b/gcc/gcc.h
> > > @@ -51,6 +51,7 @@ class driver
> > >    void do_spec_on_infiles () const;
> > >    void maybe_run_linker (const char *argv0) const;
> > >    void final_actions () const;
> > > +  void detect_jobserver () const;
> > >    int get_exit_code () const;
> > >
> > >   private:
> > > diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> > > index 353187c6043..3414adedd26 100644
> > > --- a/gcc/lto-wrapper.c
> > > +++ b/gcc/lto-wrapper.c
> > > @@ -1234,7 +1234,7 @@ jobserver_active_p (void)
> > >    int rfd = -1;
> > >    int wfd = -1;
> > >
> > > -  return ((sscanf(n, "--jobserver-auth=%d,%d", &rfd, &wfd) == 2)
> > > +  return (sscanf (n + strlen (needle), "%d,%d", &rfd, &wfd) == 2
> > >         && rfd > 0
> > >         && wfd > 0
> > >         && fcntl (rfd, F_GETFD) >= 0
> > > --
> > > 2.22.0
> > >
> >
> >
> >         Jakub