[stage1][PATCH] Provide warning for missing jobserver.

[stage1][PATCH] Provide warning for missing jobserver.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]

Hi.

I'm suggesting to provide a warning when one uses -flto=jobserver
but we can't detect job server for some reason.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed in next stage1?
Thanks,
Martin

gcc/ChangeLog:

2020-03-26  Martin Liska  <mliska@suse.cz>

	PR driver/94330
	* lto-wrapper.c (run_gcc): When using -flto=jobserver,
	report warning when the jobserver is not detected.
---
  gcc/lto-wrapper.c | 12 +++++++++---
  1 file changed, 9 insertions(+), 3 deletions(-)



[-- Attachment #2: 0001-Provide-warning-for-missing-jobserver.patch --]
[-- Type: text/x-patch, Size: 628 bytes --]

diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
index 46a88b233f6..6263c164888 100644
--- a/gcc/lto-wrapper.c
+++ b/gcc/lto-wrapper.c
@@ -1473,10 +1473,16 @@ run_gcc (unsigned argc, char *argv[])
       auto_parallel = 0;
       parallel = 0;
     }
-  else if (!jobserver && jobserver_active_p ())
+  else
     {
-      parallel = 1;
-      jobserver = 1;
+      bool active_jobserver = jobserver_active_p ();
+      if (jobserver && !active_jobserver)
+	warning (0, "jobserver is not available.");
+      else if (!jobserver && active_jobserver)
+	{
+	  parallel = 1;
+	  jobserver = 1;
+	}
     }
 
   if (linker_output)

Re: [stage1][PATCH] Provide warning for missing jobserver.

[Jan Hubicka]

> Hi.
> 
> I'm suggesting to provide a warning when one uses -flto=jobserver
> but we can't detect job server for some reason.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed in next stage1?
> Thanks,
> Martin
> 
> gcc/ChangeLog:
> 
> 2020-03-26  Martin Liska  <mliska@suse.cz>
> 
> 	PR driver/94330
> 	* lto-wrapper.c (run_gcc): When using -flto=jobserver,
> 	report warning when the jobserver is not detected.
This looks like a good idea to me (though I guess Richi needs to approve
it).  I would make message more informative so it is clear that
jobserver is supposed to be provided by GNU make and that we resort
to running in one thread.

Morivation is that prople won't get confused trying to start GCC from
other kind of build systems and also when you cut&paste the command out
the warning should be explicit enough to make you notice that you can
wait for very long time ;)

Honza
> ---
>  gcc/lto-wrapper.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> 

> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> index 46a88b233f6..6263c164888 100644
> --- a/gcc/lto-wrapper.c
> +++ b/gcc/lto-wrapper.c
> @@ -1473,10 +1473,16 @@ run_gcc (unsigned argc, char *argv[])
>        auto_parallel = 0;
>        parallel = 0;
>      }
> -  else if (!jobserver && jobserver_active_p ())
> +  else
>      {
> -      parallel = 1;
> -      jobserver = 1;
> +      bool active_jobserver = jobserver_active_p ();
> +      if (jobserver && !active_jobserver)
> +	warning (0, "jobserver is not available.");
> +      else if (!jobserver && active_jobserver)
> +	{
> +	  parallel = 1;
> +	  jobserver = 1;
> +	}
>      }
>  
>    if (linker_output)
> 

Re: [stage1][PATCH] Provide warning for missing jobserver.

[Martin Liška]

On 3/26/20 1:42 PM, Jan Hubicka wrote:
>> Hi.
>>
>> I'm suggesting to provide a warning when one uses -flto=jobserver
>> but we can't detect job server for some reason.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed in next stage1?
>> Thanks,
>> Martin
>>
>> gcc/ChangeLog:
>>
>> 2020-03-26  Martin Liska  <mliska@suse.cz>
>>
>> 	PR driver/94330
>> 	* lto-wrapper.c (run_gcc): When using -flto=jobserver,
>> 	report warning when the jobserver is not detected.
> This looks like a good idea to me (though I guess Richi needs to approve
> it).  I would make message more informative so it is clear that
> jobserver is supposed to be provided by GNU make and that we resort
> to running in one thread.

Feel free to provide a better wording.

> 
> Morivation is that prople won't get confused trying to start GCC from
> other kind of build systems and also when you cut&paste the command out
> the warning should be explicit enough to make you notice that you can
> wait for very long time ;)

Yes, it will be handy for these situations. If possible, I would like to see
it landing in GCC 10. But it's Richi's turn.

Martin

> 
> Honza
>> ---
>>   gcc/lto-wrapper.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>>
> 
>> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
>> index 46a88b233f6..6263c164888 100644
>> --- a/gcc/lto-wrapper.c
>> +++ b/gcc/lto-wrapper.c
>> @@ -1473,10 +1473,16 @@ run_gcc (unsigned argc, char *argv[])
>>         auto_parallel = 0;
>>         parallel = 0;
>>       }
>> -  else if (!jobserver && jobserver_active_p ())
>> +  else
>>       {
>> -      parallel = 1;
>> -      jobserver = 1;
>> +      bool active_jobserver = jobserver_active_p ();
>> +      if (jobserver && !active_jobserver)
>> +	warning (0, "jobserver is not available.");
>> +      else if (!jobserver && active_jobserver)
>> +	{
>> +	  parallel = 1;
>> +	  jobserver = 1;
>> +	}
>>       }
>>   
>>     if (linker_output)
>>
> 

