[DRIVER PATCH]: Nadger subprocess argv[0]

[DRIVER PATCH]: Nadger subprocess argv[0]

[Nathan Sidwell]

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

This patch nadgers the driver's subprocess names to include the driver 
name. It results in more informative error messages.  For instance, 
rather than:

   >./xg++ -B./ frob.cc -c -fdump-tree-nope
   cc1plus: error: unrecognized command line option '-fdump-tree-nope'

we get:

   >./xg++ -B./ frob.cc -c -fdump-tree-nope
   xg++(cc1plus): error: unrecognized command line option '-fdump-tree-nope'

Thereby cluing the user into this being a compiler error.  (When this 
error is buried inside a build log, the poor user can be more confused 
as to what this cc1plus thing might be).

I achieve this by taking advantage of the subprocess spawning taking 
separate arguments for the program to exec, and the argv[0] value passed 
to that program.  Because subprocesses can use argv[0] to locate 
themselves I propagate the directory name, so that remains as before.

When using valgrind, this substitution is not performed, because 
valgrind relies on argv[0] being the program pathname.

The -v output is unaffected, as that is emitted before altering argv[0]. 
  argv[0] is also restored after spawning.

This then allows us to elide process_command's check of whether the 
input file exists.  That's optimizing for failure, but I suspect is 
desired merely to avoid an error of the form:
   cc1plus: fatal error: frob.cc: No such file or directory

As you can see process_command does some special handling for lto & 
@files, which is kind of icky and now goes away.  We get:
   xg++(cc1plus): fatal error: frob.cc: No such file or directory

Thoughts? Ok? Stupid idea?

nathan

-- 
Nathan Sidwell

[-- Attachment #2: exec-name.diff --]
[-- Type: text/x-patch, Size: 4766 bytes --]

2019-05-14  Nathan Sidwell  <nathan@acm.org>

	* gcc.c (execute): Splice current executable name into spawned
	argv[0] for better subprocess diagnostics.
	(process_command): Do not check if input file exists.

	* lib/prune.exp (prune_gcc_output): Adjust collect regexps.

Index: gcc/gcc.c
===================================================================
--- gcc/gcc.c	(revision 271131)
+++ gcc/gcc.c	(working copy)
@@ -3206,6 +3206,48 @@ execute (void)
       int err;
       const char *string = commands[i].argv[0];
 
+      if (commands[i].argv == argbuf.address ())
+	{
+	  /* Munge argv[0], the one given in the exec'd command's
+	     argv, to include information about from whence it was
+	     spawned.  We preserve the directory component so the
+	     command can determine where it is, but not what it was
+	     called.  Thus its otherwise-unlocated errors specify
+	     something like 'g++(cc1plus)' rather than plain
+	     'cc1plus'.  Only do this if argv is the argbuf address,
+	     so as to not interfere with valgrind insertion.  */
+	  size_t slen = strlen (string);
+	  size_t plen = strlen (progname);
+	  size_t tlen = strlen (commands[i].prog);
+
+	  /* Remove the filename component from the path.  */
+	  while (slen && !IS_DIR_SEPARATOR (string[slen-1]))
+	    slen--;
+	  char *ren = XNEWVEC (char, slen + plen + tlen + 3);
+	  size_t off = 0;
+
+	  /* Copy directory, including final dir separator.  */
+	  memcpy (ren + off, string, slen);
+	  off += slen;
+	  /* Append our progname.  */
+	  memcpy (ren + off, progname, plen);
+	  off += plen;
+
+	  ren[off++] = '(';
+
+	  /* Append the plain progname (lacking any os-specific
+	     suffix).  */
+	  memcpy (ren + off, commands[i].prog, tlen);
+	  off += tlen;
+
+	  ren[off++] = ')';
+
+	  /* And a NUL.  */
+	  ren[off++] = 0;
+
+	  commands[i].argv[0] = ren;
+	}
+
       errmsg = pex_run (pex,
 			((i + 1 == n_commands ? PEX_LAST : 0)
 			 | (string == commands[i].prog ? PEX_SEARCH : 0)),
@@ -3220,6 +3262,12 @@ execute (void)
 		       string, errmsg);
 	}
 
+      if (commands[i].argv[0] != string)
+	{
+	  free (const_cast <char *> (commands[i].argv[0]));
+	  commands[i].argv[0] = string;
+	}
+
       if (i && string != commands[i].prog)
 	free (CONST_CAST (char *, string));
     }
