[DRIVER] : Nadger subprocess argv[0]

Message ID 377b67a8-72aa-16a4-2c53-1efb9e2e6d6a@acm.org
State New
Headers show
Series
  • [DRIVER] : Nadger subprocess argv[0]
Related show

Commit Message

Nathan Sidwell May 14, 2019, 2:02 p.m.
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

Comments

Martin Sebor May 14, 2019, 5:26 p.m. | #1
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

Patch

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