Re: [stage1][PATCH] Provide warning for missing jobserver.

[Martin Sebor]

On 3/26/20 3:40 AM, Martin Liška wrote:
> Hi.
> 
> I'm suggesting to provide a warning when one uses -flto=jobserver
> but we can't detect job server for some reason.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed in next stage1?
> Thanks,
> Martin
> 
> gcc/ChangeLog:
> 
> 2020-03-26  Martin Liska  <mliska@suse.cz>
> 
>      PR driver/94330
>      * lto-wrapper.c (run_gcc): When using -flto=jobserver,
>      report warning when the jobserver is not detected.

Sounds like a useful warning.  It would be even better if it said why.
jobserver_active_p() returns false under one of at least three distinct
conditions.  If the function provided a status string in addition to
the boolean result, the string could be included in the text of
the warning.

As an aside, I'd expect -Wformat-diag to complain about the trailing
period:

+	warning (0, "jobserver is not available.");

Martin

Re: [stage1][PATCH] Provide warning for missing jobserver.

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

On 3/29/20 12:51 AM, Martin Sebor wrote:
> On 3/26/20 3:40 AM, Martin Liška wrote:
>> Hi.
>>
>> I'm suggesting to provide a warning when one uses -flto=jobserver
>> but we can't detect job server for some reason.
>>
>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>>
>> Ready to be installed in next stage1?
>> Thanks,
>> Martin
>>
>> gcc/ChangeLog:
>>
>> 2020-03-26  Martin Liska  <mliska@suse.cz>
>>
>>      PR driver/94330
>>      * lto-wrapper.c (run_gcc): When using -flto=jobserver,
>>      report warning when the jobserver is not detected.
> 
> Sounds like a useful warning.  It would be even better if it said why.
> jobserver_active_p() returns false under one of at least three distinct
> conditions.  If the function provided a status string in addition to
> the boolean result, the string could be included in the text of
> the warning.
> 
> As an aside, I'd expect -Wformat-diag to complain about the trailing
> period:
> 
> +    warning (0, "jobserver is not available.");
> 
> Martin

Hello.

I like the suggested improvement, there's updated patch that I've just
tested.

Martin

[-- Attachment #2: 0001-Provide-warning-for-missing-jobserver.patch --]
[-- Type: text/x-patch, Size: 2535 bytes --]

From 2efd69ac2282ba42755826d265bae09900e9ce0c Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Thu, 26 Mar 2020 09:57:35 +0100
Subject: [PATCH] Provide warning for missing jobserver.

gcc/ChangeLog:

2020-03-26  Martin Liska  <mliska@suse.cz>

	PR driver/94330
	* lto-wrapper.c (run_gcc): When using -flto=jobserver,
	report warning when the jobserver is not detected.
---
 gcc/lto-wrapper.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
index 46a88b233f6..19d0c224dad 100644
--- a/gcc/lto-wrapper.c
+++ b/gcc/lto-wrapper.c
@@ -1236,28 +1236,33 @@ init_num_threads (void)
 
 /* FIXME: once using -std=c++11, we can use std::thread::hardware_concurrency.  */
 
-/* Return true when a jobserver is running and can accept a job.  */
+/* Test and return reason why a jobserver cannot be detected.  */
 
-static bool
+static const char *
 jobserver_active_p (void)
 {
+  #define JS_PREFIX "jobserver is not available: "
+  #define JS_NEEDLE "--jobserver-auth="
+
   const char *makeflags = getenv ("MAKEFLAGS");
   if (makeflags == NULL)
-    return false;
+    return JS_PREFIX "%<MAKEFLAGS%> environment variable is unset";
 
-  const char *needle = "--jobserver-auth=";
-  const char *n = strstr (makeflags, needle);
+  const char *n = strstr (makeflags, JS_NEEDLE);
   if (n == NULL)
-    return false;
+    return JS_PREFIX "%<" JS_NEEDLE "%> is not present in %<MAKEFLAGS%>";
 
   int rfd = -1;
   int wfd = -1;
 
-  return (sscanf (n + strlen (needle), "%d,%d", &rfd, &wfd) == 2
-	  && rfd > 0
-	  && wfd > 0
-	  && is_valid_fd (rfd)
-	  && is_valid_fd (wfd));
+  if (sscanf (n + strlen (JS_NEEDLE), "%d,%d", &rfd, &wfd) == 2
+      && rfd > 0
+      && wfd > 0
+      && is_valid_fd (rfd)
+      && is_valid_fd (wfd))
+    return NULL;
+  else
+    return JS_PREFIX "cannot access %<" JS_NEEDLE "%> file descriptors";
 }
 
 /* Execute gcc. ARGC is the number of arguments. ARGV contains the arguments. */
@@ -1473,10 +1478,16 @@ run_gcc (unsigned argc, char *argv[])
       auto_parallel = 0;
       parallel = 0;
     }
-  else if (!jobserver && jobserver_active_p ())
+  else
     {
-      parallel = 1;
-      jobserver = 1;
+      const char *jobserver_error = jobserver_active_p ();
+      if (jobserver && jobserver_error != NULL)
+	warning (0, jobserver_error);
+      else if (!jobserver && jobserver_error == NULL)
+	{
+	  parallel = 1;
+	  jobserver = 1;
+	}
     }
 
   if (linker_output)
-- 
2.26.0