@@ -4561,44 +4609,11 @@ process_command (unsigned int decoded_op
       if (decoded_options[j].opt_index == OPT_SPECIAL_input_file)
 	{
 	  const char *arg = decoded_options[j].arg;
-          const char *p = strrchr (arg, '@');
-          char *fname;
-	  long offset;
-	  int consumed;
 #ifdef HAVE_TARGET_OBJECT_SUFFIX
 	  arg = convert_filename (arg, 0, access (arg, F_OK));
 #endif
-	  /* For LTO static archive support we handle input file
-	     specifications that are composed of a filename and
-	     an offset like FNAME@OFFSET.  */
-	  if (p
-	      && p != arg
-	      && sscanf (p, "@%li%n", &offset, &consumed) >= 1
-	      && strlen (p) == (unsigned int)consumed)
-	    {
-              fname = (char *)xmalloc (p - arg + 1);
-              memcpy (fname, arg, p - arg);
-              fname[p - arg] = '\0';
-	      /* Only accept non-stdin and existing FNAME parts, otherwise
-		 try with the full name.  */
-	      if (strcmp (fname, "-") == 0 || access (fname, F_OK) < 0)
-		{
-		  free (fname);
-		  fname = xstrdup (arg);
-		}
-	    }
-	  else
-	    fname = xstrdup (arg);
-
-          if (strcmp (fname, "-") != 0 && access (fname, F_OK) < 0)
-	    {
-	      bool resp = fname[0] == '@' && access (fname + 1, F_OK) < 0;
-	      error ("%s: %m", fname + resp);
-	    }
-          else
-	    add_infile (arg, spec_lang);
+	  add_infile (arg, spec_lang);
 
-          free (fname);
 	  continue;
 	}
 
Index: gcc/testsuite/lib/prune.exp
===================================================================
--- gcc/testsuite/lib/prune.exp	(revision 271131)
+++ gcc/testsuite/lib/prune.exp	(working copy)
@@ -37,8 +37,8 @@ proc prune_gcc_output { text } {
     regsub -all "(^|\n)\[^\n\]*:   . skipping \[0-9\]* instantiation contexts \[^\n\]*" $text "" text
     regsub -all "(^|\n)\[^\n\]*:   in constexpr expansion \[^\n\]*" $text "" text
     regsub -all "(^|\n)    inlined from \[^\n\]*" $text "" text
-    regsub -all "(^|\n)collect2: error: ld returned \[^\n\]*" $text "" text
-    regsub -all "(^|\n)collect: re(compiling|linking)\[^\n\]*" $text "" text
+    regsub -all "(^|\n)\[a-z+]*\\(?collect2\\)?: error: ld returned \[^\n\]*" $text "" text
+    regsub -all "(^|\n)\[a-z+]*\\(?collect\\)?: re(compiling|linking)\[^\n\]*" $text "" text
     regsub -all "(^|\n)Please submit.*instructions\[^\n\]*" $text "" text
     regsub -all "(^|\n)\[0-9\]\[0-9\]* errors\." $text "" text
     regsub -all "(^|\n)(In file included|\[ \]+from)\[^\n\]*" $text "" text

Re: [DRIVER PATCH]: Nadger subprocess argv[0]

[Martin Sebor]

On 5/14/19 8:02 AM, Nathan Sidwell wrote:
> This patch nadgers the driver's subprocess names to include the driver 
> name. It results in more informative error messages.  For instance, 
> rather than:
> 
>    >./xg++ -B./ frob.cc -c -fdump-tree-nope
>    cc1plus: error: unrecognized command line option '-fdump-tree-nope'
> 
> we get:
> 
>    >./xg++ -B./ frob.cc -c -fdump-tree-nope
>    xg++(cc1plus): error: unrecognized command line option 
> '-fdump-tree-nope'
> 
> Thereby cluing the user into this being a compiler error.  (When this 
> error is buried inside a build log, the poor user can be more confused 
> as to what this cc1plus thing might be).
> 
> I achieve this by taking advantage of the subprocess spawning taking 
> separate arguments for the program to exec, and the argv[0] value passed 
> to that program.  Because subprocesses can use argv[0] to locate 
> themselves I propagate the directory name, so that remains as before.
> 
> When using valgrind, this substitution is not performed, because 
> valgrind relies on argv[0] being the program pathname.
> 
> The -v output is unaffected, as that is emitted before altering argv[0]. 
>   argv[0] is also restored after spawning.
> 
> This then allows us to elide process_command's check of whether the 
> input file exists.  That's optimizing for failure, but I suspect is 
> desired merely to avoid an error of the form:
>    cc1plus: fatal error: frob.cc: No such file or directory
> 
> As you can see process_command does some special handling for lto & 
> @files, which is kind of icky and now goes away.  We get:
>    xg++(cc1plus): fatal error: frob.cc: No such file or directory
> 
> Thoughts? Ok? Stupid idea?

It sounds like a nice little improvement to me.

The question of what's cc1 and cc1plus sometimes comes up on
message boards indicating that users new to GCC find these
errors at first confusing.  A common one is about:

   gcc: error trying to exec ‘cc1plus’: execvp: No such file or directory”

This one prints gcc but what's cc1plus and what's execvp?  After
an incomplete install, newbie users go looking for cc1plus (and
sometimes apparently even for excecvp). Improving these messages
will make GCC easier for newbies to use.

Just a nit about the output above: is no space after the driver
name and before the parenthesis.  To my eyes that looks a little
odd.  I would expect see one.

Martin

Re: [DRIVER PATCH]: Nadger subprocess argv[0]

[Nathan Sidwell]

Ping?

https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00695.html

I originally hesitated adding a space, as Martin suggested, so that 
argv[0] still obviously looked like a single arg. But I'm not opposed to 
changing that.  He's also right that the errors one can get from failing 
to spawn are very implementor-speak, as Sandra would say.  But that's 
not addressed here.

On 5/14/19 10:02 AM, Nathan Sidwell wrote:
> This patch nadgers the driver's subprocess names to include the driver 
> name. It results in more informative error messages.  For instance, 
> rather than:
> 
>    >./xg++ -B./ frob.cc -c -fdump-tree-nope
>    cc1plus: error: unrecognized command line option '-fdump-tree-nope'
> 
> we get:
> 
>    >./xg++ -B./ frob.cc -c -fdump-tree-nope
>    xg++(cc1plus): error: unrecognized command line option 
> '-fdump-tree-nope'
> 
> Thereby cluing the user into this being a compiler error.  (When this 
> error is buried inside a build log, the poor user can be more confused 
> as to what this cc1plus thing might be).
> 
> I achieve this by taking advantage of the subprocess spawning taking 
> separate arguments for the program to exec, and the argv[0] value passed 
> to that program.  Because subprocesses can use argv[0] to locate 
> themselves I propagate the directory name, so that remains as before.
> 
> When using valgrind, this substitution is not performed, because 
> valgrind relies on argv[0] being the program pathname.
> 
> The -v output is unaffected, as that is emitted before altering argv[0]. 
>   argv[0] is also restored after spawning.
> 
> This then allows us to elide process_command's check of whether the 
> input file exists.  That's optimizing for failure, but I suspect is 
> desired merely to avoid an error of the form:
>    cc1plus: fatal error: frob.cc: No such file or directory
> 
> As you can see process_command does some special handling for lto & 
> @files, which is kind of icky and now goes away.  We get:
>    xg++(cc1plus): fatal error: frob.cc: No such file or directory
> 
> Thoughts? Ok? Stupid idea?
> 
> nathan
> 


-- 
Nathan Sidwell

Re: [DRIVER PATCH]: Nadger subprocess argv[0]

[Joseph Myers]

On Tue, 14 May 2019, Nathan Sidwell wrote:

> This patch nadgers the driver's subprocess names to include the driver name.
> It results in more informative error messages.  For instance, rather than:
> 
>   >./xg++ -B./ frob.cc -c -fdump-tree-nope
>   cc1plus: error: unrecognized command line option '-fdump-tree-nope'
> 
> we get:
> 
>   >./xg++ -B./ frob.cc -c -fdump-tree-nope
>   xg++(cc1plus): error: unrecognized command line option '-fdump-tree-nope'
> 
> Thereby cluing the user into this being a compiler error.  (When this error is
> buried inside a build log, the poor user can be more confused as to what this
> cc1plus thing might be).

My inclination is that, whatever you do with argv[0], it's a bug for the 
message to mention cc1plus at all.  The fact that there is an executable 
called cc1plus is an implementation detail, which shouldn't be visible in 
diagnostics; the diagnostics should just mention the program the user 
called (e.g. g++) and not cc1plus.  (So make toplev.c use the value of 
COLLECT_GCC, if set, instead of argv[0], to determine the name for 
diagnostic purposes, I suppose.  I believe COLLECT_GCC should always be 
set except when cc1plus was called manually, and if it was called manually 
then it's reasonable to mention it in diagnostics and the person calling 
it is probably a GCC developer anyway.)

-- 
Joseph S. Myers
joseph@codesourcery.com