| Message ID | a3b387fe-d266-4ce0-ae8f-b0d84459d5f0@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | driver: do not check input file existence here [PR 98452] | expand |
On Tue, 19 Jan 2021, Nathan Sidwell wrote: > Joseph, > I was relying on this patch on the modules branch, but didn't realize the > implications when merging and thought it was just a cleanup. I'm not sure why > the driver wants to check here, rather than leave it to the compiler. Seems > optimizing for failure? The only difference I can think is that the diagnostic > might mention the driver name, rather than say (cc1plus), but that's a > different problem that I've also reported. What do the error messages look like, before and after this patch, for the various cases? (Response file missing; file handled by e.g. cc1plus missing; file handled by the linker missing.) The check here dates back to commit 48fb792a91a6b0850d723dc87bcc18eeab7ac3f5 from 1993, so we don't have any further explanation of what motivated it.
On 1/19/21 6:27 PM, Joseph Myers wrote: > On Tue, 19 Jan 2021, Nathan Sidwell wrote: > >> Joseph, >> I was relying on this patch on the modules branch, but didn't realize the >> implications when merging and thought it was just a cleanup. I'm not sure why >> the driver wants to check here, rather than leave it to the compiler. Seems >> optimizing for failure? The only difference I can think is that the diagnostic >> might mention the driver name, rather than say (cc1plus), but that's a >> different problem that I've also reported. > > What do the error messages look like, before and after this patch, for the > various cases? (Response file missing; file handled by e.g. cc1plus > missing; file handled by the linker missing.) here are some experiments: known C++ non-existent file OLD: (1)devvm293:279>g++ -c nothing.cc g++: error: nothing.cc: No such file or directory g++: fatal error: no input files compilation terminated. NEW: devvm293:277>./xg++ -B./ -c nothing.cc cc1plus: fatal error: nothing.cc: No such file or directory compilation terminated. Specified as C++ non-existent file: OLD: (1)devvm293:280>g++ -c -x c++ nothing g++: error: nothing: No such file or directory g++: fatal error: no input files compilation terminated. NEW: (1)devvm293:278>./xg++ -B./ -c -x c++ nothing cc1plus: fatal error: nothing: No such file or directory compilation terminated. Unspecified non-existent file: OLD: 1)devvm293:292>g++ -c nothing g++: error: nothing: No such file or directory g++: fatal error: no input files compilation terminated. NEW: (1)devvm293:293>./xg++ -B./ -c nothing xg++: warning: nothing: linker input file unused because linking not done Specified as C++ non-existent response file: OLD: (1)devvm293:281>g++ -c -x c++ @nothing g++: error: nothing: No such file or directory g++: fatal error: no input files compilation terminated. NEW: (1)devvm293:283>./xg++ -B./ -c -x c++ @nothing cc1plus: fatal error: @nothing: No such file or directory compilation terminated. Unspecified non-existent response file: OLD: (1)devvm293:282>g++ -c @nothing g++: error: nothing: No such file or directory g++: fatal error: no input files compilation terminated. NEW: (1)devvm293:284>./xg++ -B./ -c @nothing xg++: warning: @nothing: linker input file unused because linking not done Non-existent linker file: OLD: 1)devvm293:300>g++ nothing gcc.o g++: error: nothing: No such file or directory NEW: 1)devvm293:299>./xg++ -B./ nothing gcc.o /data/users/nathans/tools/lib/gcc/x86_64-pc-linux-gnu/10.1.1/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find nothing: No such file or directory collect2: error: ld returned 1 exit status We already have ways of making 'cc1plus' appear on the error line, here's a module-specific case, but I'm sure there are others? (1)devvm293:298>./xg++ -B./ -c n.cc -fmodule-header=bob cc1plus: error: unknown header kind 'bob' > The check here dates back to commit > 48fb792a91a6b0850d723dc87bcc18eeab7ac3f5 from 1993, so we don't have any > further explanation of what motivated it. wow, ancient history :) nathan
On Wed, 20 Jan 2021, Nathan Sidwell wrote: > On 1/19/21 6:27 PM, Joseph Myers wrote: > > On Tue, 19 Jan 2021, Nathan Sidwell wrote: > > > > > Joseph, > > > I was relying on this patch on the modules branch, but didn't realize the > > > implications when merging and thought it was just a cleanup. I'm not sure > > > why > > > the driver wants to check here, rather than leave it to the compiler. > > > Seems > > > optimizing for failure? The only difference I can think is that the > > > diagnostic > > > might mention the driver name, rather than say (cc1plus), but that's a > > > different problem that I've also reported. > > > > What do the error messages look like, before and after this patch, for the > > various cases? (Response file missing; file handled by e.g. cc1plus > > missing; file handled by the linker missing.) > > here are some experiments: Thanks. Mentioning cc1plus is a pre-existing issue that applies in general to diagnostics coming from cc1plus and not associated with a given source file (as I said in <https://gcc.gnu.org/pipermail/gcc-patches/2019-November/534846.html>, I think such diagnostics ought to mention the program the user called, i.e. the driver, and not mention cc1plus at all unless the user really called it manually without the driver). Given that, the interesting cases are the ones where an error becomes a warning. > Unspecified non-existent file: > OLD: > 1)devvm293:292>g++ -c nothing > g++: error: nothing: No such file or directory > g++: fatal error: no input files > compilation terminated. > > NEW: > (1)devvm293:293>./xg++ -B./ -c nothing > xg++: warning: nothing: linker input file unused because linking not done This seems analogous to e.g. not detecting syntax errors when using -E, or not detecting that a file #included inside #if 0 doesn't exist: the requested processing doesn't involve doing anything with the file "nothing" so it seems appropriate for its absence not to be treated as an error. > Unspecified non-existent response file: > OLD: > (1)devvm293:282>g++ -c @nothing > g++: error: nothing: No such file or directory > g++: fatal error: no input files > compilation terminated. > > NEW: > (1)devvm293:284>./xg++ -B./ -c @nothing > xg++: warning: @nothing: linker input file unused because linking not done This is less clear, in that one might suppose "nothing" is meant to be a file listing C++ sources to compile, so should be processed and should result in an error for its absence. However, this particular place in the driver handling OPT_SPECIAL_input_file doesn't seem like the right place for such an error. Either the correct handling of response files is that the @file argument is silently kept as-is and so the warning after your patch is correct, or the correct handling of response files is that there should be an error if such a file cannot be found (and so users with filenames starting @ must reference them e.g. as ./@file.cc) and expandargv should have some way to report that error. Leaving it to OPT_SPECIAL_input_file as in the driver at present would mean such an error doesn't occur when @file, unexpanded, appears to be an option argument rather than an input file.
On 1/21/21 12:59 PM, Joseph Myers wrote: > On Wed, 20 Jan 2021, Nathan Sidwell wrote: > >> Unspecified non-existent response file: >> OLD: >> (1)devvm293:282>g++ -c @nothing >> g++: error: nothing: No such file or directory >> g++: fatal error: no input files >> compilation terminated. >> >> NEW: >> (1)devvm293:284>./xg++ -B./ -c @nothing >> xg++: warning: @nothing: linker input file unused because linking not done > > This is less clear, in that one might suppose "nothing" is meant to be a > file listing C++ sources to compile, so should be processed and should > result in an error for its absence. However, this particular place in the > driver handling OPT_SPECIAL_input_file doesn't seem like the right place > for such an error. Either the correct handling of response files is that > the @file argument is silently kept as-is and so the warning after your > patch is correct, or the correct handling of response files is that there > should be an error if such a file cannot be found (and so users with > filenames starting @ must reference them e.g. as ./@file.cc) and > expandargv should have some way to report that error. Leaving it to > OPT_SPECIAL_input_file as in the driver at present would mean such an > error doesn't occur when @file, unexpanded, appears to be an option > argument rather than an input file. > Agreed. I wonder who cares about naming sources '@file.cc' or '@linkerscript' or whatever? Do you want expandargv altered alongs the lines you mention? Or a bug filed? [in order for my patch to be acceptable] nathan
On Thu, 21 Jan 2021, Nathan Sidwell wrote: > Do you want expandargv altered alongs the lines you mention? Or a bug filed? > [in order for my patch to be acceptable] The patch is OK as-is. Filing a bug for expandargv handling of missing files might be a good idea; it seems very arbitrary that @directory produces an error, but @file for a nonexistent file, or a file without read access, or a file with an I/O error on reading, gets passed through.
On 1/21/21 3:20 PM, Joseph Myers wrote: > On Thu, 21 Jan 2021, Nathan Sidwell wrote: > >> Do you want expandargv altered alongs the lines you mention? Or a bug filed? >> [in order for my patch to be acceptable] > > The patch is OK as-is. Filing a bug for expandargv handling of missing > files might be a good idea; it seems very arbitrary that @directory > produces an error, but @file for a nonexistent file, or a file without > read access, or a file with an I/O error on reading, gets passed through. > 98794 filed, thanks! nathan
diff --git i/gcc/gcc.c w/gcc/gcc.c index 7dccfadfef2..aa5774af7e7 100644 --- i/gcc/gcc.c +++ w/gcc/gcc.c @@ -4811,44 +4811,12 @@ process_command (unsigned int decoded_options_count, 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; }
Joseph, I was relying on this patch on the modules branch, but didn't realize the implications when merging and thought it was just a cleanup. I'm not sure why the driver wants to check here, rather than leave it to the compiler. Seems optimizing for failure? The only difference I can think is that the diagnostic might mention the driver name, rather than say (cc1plus), but that's a different problem that I've also reported. The existing code seems confused anyway, we look for NAME, but then report NAME + 1 if name's initial char indicates a response file '@'. IMHO if that's some subtle way of giving an error for an (already) failed missing response file, it seems odd. The failure for modules concerns -x c++-system-header (or c++-user-header), where the compiler will search for the file on the appropriate include path. booted & tested on x86_64-linux, ok? gcc/ * gcc.c (process_command): Don't check OPT_SPECIAL_input_file existence here.