| Message ID | 377b67a8-72aa-16a4-2c53-1efb9e2e6d6a@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | [DRIVER] : Nadger subprocess argv[0] | expand |
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
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 >
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.)
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