Do not call the linker if we are creating precompiled header files

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
Related show

Commit Message

Steve Ellcey May 2, 2018, 7:47 p.m.
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.

Comments

Steve Ellcey May 17, 2018, 9:50 p.m. | #1
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;
Steve Ellcey June 4, 2018, 6:04 p.m. | #2
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;
Joseph Myers June 8, 2018, 11:17 a.m. | #3
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).

-- 
Joseph S. Myers
joseph@codesourcery.com
Steve Ellcey June 19, 2018, 2:47 p.m. | #4
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
Joseph Myers June 19, 2018, 3:22 p.m. | #5
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.

-- 
Joseph S. Myers
joseph@codesourcery.com
Steve Ellcey June 22, 2018, 3:54 p.m. | #6
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;
Joseph Myers Aug. 9, 2018, 10:34 p.m. | #7
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.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Patch

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;