| Message ID | 1525290467.28825.24.camel@cavium.com |
|---|---|
| State | New |
| Headers | show |
| Series | Do not call the linker if we are creating precompiled header files | expand |
Ping. Steve Ellcey sellcey@cavium.com On Wed, 2018-05-02 at 12:47 -0700, Steve Ellcey wrote: > This is a new version of a patch I sent out last year to stop gcc from > trying to do a link when creating precompiled headers and a linker > flag is also given. > > When I build and test GCC I also build glibc and then I run the GCC tests > with -Wl,-rpath and -Wl,--dynamic-linker so that I don't have to install > glibc and the compiler in the default locations. When I do this some > precompiled header tests fail because the existance of the linker flags > causes the compiler to try and call the linker when we really just want to > create pch files. > > I tracked this down to driver::maybe_run_linker where it sees the linker > flags and increments num_linker_inputs, this causes the routine to call > the linker. This patch checks to see if we are creating precompiled > header files and avoids calling the linker in that case. > > I tested this with the GCC testsuite and got no regressions, OK to > checkin? > > Steve Ellcey > sellcey@cavium.com > > > 2018-05-02 Steve Ellcey <sellcey@cavium.com> > > * gcc.c (create_pch_flag): New variable. > (driver::prepare_infiles): Set create_pch_flag > when we are creating precompiled headers. > (driver::maybe_run_linker): Do not link if > create_pch_flag is set. > (driver::finalize): Reset create_pch_flag. > > > diff --git a/gcc/gcc.c b/gcc/gcc.c > index a716f70..ca986cf 100644 > --- a/gcc/gcc.c > +++ b/gcc/gcc.c > @@ -208,6 +208,9 @@ int is_cpp_driver; > /* Flag set to nonzero if an @file argument has been supplied to gcc. */ > static bool at_file_supplied; > > +/* Flag set to nonzero if we are generating a precompiled header. */ > +static bool create_pch_flag; > + > /* Definition of string containing the arguments given to configure. */ > #include "configargs.h" > > @@ -8095,8 +8098,15 @@ driver::prepare_infiles () > strlen (name), > infiles[i].langua > ge); > > - if (compiler && !(compiler->combinable)) > - combine_inputs = false; > + if (compiler) > + { > + if (!(compiler->combinable)) > + combine_inputs = false; > + > + if ((strcmp(compiler->suffix, "@c-header") == 0) > + || (strcmp(compiler->suffix, "@c++-header") == 0)) > + create_pch_flag = true; > + } > > if (lang_n_infiles > 0 && compiler != input_file_compiler > && infiles[i].language && infiles[i].language[0] != '*') > @@ -8282,6 +8292,10 @@ driver::maybe_run_linker (const char *argv0) > const > int linker_was_run = 0; > int num_linker_inputs; > > + /* If we are creating a precompiled header, do not run the linker. */ > + if (create_pch_flag) > + return; > + > /* Determine if there are any linker input files. */ > num_linker_inputs = 0; > for (i = 0; (int) i < n_infiles; i++) > @@ -10052,6 +10066,7 @@ driver::finalize () > > is_cpp_driver = 0; > at_file_supplied = 0; > + create_pch_flag = 0; > print_help_list = 0; > print_version = 0; > verbose_only_flag = 0;
Ping^2 Steve Ellcey sellcey@cavium.com On Thu, 2018-05-17 at 14:50 -0700, Steve Ellcey wrote: > Ping. > > Steve Ellcey > sellcey@cavium.com > > > On Wed, 2018-05-02 at 12:47 -0700, Steve Ellcey wrote: > > > > This is a new version of a patch I sent out last year to stop gcc from > > trying to do a link when creating precompiled headers and a linker > > flag is also given. > > > > When I build and test GCC I also build glibc and then I run the GCC tests > > with -Wl,-rpath and -Wl,--dynamic-linker so that I don't have to install > > glibc and the compiler in the default locations. When I do this some > > precompiled header tests fail because the existance of the linker flags > > causes the compiler to try and call the linker when we really just want to > > create pch files. > > > > I tracked this down to driver::maybe_run_linker where it sees the linker > > flags and increments num_linker_inputs, this causes the routine to call > > the linker. This patch checks to see if we are creating precompiled > > header files and avoids calling the linker in that case. > > > > I tested this with the GCC testsuite and got no regressions, OK to > > checkin? > > > > Steve Ellcey > > sellcey@cavium.com > > > > > > 2018-05-02 Steve Ellcey <sellcey@cavium.com> > > > > * gcc.c (create_pch_flag): New variable. > > (driver::prepare_infiles): Set create_pch_flag > > when we are creating precompiled headers. > > (driver::maybe_run_linker): Do not link if > > create_pch_flag is set. > > (driver::finalize): Reset create_pch_flag. > > > > > > diff --git a/gcc/gcc.c b/gcc/gcc.c > > index a716f70..ca986cf 100644 > > --- a/gcc/gcc.c > > +++ b/gcc/gcc.c > > @@ -208,6 +208,9 @@ int is_cpp_driver; > > /* Flag set to nonzero if an @file argument has been supplied to > > gcc. */ > > static bool at_file_supplied; > > > > +/* Flag set to nonzero if we are generating a precompiled > > header. */ > > +static bool create_pch_flag; > > + > > /* Definition of string containing the arguments given to > > configure. */ > > #include "configargs.h" > > > > @@ -8095,8 +8098,15 @@ driver::prepare_infiles () > > strlen (name), > > infiles[i].lang > > ua > > ge); > > > > - if (compiler && !(compiler->combinable)) > > - combine_inputs = false; > > + if (compiler) > > + { > > + if (!(compiler->combinable)) > > + combine_inputs = false; > > + > > + if ((strcmp(compiler->suffix, "@c-header") == 0) > > + || (strcmp(compiler->suffix, "@c++-header") == 0)) > > + create_pch_flag = true; > > + } > > > > if (lang_n_infiles > 0 && compiler != input_file_compiler > > && infiles[i].language && infiles[i].language[0] != '*') > > @@ -8282,6 +8292,10 @@ driver::maybe_run_linker (const char *argv0) > > const > > int linker_was_run = 0; > > int num_linker_inputs; > > > > + /* If we are creating a precompiled header, do not run the > > linker. */ > > + if (create_pch_flag) > > + return; > > + > > /* Determine if there are any linker input files. */ > > num_linker_inputs = 0; > > for (i = 0; (int) i < n_infiles; i++) > > @@ -10052,6 +10066,7 @@ driver::finalize () > > > > is_cpp_driver = 0; > > at_file_supplied = 0; > > + create_pch_flag = 0; > > print_help_list = 0; > > print_version = 0; > > verbose_only_flag = 0;
On Wed, 2 May 2018, Steve Ellcey wrote: > I tracked this down to driver::maybe_run_linker where it sees the linker > flags and increments num_linker_inputs, this causes the routine to call > the linker. This patch checks to see if we are creating precompiled > header files and avoids calling the linker in that case. Making it a global check like this, with an early exit from driver::maybe_run_linker, seems wrong to me. I think the correct logical check is, for each output file, whether it is a precompiled header - if it is, then num_linker_inputs should not be incremented for that particular output file (but if there are other output files that are not precompiled headers, or if there are linker input files, num_linker_inputs should still be incremented for *those*). Then the existing logic in the rest of driver::maybe_run_linker should run as usual (including the logic to warn about explicit linker input files when linking is not done).
On Fri, 2018-06-08 at 11:17 +0000, Joseph Myers wrote: > On Wed, 2 May 2018, Steve Ellcey wrote: > > > > > I tracked this down to driver::maybe_run_linker where it sees the linker > > flags and increments num_linker_inputs, this causes the routine to call > > the linker. This patch checks to see if we are creating precompiled > > header files and avoids calling the linker in that case. > Making it a global check like this, with an early exit from > driver::maybe_run_linker, seems wrong to me. I think the correct logical > check is, for each output file, whether it is a precompiled header - if it > is, then num_linker_inputs should not be incremented for that particular > output file (but if there are other output files that are not precompiled > headers, or if there are linker input files, num_linker_inputs should > still be incremented for *those*). Then the existing logic in the rest of > driver::maybe_run_linker should run as usual (including the logic to warn > about explicit linker input files when linking is not done). The problem with this is that num_linker_inputs isn't getting incremented when it sees a file to be turned into a precompiled header it gets incremented when it sees a linker flag like '-rpath'. The comments in driver::prepare_infiles make it seem like we are just processing a list of file names but when I use something like -Wl,- rpath=/foo, the -rpath flag shows up in the list of 'files' that this function is processing and that is what causes the routine to set explicit_link_files and trigger the link. Steve Ellcey sellcey@cavium.com
On Tue, 19 Jun 2018, Steve Ellcey wrote: > The problem with this is that num_linker_inputs isn't getting > incremented when it sees a file to be turned into a precompiled header > it gets incremented when it sees a linker flag like '-rpath'. > > The comments in driver::prepare_infiles make it seem like we are just > processing a list of file names but when I use something like -Wl,- > rpath=/foo, the -rpath flag shows up in the list of 'files' that this > function is processing and that is what causes the routine to set > explicit_link_files and trigger the link. In that case this has nothing to do with precompiled headers at all. What should plain gcc -Wl,-rpath=/foo do? Whatever it should do regarding trying to link or not trying to link, it should do the same if you also name a header on the command line to be compiled to a precompiled header. So if you don't want it to try to link when building a precompiled header, you presumably want it to say "gcc: fatal error: no input files" just like if you pass some compilation option to GCC, rather than trying to run the linker. So the basic question is: is it valid to link a program (or shared library) using *only* -Wl, / -Xlinker to provide input files? Given that those are documented as being for passing options rather than input files I think it's reasonable to say it's not valid - so when -Wl, / -Xlinker are used, their arguments, stored alongside actual linker input files (any non-option that doesn't have a file extension known to be a language compiled by GCC), should be marked in some way to indicate that they are options not input files and so should not by themselves cause linking to be done. Then there's the question of whether -l<library> counts as an input file or an option for this purpose. Using it as an input file is a bit less contrived than using -Wl, options for that purpose - for example, it's valid to define main in a shared library (there's a glibc test for this), and then you can link a program using just -l<library> without naming any object files to link - so I'd be inclined to keep it as an input file, not an option.
On Tue, 2018-06-19 at 15:22 +0000, Joseph Myers wrote: > > > What should plain > > gcc -Wl,-rpath=/foo > > do? Whatever it should do regarding trying to link or not trying to link, > it should do the same if you also name a header on the command line to be > compiled to a precompiled header. So if you don't want it to try to link > when building a precompiled header, you presumably want it to say "gcc: > fatal error: no input files" just like if you pass some compilation option > to GCC, rather than trying to run the linker. Actually my preference would be for gcc to not call the linker and to not print an error message. If I use gcc to call the linker and link object files into an executable and I pass it compiler options like '-fauto-inc-dec' or '-Wa,--gdwarf-2' GCC does not complain that it is not calling the compiler or the assembler. We seem to accept the fact that some flags may not be used due to what type of compilation is being done and just ignore them. I would like to do the same with linker flags. > So the basic question is: is it valid to link a program (or shared > library) using *only* -Wl, / -Xlinker to provide input files? Given that > those are documented as being for passing options rather than input files > I think it's reasonable to say it's not valid - so when -Wl, / -Xlinker > are used, their arguments, stored alongside actual linker input files (any > non-option that doesn't have a file extension known to be a language > compiled by GCC), should be marked in some way to indicate that they are > options not input files and so should not by themselves cause linking to > be done. I think this seems right. > Then there's the question of whether -l counts as an input file > or an option for this purpose. Using it as an input file is a bit less > contrived than using -Wl, options for that purpose - for example, it's > valid to define main in a shared library (there's a glibc test for this), > and then you can link a program using just -l without naming any > object files to link - so I'd be inclined to keep it as an input file, not > an option. I can see both sides of this and don't feel strongly one way or the other. I have attached a new patch that does allow for the use of just -l options to force the linker to be called. Currently explicit_link_files gets set for arguments and options that get sent to the linker and if there are at least one of these then GCC calls the linker. I added a new field that gets set for arguments but not for files and changed GCC to only call the linker if it sees at least one non-argument input. Originally I was just not setting explicit_link_files for arguments but that did not work because outfiles was still getting set and that was still triggering the linker call. '-l' arguments are handled in two ways, if the user passes in a '-l' we count that as an input file (since a user could put main in a library and pass that to the linker with a -l), but if the -l comes from a SPEC file (like the -lstdc++ that gets added automtically) we do not count that as an input to the linker. I changed explicit_link_files from an array of char to an array of bool since that was how it was being used and made explicit_link_args to be an array of bool also. My reason for wanting this patch is that I build a complete toolchain with gcc, binutils, and glibc in a non-standard place and then run the GCC testsuite with: make check RUNTESTFLAGS="--tool_opts '--sysroot=$T/install -Wl,--dynamic-linker=$T/install/lib/ld-linux-aarch64.so.1 -Wl,- rpath=$T/install/lib64'" and without my patch a number of pch tests fail. Steve Ellcey sellcey@cavium.com 2018-06-22 Steve Ellcey <sellcey@cavium.com> * gcc.h (driver): Add explicit_link_args field. * gcc.c (driver::driver): Initialize explicit_link_args. (driver::~driver): Delete explicit_link_args. (driver::prepare_infiles): Allocate explicit_link_args and set it if a non library argument is seen. (driver::do_spec_on_infiles): Set explicit_link_args if linker argument is seen. (driver::maybe_run_linker): Do not increment num_linker_inputs for linker arguments (only for linker inputs). diff --git a/gcc/gcc.c b/gcc/gcc.c index 405d2e3..8c09687 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -7270,6 +7270,7 @@ compare_files (char *cmpfile[]) driver::driver (bool can_finalize, bool debug) : explicit_link_files (NULL), + explicit_link_args (NULL), decoded_options (NULL), m_option_suggestions (NULL) { @@ -7279,6 +7280,7 @@ driver::driver (bool can_finalize, bool debug) : driver::~driver () { XDELETEVEC (explicit_link_files); + XDELETEVEC (explicit_link_args); XDELETEVEC (decoded_options); if (m_option_suggestions) { @@ -8092,7 +8094,8 @@ driver::prepare_infiles () /* Record which files were specified explicitly as link input. */ - explicit_link_files = XCNEWVEC (char, n_infiles); + explicit_link_files = XCNEWVEC (bool, n_infiles); + explicit_link_args = XCNEWVEC (bool, n_infiles); combine_inputs = have_o || flag_wpa; @@ -8118,9 +8121,15 @@ driver::prepare_infiles () else { /* Since there is no compiler for this input file, assume it is a - linker file. */ - explicit_link_files[i] = 1; + linker file or linker argument. */ + explicit_link_files[i] = true; infiles[i].incompiler = NULL; + /* If this is an argument and not an object to be linked, set + explicit_link_args. We do not do this for -l because a user + can put objects (including main) into a library linnked with + -l. */ + if (infiles[i].name[0] == '-' && infiles[i].name[1] != 'l') + explicit_link_args[i] = true; } infiles[i].compiled = false; infiles[i].preprocessed = false; @@ -8240,7 +8249,15 @@ driver::do_spec_on_infiles () const record it as explicit linker input. */ else - explicit_link_files[i] = 1; + { + explicit_link_files[i] = true; + /* Set explicit_link_args if this is a linker argument and + not a object to be linked. This includes -l since this + comes from a SPEC file and not from the user. */ + if (infiles[i].name[0] == '-') + explicit_link_args[i] = true; + + } /* Clear the delete-on-failure queue, deleting the files in it if this compilation failed. */ @@ -8293,7 +8310,8 @@ driver::maybe_run_linker (const char *argv0) const /* Determine if there are any linker input files. */ num_linker_inputs = 0; for (i = 0; (int) i < n_infiles; i++) - if (explicit_link_files[i] || outfiles[i] != NULL) + if ((explicit_link_files[i] || outfiles[i] != NULL) + && !explicit_link_args[i]) num_linker_inputs++; /* Run ld to link all the compiler output files. */ diff --git a/gcc/gcc.h b/gcc/gcc.h index ddbf42f..79fe7ac 100644 --- a/gcc/gcc.h +++ b/gcc/gcc.h @@ -56,7 +56,8 @@ class driver int get_exit_code () const; private: - char *explicit_link_files; + bool *explicit_link_files; + bool *explicit_link_args; struct cl_decoded_option *decoded_options; unsigned int decoded_options_count; auto_vec <char *> *m_option_suggestions;
On Fri, 22 Jun 2018, Steve Ellcey wrote: > I can see both sides of this and don't feel strongly one way or the > other. I have attached a new patch that does allow for the use of just > -l options to force the linker to be called. I don't think this patch achieves the desired semantics. My understanding of the desired semantics is: just as doing plain gcc or gcc -O2 produces an error gcc: fatal error: no input files compilation terminated. so the same should apply if a linker option is passed, e.g. gcc -Wl,-rpath,/some/where (rather than the present attempt to link) but if a -l argument is passed, that should be treated as an input file and the driver should continue to try to link just as it does at present. And then, for the case you were concerned with, building precompiled headers, all three commands, with the name of a header to compile to a PCH file added to them, should again act the same: compile the header to a PCH file, do not try to link. But actually, what I see with the patch applied does not follow that. If I do ./gcc/xgcc -B./gcc/ -lm it does *not* try to link, just exits silently. And if I do ./gcc/xgcc -B./gcc/ -Wl,-nosuchoption it does *not* complain about a lack of input files, but again just exits silently. However, if I do ./gcc/xgcc -B./gcc/ -Wl,-rpath,/some/where it *does* try to link, when by the expected semantics, it should complain about the lack of input files. It appears that it's caring about whether the -Wl, arguments start with '-' or not, which is completely wrong - /some/where above is an argument to the -rpath linker option, it's not an input file. (For the next patch revision also note the "linnked" typo in a comment.)
diff --git a/gcc/gcc.c b/gcc/gcc.c index a716f70..ca986cf 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -208,6 +208,9 @@ int is_cpp_driver; /* Flag set to nonzero if an @file argument has been supplied to gcc. */ static bool at_file_supplied; +/* Flag set to nonzero if we are generating a precompiled header. */ +static bool create_pch_flag; + /* Definition of string containing the arguments given to configure. */ #include "configargs.h" @@ -8095,8 +8098,15 @@ driver::prepare_infiles () strlen (name), infiles[i].language); - if (compiler && !(compiler->combinable)) - combine_inputs = false; + if (compiler) + { + if (!(compiler->combinable)) + combine_inputs = false; + + if ((strcmp(compiler->suffix, "@c-header") == 0) + || (strcmp(compiler->suffix, "@c++-header") == 0)) + create_pch_flag = true; + } if (lang_n_infiles > 0 && compiler != input_file_compiler && infiles[i].language && infiles[i].language[0] != '*') @@ -8282,6 +8292,10 @@ driver::maybe_run_linker (const char *argv0) const int linker_was_run = 0; int num_linker_inputs; + /* If we are creating a precompiled header, do not run the linker. */ + if (create_pch_flag) + return; + /* Determine if there are any linker input files. */ num_linker_inputs = 0; for (i = 0; (int) i < n_infiles; i++) @@ -10052,6 +10066,7 @@ driver::finalize () is_cpp_driver = 0; at_file_supplied = 0; + create_pch_flag = 0; print_help_list = 0; print_version = 0; verbose_only_flag = 0;