[PATCH, driver specs] Put -flto-partition= on the collect2 c/l

[PATCH, driver specs] Put -flto-partition= on the collect2 c/l

[Iain Sandoe]

Hi 

While working on the Darwin LTO issues I noticed that collect2 looks for "-flto-partition=none” in its command line option, but it doesn’t get passed.

So - is the attached patch the right idea, or should collect2 be looking in the COLLECT_GCC_OPTIONS as the lto-wrapper does?

(or maybe it should just be just "%{fno-lto} %{flto*}" ?)

I plan on making Darwin default to fno-lto for link unless there’s an “flto*” on the link line, since otherwise there’s a process launch for every object on the c/l to do “nm”, which is quite heavy weight.  At present, ISTM if the compiler is configured with —enable-lto, the link line defaults to assuming that every object needs to be checked.

Iain.

 diff --git a/gcc/gcc.c b/gcc/gcc.c
index 780d485..bc9772b 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -1038,7 +1038,7 @@ proper position among the other output files.  */
     %(linker) " \
     LINK_PLUGIN_SPEC \
    "%{flto|flto=*:%<fcompare-debug*} \
-    %{flto} %{fno-lto} %{flto=*} %l " LINK_PIE_SPEC \
+    %{flto} %{flto-*} %{fno-lto} %{flto=*} %l " LINK_PIE_SPEC \
    "%{fuse-ld=*:-fuse-ld=%*} " LINK_COMPRESS_DEBUG_SPEC \
    "%X %{o*} %{e*} %{N} %{n} %{r}\
     %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}} \

Re: [PATCH, driver specs] Put -flto-partition= on the collect2 c/l

[Richard Biener]

On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi
>
> While working on the Darwin LTO issues I noticed that collect2 looks for "-flto-partition=none” in its command line option, but it doesn’t get passed.
>
> So - is the attached patch the right idea, or should collect2 be looking in the COLLECT_GCC_OPTIONS as the lto-wrapper does?

Looking at COLLECT_GCC_OPTIONS is probably better.

> (or maybe it should just be just "%{fno-lto} %{flto*}" ?)
>
> I plan on making Darwin default to fno-lto for link unless there’s an “flto*” on the link line, since otherwise there’s a process launch for every object on the c/l to do “nm”, which is quite heavy weight.  At present, ISTM if the compiler is configured with —enable-lto, the link line defaults to assuming that every object needs to be checked.

I think we wanted to transparently handle LTO objects (similar to how
it works with a linker plugin).

Ideally collect2 wouldn't use nm but simple-object to inspect files
(but that doesn't have symbol table query support
though looking for LTO specific sections would have been better in the
first place - I'd suggest .gnu.lto_.symtab).

Richard.

> Iain.
>
>  diff --git a/gcc/gcc.c b/gcc/gcc.c
> index 780d485..bc9772b 100644
> --- a/gcc/gcc.c
> +++ b/gcc/gcc.c
> @@ -1038,7 +1038,7 @@ proper position among the other output files.  */
>      %(linker) " \
>      LINK_PLUGIN_SPEC \
>     "%{flto|flto=*:%<fcompare-debug*} \
> -    %{flto} %{fno-lto} %{flto=*} %l " LINK_PIE_SPEC \
> +    %{flto} %{flto-*} %{fno-lto} %{flto=*} %l " LINK_PIE_SPEC \
>     "%{fuse-ld=*:-fuse-ld=%*} " LINK_COMPRESS_DEBUG_SPEC \
>     "%X %{o*} %{e*} %{N} %{n} %{r}\
>      %{s} %{t} %{u*} %{z} %{Z} %{!nostdlib:%{!nostartfiles:%S}} \
>

Re: [PATCH, driver specs][1] Put -flto-partition= on the collect2 c/l

[Iain Sandoe]

> On 20 Aug 2018, at 11:01, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe <iain@sandoe.co.uk> wrote:

>> While working on the Darwin LTO issues I noticed that collect2 looks for "-flto-partition=none” in its command line option, but it doesn’t get passed.
>> 
>> So - is the attached patch the right idea, or should collect2 be looking in the COLLECT_GCC_OPTIONS as the lto-wrapper does?
> 
> Looking at COLLECT_GCC_OPTIONS is probably better.

So I bootstrapped and tested the following on Darwin and Linux (both with a 32b multilib)

It’s kinda odd that we really have to look in two places for the union of the options to be handled, although the reasoning is clear enough.

Comments?
thanks
Iain

gcc/

	* collect2.c (main): Combine flags from both the command line and
	COLLECT_GCC_OPTIONS to determine the set in force.

---
 gcc/collect2.c | 55 +++++++++++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/gcc/collect2.c b/gcc/collect2.c
index a96af137a4..9ead5a6a1d 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -979,13 +979,14 @@ main (int argc, char **argv)
   object = CONST_CAST2 (const char **, char **, object_lst);
 
 #ifdef DEBUG
-  debug = 1;
+  debug = true;
 #endif
 
-  /* Parse command line early for instances of -debug.  This allows
-     the debug flag to be set before functions like find_a_file()
-     are called.  We also look for the -flto or -flto-partition=none flag to know
-     what LTO mode we are in.  */
+  save_temps = false;
+  verbose = false;
+  /* Parse command line / environment for flags we want early.
+     This allows the debug flag to be set before functions like find_a_file()
+     are called. */
   {
     bool no_partition = false;
 
@@ -993,8 +994,6 @@ main (int argc, char **argv)
       {
 	if (! strcmp (argv[i], "-debug"))
 	  debug = true;
-        else if (! strcmp (argv[i], "-flto-partition=none"))
-	  no_partition = true;
 	else if (!strncmp (argv[i], "-fno-lto", 8))
 	  lto_mode = LTO_MODE_NONE;
         else if (! strcmp (argv[i], "-plugin"))
@@ -1027,13 +1026,6 @@ main (int argc, char **argv)
 	    aixlazy_flag = 1;
 #endif
       }
-    verbose = debug;
-    find_file_set_debug (debug);
-    if (use_plugin)
-      lto_mode = LTO_MODE_NONE;
-    if (no_partition && lto_mode == LTO_MODE_WHOPR)
-      lto_mode = LTO_MODE_LTO;
-  }
 
 #ifndef DEFAULT_A_OUT_NAME
   output_file = "a.out";
@@ -1041,20 +1033,37 @@ main (int argc, char **argv)
   output_file = DEFAULT_A_OUT_NAME;
 #endif
 
-  obstack_begin (&temporary_obstack, 0);
-  temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
+    obstack_begin (&temporary_obstack, 0);
+    temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
 
 #ifndef HAVE_LD_DEMANGLE
   current_demangling_style = auto_demangling;
 #endif
-  p = getenv ("COLLECT_GCC_OPTIONS");
-  while (p && *p)
-    {
-      const char *q = extract_string (&p);
-      if (*q == '-' && (q[1] == 'm' || q[1] == 'f'))
-	num_c_args++;
+
+    /* Now pick up any flags we want early from COLLECT_GCC_OPTIONS
+       The LTO options are passed here as are other options that might
+       be unsuitable for ld (e.g. -save-temps).  */
+    p = getenv ("COLLECT_GCC_OPTIONS");
+    while (p && *p)
+      {
+	const char *q = extract_string (&p);
+	if (*q == '-' && (q[1] == 'm' || q[1] == 'f'))
+	  num_c_args++;
+	if (strncmp (q, "-flto-partition=none", 20) == 0)
+	  no_partition = true;
+	else if (strncmp (q, "-fno-lto", 8) == 0)
+	  lto_mode = LTO_MODE_NONE;
     }
-  obstack_free (&temporary_obstack, temporary_firstobj);
+    obstack_free (&temporary_obstack, temporary_firstobj);
+
+    verbose |= debug;
+    save_temps |= debug;
+    find_file_set_debug (debug);
+    if (use_plugin)
+      lto_mode = LTO_MODE_NONE;
+    if (no_partition && lto_mode == LTO_MODE_WHOPR)
+      lto_mode = LTO_MODE_LTO;
+  }
 
   /* -fno-profile-arcs -fno-test-coverage -fno-branch-probabilities
      -fno-exceptions -w -fno-whole-program */
-- 
2.17.1

Re: [PATCH, driver specs][1] Put -flto-partition= on the collect2 c/l

[Richard Biener]

On Wed, Aug 22, 2018 at 2:51 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
>
> > On 20 Aug 2018, at 11:01, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> >> While working on the Darwin LTO issues I noticed that collect2 looks for "-flto-partition=none” in its command line option, but it doesn’t get passed.
> >>
> >> So - is the attached patch the right idea, or should collect2 be looking in the COLLECT_GCC_OPTIONS as the lto-wrapper does?
> >
> > Looking at COLLECT_GCC_OPTIONS is probably better.
>
> So I bootstrapped and tested the following on Darwin and Linux (both with a 32b multilib)
>
> It’s kinda odd that we really have to look in two places for the union of the options to be handled, although the reasoning is clear enough.
>
> Comments?

OK.

Richard.

> thanks
> Iain
>
> gcc/
>
>         * collect2.c (main): Combine flags from both the command line and
>         COLLECT_GCC_OPTIONS to determine the set in force.
>
> ---
>  gcc/collect2.c | 55 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/gcc/collect2.c b/gcc/collect2.c
> index a96af137a4..9ead5a6a1d 100644
> --- a/gcc/collect2.c
> +++ b/gcc/collect2.c
> @@ -979,13 +979,14 @@ main (int argc, char **argv)
>    object = CONST_CAST2 (const char **, char **, object_lst);
>
>  #ifdef DEBUG
> -  debug = 1;
> +  debug = true;
>  #endif
>
> -  /* Parse command line early for instances of -debug.  This allows
> -     the debug flag to be set before functions like find_a_file()
> -     are called.  We also look for the -flto or -flto-partition=none flag to know
> -     what LTO mode we are in.  */
> +  save_temps = false;
> +  verbose = false;
> +  /* Parse command line / environment for flags we want early.
> +     This allows the debug flag to be set before functions like find_a_file()
> +     are called. */
>    {
>      bool no_partition = false;
>
> @@ -993,8 +994,6 @@ main (int argc, char **argv)
>        {
>         if (! strcmp (argv[i], "-debug"))
>           debug = true;
> -        else if (! strcmp (argv[i], "-flto-partition=none"))
> -         no_partition = true;
>         else if (!strncmp (argv[i], "-fno-lto", 8))
>           lto_mode = LTO_MODE_NONE;
>          else if (! strcmp (argv[i], "-plugin"))
> @@ -1027,13 +1026,6 @@ main (int argc, char **argv)
>             aixlazy_flag = 1;
>  #endif
>        }
> -    verbose = debug;
> -    find_file_set_debug (debug);
> -    if (use_plugin)
> -      lto_mode = LTO_MODE_NONE;
> -    if (no_partition && lto_mode == LTO_MODE_WHOPR)
> -      lto_mode = LTO_MODE_LTO;
> -  }
>
>  #ifndef DEFAULT_A_OUT_NAME
>    output_file = "a.out";
> @@ -1041,20 +1033,37 @@ main (int argc, char **argv)
>    output_file = DEFAULT_A_OUT_NAME;
>  #endif
>
> -  obstack_begin (&temporary_obstack, 0);
> -  temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
> +    obstack_begin (&temporary_obstack, 0);
> +    temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
>
>  #ifndef HAVE_LD_DEMANGLE
>    current_demangling_style = auto_demangling;
>  #endif
> -  p = getenv ("COLLECT_GCC_OPTIONS");
> -  while (p && *p)
> -    {
> -      const char *q = extract_string (&p);
> -      if (*q == '-' && (q[1] == 'm' || q[1] == 'f'))
> -       num_c_args++;
> +
> +    /* Now pick up any flags we want early from COLLECT_GCC_OPTIONS
> +       The LTO options are passed here as are other options that might
> +       be unsuitable for ld (e.g. -save-temps).  */
> +    p = getenv ("COLLECT_GCC_OPTIONS");
> +    while (p && *p)
> +      {
> +       const char *q = extract_string (&p);
> +       if (*q == '-' && (q[1] == 'm' || q[1] == 'f'))
> +         num_c_args++;
> +       if (strncmp (q, "-flto-partition=none", 20) == 0)
> +         no_partition = true;
> +       else if (strncmp (q, "-fno-lto", 8) == 0)
> +         lto_mode = LTO_MODE_NONE;
>      }
> -  obstack_free (&temporary_obstack, temporary_firstobj);
> +    obstack_free (&temporary_obstack, temporary_firstobj);
> +
> +    verbose |= debug;
> +    save_temps |= debug;
> +    find_file_set_debug (debug);
> +    if (use_plugin)
> +      lto_mode = LTO_MODE_NONE;
> +    if (no_partition && lto_mode == LTO_MODE_WHOPR)
> +      lto_mode = LTO_MODE_LTO;
> +  }
>
>    /* -fno-profile-arcs -fno-test-coverage -fno-branch-probabilities
>       -fno-exceptions -w -fno-whole-program */
> --
> 2.17.1
>
>

Re: [PATCH, driver specs][2] Put -flto-partition= on the collect2 c/l

[Iain Sandoe]

> On 20 Aug 2018, at 11:01, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>> 

>> I plan on making Darwin default to fno-lto for link unless there’s an “flto*” on the link line, since otherwise there’s a process launch for every object on the c/l to do “nm”, which is quite heavy weight.  At present, ISTM if the compiler is configured with —enable-lto, the link line defaults to assuming that every object needs to be checked.
> 
> I think we wanted to transparently handle LTO objects (similar to how
> it works with a linker plugin).

At present, we are not only calling nm for every object, but also dsymutil sometimes unnecessarily - since the assumption on the “maybe_lto” path is that an temporary file will be generated.

as a headline - when I did the simplistic “default is to assume fno-lto” that took 10% off the bootstrap time (with a stage#1 compiler without the optimisation).

- I have a patch under test as below + better fidelity of avoiding dsymutil runs.

> Ideally collect2 wouldn't use nm but simple-object to inspect files
> (but that doesn't have symbol table query support

Hrm.my WIP had grow symtab awareness to support debug-section copy on mach-o
(and the ELF impl. looks like it understands both symtab and relocs) 
- so maybe that’s not as far away as we might think.

> though looking for LTO specific sections would have been better in the
> first place - I'd suggest .gnu.lto_.symtab).

I’ve done this noting that the symtab seems to be the last emitted section - is that why you chose it (for security, rather than just stopping as soon as we see a .gnu.lto_. section?)

bootstrapped (Darwin and Linux) along with a third patch to make collect2 respond to -save-temps.

comments?
thanks
Iain

gcc/

	* collect2.c (maybe_run_lto_and_relink): Don’t say we have a temp file
	unless we actually did some LTO.
	(has_lto_symtab, is_lto_object_file): New.
	(maybe_lto_object_file): Remove.
	(scan_prog_file): Use is_lto_object_file() instead of scanning the output
	of nm.

---
 gcc/collect2.c | 116 ++++++++++++++++++++++---------------------------
 1 file changed, 53 insertions(+), 63 deletions(-)

diff --git a/gcc/collect2.c b/gcc/collect2.c
index 9ead5a6a1d..4b0f191ad3 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -30,6 +30,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "tm.h"
 #include "filenames.h"
 #include "file-find.h"
+#include "simple-object.h"
+#include "lto-section-names.h"
 
 /* TARGET_64BIT may be defined to use driver specific functionality. */
 #undef TARGET_64BIT
@@ -804,7 +806,9 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
       /* Run the linker again, this time replacing the object files
          optimized by the LTO with the temporary file generated by the LTO.  */
       fork_execute ("ld", out_lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
-      post_ld_pass (true);
+      /* We assume that temp files were created, and therefore we need to take
+         that into account (maybe run dsymutil).  */
+      post_ld_pass (/*temp_file*/true);
       free (lto_ld_argv);
 
       maybe_unlink_list (lto_o_files);
@@ -814,10 +818,11 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
       /* Our caller is relying on us to do the link
          even though there is no LTO back end work to be done.  */
       fork_execute ("ld", lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
-      post_ld_pass (false);
+      /* No LTO objects were found, so no new temp file.  */
+      post_ld_pass (/*temp_file*/false);
     }
   else
-    post_ld_pass (true);
+    post_ld_pass (false); /* No LTO objects were found, no temp file.  */
 }
 \f
 /* Main program.  */
@@ -1677,7 +1682,7 @@ main (int argc, char **argv)
 	if (lto_mode != LTO_MODE_NONE)
 	  maybe_run_lto_and_relink (ld1_argv, object_lst, object, false);
 	else
-	  post_ld_pass (false);
+	  post_ld_pass (/*temp_file*/false);
 
 	maybe_unlink (c_file);
 	maybe_unlink (o_file);
@@ -1749,7 +1754,7 @@ main (int argc, char **argv)
 #ifdef COLLECT_EXPORT_LIST
       maybe_unlink (export_file);
 #endif
-      post_ld_pass (false);
+      post_ld_pass (/*temp_file*/false);
 
       maybe_unlink (c_file);
       maybe_unlink (o_file);
@@ -1843,7 +1848,7 @@ main (int argc, char **argv)
   else
     {
       fork_execute ("ld", ld2_argv, HAVE_GNU_LD && at_file_supplied);
-      post_ld_pass (false);
+      post_ld_pass (/*temp_file*/false);
     }
 
   /* Let scan_prog_file do any final mods (OSF/rose needs this for
@@ -2300,38 +2305,48 @@ write_aix_file (FILE *stream, struct id *list)
 
 /* Check to make sure the file is an LTO object file.  */
 
+static int
+has_lto_symtab (void *data, const char *name ATTRIBUTE_UNUSED,
+		off_t offset ATTRIBUTE_UNUSED,
+		off_t length ATTRIBUTE_UNUSED)
+{
+  int *found = (int *) data;
+
+  if (strncmp (name, ".gnu.lto_.symtab.",
+	       sizeof (".gnu.lto_.symtab.") - 1) != 0)
+    return 1;
+
+  *found = 1;
+
+  /* Stop iteration.  */
+  return 0;
+}
+
 static bool
-maybe_lto_object_file (const char *prog_name)
+is_lto_object_file (const char *prog_name)
 {
-  FILE *f;
-  unsigned char buf[4];
-  int i;
+  const char *errmsg;
+  int err;
+  int found = 0;
+  off_t inoff = 0;
+  int infd = open (prog_name, O_RDONLY | O_BINARY);
 
-  static unsigned char elfmagic[4] = { 0x7f, 'E', 'L', 'F' };
-  static unsigned char coffmagic[2] = { 0x4c, 0x01 };
-  static unsigned char coffmagic_x64[2] = { 0x64, 0x86 };
-  static unsigned char machomagic[4][4] = {
-    { 0xcf, 0xfa, 0xed, 0xfe },
-    { 0xce, 0xfa, 0xed, 0xfe },
-    { 0xfe, 0xed, 0xfa, 0xcf },
-    { 0xfe, 0xed, 0xfa, 0xce }
-  };
+  if (infd == -1)
+    return false;
 
-  f = fopen (prog_name, "rb");
-  if (f == NULL)
+  simple_object_read *inobj = simple_object_start_read (infd, inoff,
+							LTO_SEGMENT_NAME,
+							&errmsg, &err);
+  if (!inobj)
     return false;
-  if (fread (buf, sizeof (buf), 1, f) != 1)
-    buf[0] = 0;
-  fclose (f);
 
-  if (memcmp (buf, elfmagic, sizeof (elfmagic)) == 0
-      || memcmp (buf, coffmagic, sizeof (coffmagic)) == 0
-      || memcmp (buf, coffmagic_x64, sizeof (coffmagic_x64)) == 0)
+  errmsg = simple_object_find_sections (inobj, has_lto_symtab,
+					(void *) &found, &err);
+  if (! errmsg && found)
     return true;
-  for (i = 0; i < 4; i++)
-    if (memcmp (buf, machomagic[i], sizeof (machomagic[i])) == 0)
-      return true;
 
+  if (errmsg)
+    fatal_error (0, "%s: %s\n", errmsg, xstrerror (err));
   return false;
 }
 
@@ -2354,7 +2369,6 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
   int err;
   char *p, buf[1024];
   FILE *inf;
-  int found_lto = 0;
 
   if (which_pass == PASS_SECOND)
     return;
@@ -2362,8 +2376,13 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
   /* LTO objects must be in a known format.  This check prevents
      us from accepting an archive containing LTO objects, which
      gcc cannot currently handle.  */
-  if (which_pass == PASS_LTOINFO && !maybe_lto_object_file (prog_name))
-    return;
+  if (which_pass == PASS_LTOINFO)
+    {
+      if(is_lto_object_file (prog_name)) {
+	add_lto_object (&lto_objects, prog_name);
+      }
+      return;
+    }
 
   /* If we do not have an `nm', complain.  */
   if (nm_file_name == 0)
@@ -2418,12 +2437,7 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
     fatal_error (input_location, "can't open nm output: %m");
 
   if (debug)
-    {
-      if (which_pass == PASS_LTOINFO)
-        fprintf (stderr, "\nnm output with LTO info marker symbol.\n");
-      else
-        fprintf (stderr, "\nnm output with constructors/destructors.\n");
-    }
+    fprintf (stderr, "\nnm output with constructors/destructors.\n");
 
   /* Read each line of nm output.  */
   while (fgets (buf, sizeof buf, inf) != (char *) 0)
@@ -2434,30 +2448,6 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
       if (debug)
         fprintf (stderr, "\t%s\n", buf);
 
-      if (which_pass == PASS_LTOINFO)
-        {
-          if (found_lto)
-            continue;
-
-          /* Look for the LTO info marker symbol, and add filename to
-             the LTO objects list if found.  */
-          for (p = buf; (ch = *p) != '\0' && ch != '\n'; p++)
-            if (ch == ' '  && p[1] == '_' && p[2] == '_'
-		&& (strncmp (p + (p[3] == '_' ? 2 : 1), "__gnu_lto_v1", 12) == 0)
-		&& ISSPACE (p[p[3] == '_' ? 14 : 13]))
-              {
-                add_lto_object (&lto_objects, prog_name);
-
-                /* We need to read all the input, so we can't just
-                   return here.  But we can avoid useless work.  */
-                found_lto = 1;
-
-                break;
-              }
-
-	  continue;
-        }
-
       /* If it contains a constructor or destructor name, add the name
 	 to the appropriate list unless this is a kind of symbol we're
 	 not supposed to even consider.  */
-- 
2.17.1

Re: [PATCH, driver specs][2] Put -flto-partition= on the collect2 c/l

[Richard Biener]

On Wed, Aug 22, 2018 at 2:56 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
>
> > On 20 Aug 2018, at 11:01, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>
>
> >> I plan on making Darwin default to fno-lto for link unless there’s an “flto*” on the link line, since otherwise there’s a process launch for every object on the c/l to do “nm”, which is quite heavy weight.  At present, ISTM if the compiler is configured with —enable-lto, the link line defaults to assuming that every object needs to be checked.
> >
> > I think we wanted to transparently handle LTO objects (similar to how
> > it works with a linker plugin).
>
> At present, we are not only calling nm for every object, but also dsymutil sometimes unnecessarily - since the assumption on the “maybe_lto” path is that an temporary file will be generated.
>
> as a headline - when I did the simplistic “default is to assume fno-lto” that took 10% off the bootstrap time (with a stage#1 compiler without the optimisation).
>
> - I have a patch under test as below + better fidelity of avoiding dsymutil runs.
>
> > Ideally collect2 wouldn't use nm but simple-object to inspect files
> > (but that doesn't have symbol table query support
>
> Hrm.my WIP had grow symtab awareness to support debug-section copy on mach-o
> (and the ELF impl. looks like it understands both symtab and relocs)
> - so maybe that’s not as far away as we might think.
>
> > though looking for LTO specific sections would have been better in the
> > first place - I'd suggest .gnu.lto_.symtab).
>
> I’ve done this noting that the symtab seems to be the last emitted section - is that why you chose it (for security, rather than just stopping as soon as we see a .gnu.lto_. section?)

Ah, any .gnu.lto_.section would work as well I guess - I just picked
one that should be always
created.  .gnu.lto_.opts is another one.

> bootstrapped (Darwin and Linux) along with a third patch to make collect2 respond to -save-temps.
>
> comments?

OK.  Feel free to stop when recognizing any LTO section.  Btw, as you
include lto-section-names.h
you should probably use LTO_SECTION_NAME_PREFIX instead of hard-coding
.gnu.lto_.

I'm not sure if/how we should handle offload sections and if those
work at all without a
linker plugin currently?

Richard.

> thanks
> Iain
>
> gcc/
>
>         * collect2.c (maybe_run_lto_and_relink): Don’t say we have a temp file
>         unless we actually did some LTO.
>         (has_lto_symtab, is_lto_object_file): New.
>         (maybe_lto_object_file): Remove.
>         (scan_prog_file): Use is_lto_object_file() instead of scanning the output
>         of nm.
>
> ---
>  gcc/collect2.c | 116 ++++++++++++++++++++++---------------------------
>  1 file changed, 53 insertions(+), 63 deletions(-)
>
> diff --git a/gcc/collect2.c b/gcc/collect2.c
> index 9ead5a6a1d..4b0f191ad3 100644
> --- a/gcc/collect2.c
> +++ b/gcc/collect2.c
> @@ -30,6 +30,8 @@ along with GCC; see the file COPYING3.  If not see
>  #include "tm.h"
>  #include "filenames.h"
>  #include "file-find.h"
> +#include "simple-object.h"
> +#include "lto-section-names.h"
>
>  /* TARGET_64BIT may be defined to use driver specific functionality. */
>  #undef TARGET_64BIT
> @@ -804,7 +806,9 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
>        /* Run the linker again, this time replacing the object files
>           optimized by the LTO with the temporary file generated by the LTO.  */
>        fork_execute ("ld", out_lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
> -      post_ld_pass (true);
> +      /* We assume that temp files were created, and therefore we need to take
> +         that into account (maybe run dsymutil).  */
> +      post_ld_pass (/*temp_file*/true);
>        free (lto_ld_argv);
>
>        maybe_unlink_list (lto_o_files);
> @@ -814,10 +818,11 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
>        /* Our caller is relying on us to do the link
>           even though there is no LTO back end work to be done.  */
>        fork_execute ("ld", lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
> -      post_ld_pass (false);
> +      /* No LTO objects were found, so no new temp file.  */
> +      post_ld_pass (/*temp_file*/false);
>      }
>    else
> -    post_ld_pass (true);
> +    post_ld_pass (false); /* No LTO objects were found, no temp file.  */
>  }
>
>  /* Main program.  */
> @@ -1677,7 +1682,7 @@ main (int argc, char **argv)
>         if (lto_mode != LTO_MODE_NONE)
>           maybe_run_lto_and_relink (ld1_argv, object_lst, object, false);
>         else
> -         post_ld_pass (false);
> +         post_ld_pass (/*temp_file*/false);
>
>         maybe_unlink (c_file);
>         maybe_unlink (o_file);
> @@ -1749,7 +1754,7 @@ main (int argc, char **argv)
>  #ifdef COLLECT_EXPORT_LIST
>        maybe_unlink (export_file);
>  #endif
> -      post_ld_pass (false);
> +      post_ld_pass (/*temp_file*/false);
>
>        maybe_unlink (c_file);
>        maybe_unlink (o_file);
> @@ -1843,7 +1848,7 @@ main (int argc, char **argv)
>    else
>      {
>        fork_execute ("ld", ld2_argv, HAVE_GNU_LD && at_file_supplied);
> -      post_ld_pass (false);
> +      post_ld_pass (/*temp_file*/false);
>      }
>
>    /* Let scan_prog_file do any final mods (OSF/rose needs this for
> @@ -2300,38 +2305,48 @@ write_aix_file (FILE *stream, struct id *list)
>
>  /* Check to make sure the file is an LTO object file.  */
>
> +static int
> +has_lto_symtab (void *data, const char *name ATTRIBUTE_UNUSED,
> +               off_t offset ATTRIBUTE_UNUSED,
> +               off_t length ATTRIBUTE_UNUSED)
> +{
> +  int *found = (int *) data;
> +
> +  if (strncmp (name, ".gnu.lto_.symtab.",
> +              sizeof (".gnu.lto_.symtab.") - 1) != 0)
> +    return 1;
> +
> +  *found = 1;
> +
> +  /* Stop iteration.  */
> +  return 0;
> +}
> +
>  static bool
> -maybe_lto_object_file (const char *prog_name)
> +is_lto_object_file (const char *prog_name)
>  {
> -  FILE *f;
> -  unsigned char buf[4];
> -  int i;
> +  const char *errmsg;
> +  int err;
> +  int found = 0;
> +  off_t inoff = 0;
> +  int infd = open (prog_name, O_RDONLY | O_BINARY);
>
> -  static unsigned char elfmagic[4] = { 0x7f, 'E', 'L', 'F' };
> -  static unsigned char coffmagic[2] = { 0x4c, 0x01 };
> -  static unsigned char coffmagic_x64[2] = { 0x64, 0x86 };
> -  static unsigned char machomagic[4][4] = {
> -    { 0xcf, 0xfa, 0xed, 0xfe },
> -    { 0xce, 0xfa, 0xed, 0xfe },
> -    { 0xfe, 0xed, 0xfa, 0xcf },
> -    { 0xfe, 0xed, 0xfa, 0xce }
> -  };
> +  if (infd == -1)
> +    return false;
>
> -  f = fopen (prog_name, "rb");
> -  if (f == NULL)
> +  simple_object_read *inobj = simple_object_start_read (infd, inoff,
> +                                                       LTO_SEGMENT_NAME,
> +                                                       &errmsg, &err);
> +  if (!inobj)
>      return false;
> -  if (fread (buf, sizeof (buf), 1, f) != 1)
> -    buf[0] = 0;
> -  fclose (f);
>
> -  if (memcmp (buf, elfmagic, sizeof (elfmagic)) == 0
> -      || memcmp (buf, coffmagic, sizeof (coffmagic)) == 0
> -      || memcmp (buf, coffmagic_x64, sizeof (coffmagic_x64)) == 0)
> +  errmsg = simple_object_find_sections (inobj, has_lto_symtab,
> +                                       (void *) &found, &err);
> +  if (! errmsg && found)
>      return true;
> -  for (i = 0; i < 4; i++)
> -    if (memcmp (buf, machomagic[i], sizeof (machomagic[i])) == 0)
> -      return true;
>
> +  if (errmsg)
> +    fatal_error (0, "%s: %s\n", errmsg, xstrerror (err));
>    return false;
>  }
>
> @@ -2354,7 +2369,6 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>    int err;
>    char *p, buf[1024];
>    FILE *inf;
> -  int found_lto = 0;
>
>    if (which_pass == PASS_SECOND)
>      return;
> @@ -2362,8 +2376,13 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>    /* LTO objects must be in a known format.  This check prevents
>       us from accepting an archive containing LTO objects, which
>       gcc cannot currently handle.  */
> -  if (which_pass == PASS_LTOINFO && !maybe_lto_object_file (prog_name))
> -    return;
> +  if (which_pass == PASS_LTOINFO)
> +    {
> +      if(is_lto_object_file (prog_name)) {
> +       add_lto_object (&lto_objects, prog_name);
> +      }
> +      return;
> +    }
>
>    /* If we do not have an `nm', complain.  */
>    if (nm_file_name == 0)
> @@ -2418,12 +2437,7 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>      fatal_error (input_location, "can't open nm output: %m");
>
>    if (debug)
> -    {
> -      if (which_pass == PASS_LTOINFO)
> -        fprintf (stderr, "\nnm output with LTO info marker symbol.\n");
> -      else
> -        fprintf (stderr, "\nnm output with constructors/destructors.\n");
> -    }
> +    fprintf (stderr, "\nnm output with constructors/destructors.\n");
>
>    /* Read each line of nm output.  */
>    while (fgets (buf, sizeof buf, inf) != (char *) 0)
> @@ -2434,30 +2448,6 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>        if (debug)
>          fprintf (stderr, "\t%s\n", buf);
>
> -      if (which_pass == PASS_LTOINFO)
> -        {
> -          if (found_lto)
> -            continue;
> -
> -          /* Look for the LTO info marker symbol, and add filename to
> -             the LTO objects list if found.  */
> -          for (p = buf; (ch = *p) != '\0' && ch != '\n'; p++)
> -            if (ch == ' '  && p[1] == '_' && p[2] == '_'
> -               && (strncmp (p + (p[3] == '_' ? 2 : 1), "__gnu_lto_v1", 12) == 0)
> -               && ISSPACE (p[p[3] == '_' ? 14 : 13]))
> -              {
> -                add_lto_object (&lto_objects, prog_name);
> -
> -                /* We need to read all the input, so we can't just
> -                   return here.  But we can avoid useless work.  */
> -                found_lto = 1;
> -
> -                break;
> -              }
> -
> -         continue;
> -        }
> -
>        /* If it contains a constructor or destructor name, add the name
>          to the appropriate list unless this is a kind of symbol we're
>          not supposed to even consider.  */
> --
> 2.17.1
>
>

Re: [PATCH, driver specs][2] Put -flto-partition= on the collect2 c/l

[Iain Sandoe]

Hi

This got stuck in my stack of patches for LTO debug support, and I forgot to ping it…

> On 22 Aug 2018, at 14:20, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Wed, Aug 22, 2018 at 2:56 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>> 
>> 
>>> On 20 Aug 2018, at 11:01, Richard Biener <richard.guenther@gmail.com> wrote:
>>> 
>>> On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>>>> 
>> 
>>>> I plan on making Darwin default to fno-lto for link unless there’s an “flto*” on the link line, since otherwise there’s a process launch for every object on the c/l to do “nm”, which is quite heavy weight.  At present, ISTM if the compiler is configured with —enable-lto, the link line defaults to assuming that every object needs to be checked.
>>> 
>>> I think we wanted to transparently handle LTO objects (similar to how
>>> it works with a linker plugin).
>> 
>> At present, we are not only calling nm for every object, but also dsymutil sometimes unnecessarily - since the assumption on the “maybe_lto” path is that an temporary file will be generated.
>> 
>> as a headline - when I did the simplistic “default is to assume fno-lto” that took 10% off the bootstrap time (with a stage#1 compiler without the optimisation).
>> 
>> - I have a patch under test as below + better fidelity of avoiding dsymutil runs.
>> 
>>> Ideally collect2 wouldn't use nm but simple-object to inspect files
>>> (but that doesn't have symbol table query support
>> 
>> Hrm.my WIP had grow symtab awareness to support debug-section copy on mach-o
>> (and the ELF impl. looks like it understands both symtab and relocs)
>> - so maybe that’s not as far away as we might think.
>> 
>>> though looking for LTO specific sections would have been better in the
>>> first place - I'd suggest .gnu.lto_.symtab).
>> 
>> I’ve done this noting that the symtab seems to be the last emitted section - is that why you chose it (for security, rather than just stopping as soon as we see a .gnu.lto_. section?)
> 
> Ah, any .gnu.lto_.section would work as well I guess - I just picked
> one that should be always
> created.  .gnu.lto_.opts is another one.

>> comments?
> 
> OK.  Feel free to stop when recognizing any LTO section.  Btw, as you
> include lto-section-names.h
> you should probably use LTO_SECTION_NAME_PREFIX instead of hard-coding
> .gnu.lto_.

So I just look for the LTO_SECTION_NAME_PREFIX

> I'm not sure if/how we should handle offload sections and if those
> work at all without a
> linker plugin currently?

In this version, I look for the OFFLOAD_SECTION_NAME_PREFIX as well,
although AFAICS collect2 itself has no specific handling of offload, the detection
is done in lto-wrapper.

OK for trunk now?

thanks
Iain


From: Iain Sandoe <iain@sandoe.co.uk>
Date: Tue, 21 Aug 2018 12:55:02 +0100
Subject: [PATCH] [PATCH, collect2] Use simple-object to find out if objects
 contain LTO.

This replaces the use of nm to search for the LTO common symbol marker
and uses simple object to see if there's a section starting with
".gnu.lto_." or .gnu.offload_lto_
---
 gcc/collect2.c | 120 +++++++++++++++++++++++--------------------------
 1 file changed, 57 insertions(+), 63 deletions(-)

diff --git a/gcc/collect2.c b/gcc/collect2.c
index 60269682ec..dcbd3e18a6 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -30,6 +30,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "tm.h"
 #include "filenames.h"
 #include "file-find.h"
+#include "simple-object.h"
+#include "lto-section-names.h"
 
 /* TARGET_64BIT may be defined to use driver specific functionality. */
 #undef TARGET_64BIT
@@ -804,7 +806,9 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
       /* Run the linker again, this time replacing the object files
          optimized by the LTO with the temporary file generated by the LTO.  */
       fork_execute ("ld", out_lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
-      post_ld_pass (true);
+      /* We assume that temp files were created, and therefore we need to take
+         that into account (maybe run dsymutil).  */
+      post_ld_pass (/*temp_file*/true);
       free (lto_ld_argv);
 
       maybe_unlink_list (lto_o_files);
@@ -814,10 +818,11 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
       /* Our caller is relying on us to do the link
          even though there is no LTO back end work to be done.  */
       fork_execute ("ld", lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
-      post_ld_pass (false);
+      /* No LTO objects were found, so no new temp file.  */
+      post_ld_pass (/*temp_file*/false);
     }
   else
-    post_ld_pass (true);
+    post_ld_pass (false); /* No LTO objects were found, no temp file.  */
 }
 \f
 /* Main program.  */
@@ -1710,7 +1715,7 @@ main (int argc, char **argv)
 	if (lto_mode != LTO_MODE_NONE)
 	  maybe_run_lto_and_relink (ld1_argv, object_lst, object, false);
 	else
-	  post_ld_pass (false);
+	  post_ld_pass (/*temp_file*/false);
 
 	return 0;
       }
@@ -1780,7 +1785,7 @@ main (int argc, char **argv)
 #ifdef COLLECT_EXPORT_LIST
       maybe_unlink (export_file);
 #endif
-      post_ld_pass (false);
+      post_ld_pass (/*temp_file*/false);
 
       maybe_unlink (c_file);
       maybe_unlink (o_file);
@@ -1874,7 +1879,7 @@ main (int argc, char **argv)
   else
     {
       fork_execute ("ld", ld2_argv, HAVE_GNU_LD && at_file_supplied);
-      post_ld_pass (false);
+      post_ld_pass (/*temp_file*/false);
     }
 
   /* Let scan_prog_file do any final mods (OSF/rose needs this for
@@ -2332,38 +2337,52 @@ write_aix_file (FILE *stream, struct id *list)
 
 /* Check to make sure the file is an LTO object file.  */
 
+static int
+has_lto_section (void *data, const char *name ATTRIBUTE_UNUSED,
+		 off_t offset ATTRIBUTE_UNUSED,
+		 off_t length ATTRIBUTE_UNUSED)
+{
+  int *found = (int *) data;
+
+  if (strncmp (name, LTO_SECTION_NAME_PREFIX,
+	       sizeof (LTO_SECTION_NAME_PREFIX) - 1) != 0)
+    {
+      if (strncmp (name, OFFLOAD_SECTION_NAME_PREFIX,
+	           sizeof (OFFLOAD_SECTION_NAME_PREFIX) - 1) != 0)
+        return 1;
+    }
+
+  *found = 1;
+
+  /* Stop iteration.  */
+  return 0;
+}
+
 static bool
-maybe_lto_object_file (const char *prog_name)
+is_lto_object_file (const char *prog_name)
 {
-  FILE *f;
-  unsigned char buf[4];
-  int i;
+  const char *errmsg;
+  int err;
+  int found = 0;
+  off_t inoff = 0;
+  int infd = open (prog_name, O_RDONLY | O_BINARY);
 
-  static unsigned char elfmagic[4] = { 0x7f, 'E', 'L', 'F' };
-  static unsigned char coffmagic[2] = { 0x4c, 0x01 };
-  static unsigned char coffmagic_x64[2] = { 0x64, 0x86 };
-  static unsigned char machomagic[4][4] = {
-    { 0xcf, 0xfa, 0xed, 0xfe },
-    { 0xce, 0xfa, 0xed, 0xfe },
-    { 0xfe, 0xed, 0xfa, 0xcf },
-    { 0xfe, 0xed, 0xfa, 0xce }
-  };
+  if (infd == -1)
+    return false;
 
-  f = fopen (prog_name, "rb");
-  if (f == NULL)
+  simple_object_read *inobj = simple_object_start_read (infd, inoff,
+							LTO_SEGMENT_NAME,
+							&errmsg, &err);
+  if (!inobj)
     return false;
-  if (fread (buf, sizeof (buf), 1, f) != 1)
-    buf[0] = 0;
-  fclose (f);
 
-  if (memcmp (buf, elfmagic, sizeof (elfmagic)) == 0
-      || memcmp (buf, coffmagic, sizeof (coffmagic)) == 0
-      || memcmp (buf, coffmagic_x64, sizeof (coffmagic_x64)) == 0)
+  errmsg = simple_object_find_sections (inobj, has_lto_section,
+					(void *) &found, &err);
+  if (! errmsg && found)
     return true;
-  for (i = 0; i < 4; i++)
-    if (memcmp (buf, machomagic[i], sizeof (machomagic[i])) == 0)
-      return true;
 
+  if (errmsg)
+    fatal_error (0, "%s: %s\n", errmsg, xstrerror (err));
   return false;
 }
 
@@ -2386,7 +2405,6 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
   int err;
   char *p, buf[1024];
   FILE *inf;
-  int found_lto = 0;
 
   if (which_pass == PASS_SECOND)
     return;
@@ -2394,8 +2412,13 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
   /* LTO objects must be in a known format.  This check prevents
      us from accepting an archive containing LTO objects, which
      gcc cannot currently handle.  */
-  if (which_pass == PASS_LTOINFO && !maybe_lto_object_file (prog_name))
-    return;
+  if (which_pass == PASS_LTOINFO)
+    {
+      if(is_lto_object_file (prog_name)) {
+	add_lto_object (&lto_objects, prog_name);
+      }
+      return;
+    }
 
   /* If we do not have an `nm', complain.  */
   if (nm_file_name == 0)
@@ -2450,12 +2473,7 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
     fatal_error (input_location, "can't open nm output: %m");
 
   if (debug)
-    {
-      if (which_pass == PASS_LTOINFO)
-        fprintf (stderr, "\nnm output with LTO info marker symbol.\n");
-      else
-        fprintf (stderr, "\nnm output with constructors/destructors.\n");
-    }
+    fprintf (stderr, "\nnm output with constructors/destructors.\n");
 
   /* Read each line of nm output.  */
   while (fgets (buf, sizeof buf, inf) != (char *) 0)
@@ -2466,30 +2484,6 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
       if (debug)
         fprintf (stderr, "\t%s\n", buf);
 
-      if (which_pass == PASS_LTOINFO)
-        {
-          if (found_lto)
-            continue;
-
-          /* Look for the LTO info marker symbol, and add filename to
-             the LTO objects list if found.  */
-          for (p = buf; (ch = *p) != '\0' && ch != '\n'; p++)
-            if (ch == ' '  && p[1] == '_' && p[2] == '_'
-		&& (strncmp (p + (p[3] == '_' ? 2 : 1), "__gnu_lto_v1", 12) == 0)
-		&& ISSPACE (p[p[3] == '_' ? 14 : 13]))
-              {
-                add_lto_object (&lto_objects, prog_name);
-
-                /* We need to read all the input, so we can't just
-                   return here.  But we can avoid useless work.  */
-                found_lto = 1;
-
-                break;
-              }
-
-	  continue;
-        }
-
       /* If it contains a constructor or destructor name, add the name
 	 to the appropriate list unless this is a kind of symbol we're
 	 not supposed to even consider.  */
-- 
2.17.1

Re: [PATCH, driver specs][2] Put -flto-partition= on the collect2 c/l

[Richard Biener]

On Fri, Dec 7, 2018 at 1:24 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
> Hi
>
> This got stuck in my stack of patches for LTO debug support, and I forgot to ping it…
>
> > On 22 Aug 2018, at 14:20, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Wed, Aug 22, 2018 at 2:56 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>
> >>
> >>> On 20 Aug 2018, at 11:01, Richard Biener <richard.guenther@gmail.com> wrote:
> >>>
> >>> On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>>>
> >>
> >>>> I plan on making Darwin default to fno-lto for link unless there’s an “flto*” on the link line, since otherwise there’s a process launch for every object on the c/l to do “nm”, which is quite heavy weight.  At present, ISTM if the compiler is configured with —enable-lto, the link line defaults to assuming that every object needs to be checked.
> >>>
> >>> I think we wanted to transparently handle LTO objects (similar to how
> >>> it works with a linker plugin).
> >>
> >> At present, we are not only calling nm for every object, but also dsymutil sometimes unnecessarily - since the assumption on the “maybe_lto” path is that an temporary file will be generated.
> >>
> >> as a headline - when I did the simplistic “default is to assume fno-lto” that took 10% off the bootstrap time (with a stage#1 compiler without the optimisation).
> >>
> >> - I have a patch under test as below + better fidelity of avoiding dsymutil runs.
> >>
> >>> Ideally collect2 wouldn't use nm but simple-object to inspect files
> >>> (but that doesn't have symbol table query support
> >>
> >> Hrm.my WIP had grow symtab awareness to support debug-section copy on mach-o
> >> (and the ELF impl. looks like it understands both symtab and relocs)
> >> - so maybe that’s not as far away as we might think.
> >>
> >>> though looking for LTO specific sections would have been better in the
> >>> first place - I'd suggest .gnu.lto_.symtab).
> >>
> >> I’ve done this noting that the symtab seems to be the last emitted section - is that why you chose it (for security, rather than just stopping as soon as we see a .gnu.lto_. section?)
> >
> > Ah, any .gnu.lto_.section would work as well I guess - I just picked
> > one that should be always
> > created.  .gnu.lto_.opts is another one.
>
> >> comments?
> >
> > OK.  Feel free to stop when recognizing any LTO section.  Btw, as you
> > include lto-section-names.h
> > you should probably use LTO_SECTION_NAME_PREFIX instead of hard-coding
> > .gnu.lto_.
>
> So I just look for the LTO_SECTION_NAME_PREFIX
>
> > I'm not sure if/how we should handle offload sections and if those
> > work at all without a
> > linker plugin currently?
>
> In this version, I look for the OFFLOAD_SECTION_NAME_PREFIX as well,
> although AFAICS collect2 itself has no specific handling of offload, the detection
> is done in lto-wrapper.
>
> OK for trunk now?

OK.

Thanks,
Richard.

> thanks
> Iain
>
>
> From: Iain Sandoe <iain@sandoe.co.uk>
> Date: Tue, 21 Aug 2018 12:55:02 +0100
> Subject: [PATCH] [PATCH, collect2] Use simple-object to find out if objects
>  contain LTO.
>
> This replaces the use of nm to search for the LTO common symbol marker
> and uses simple object to see if there's a section starting with
> ".gnu.lto_." or .gnu.offload_lto_
> ---
>  gcc/collect2.c | 120 +++++++++++++++++++++++--------------------------
>  1 file changed, 57 insertions(+), 63 deletions(-)
>
> diff --git a/gcc/collect2.c b/gcc/collect2.c
> index 60269682ec..dcbd3e18a6 100644
> --- a/gcc/collect2.c
> +++ b/gcc/collect2.c
> @@ -30,6 +30,8 @@ along with GCC; see the file COPYING3.  If not see
>  #include "tm.h"
>  #include "filenames.h"
>  #include "file-find.h"
> +#include "simple-object.h"
> +#include "lto-section-names.h"
>
>  /* TARGET_64BIT may be defined to use driver specific functionality. */
>  #undef TARGET_64BIT
> @@ -804,7 +806,9 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
>        /* Run the linker again, this time replacing the object files
>           optimized by the LTO with the temporary file generated by the LTO.  */
>        fork_execute ("ld", out_lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
> -      post_ld_pass (true);
> +      /* We assume that temp files were created, and therefore we need to take
> +         that into account (maybe run dsymutil).  */
> +      post_ld_pass (/*temp_file*/true);
>        free (lto_ld_argv);
>
>        maybe_unlink_list (lto_o_files);
> @@ -814,10 +818,11 @@ maybe_run_lto_and_relink (char **lto_ld_argv, char **object_lst,
>        /* Our caller is relying on us to do the link
>           even though there is no LTO back end work to be done.  */
>        fork_execute ("ld", lto_ld_argv, HAVE_GNU_LD && at_file_supplied);
> -      post_ld_pass (false);
> +      /* No LTO objects were found, so no new temp file.  */
> +      post_ld_pass (/*temp_file*/false);
>      }
>    else
> -    post_ld_pass (true);
> +    post_ld_pass (false); /* No LTO objects were found, no temp file.  */
>  }
>
>  /* Main program.  */
> @@ -1710,7 +1715,7 @@ main (int argc, char **argv)
>         if (lto_mode != LTO_MODE_NONE)
>           maybe_run_lto_and_relink (ld1_argv, object_lst, object, false);
>         else
> -         post_ld_pass (false);
> +         post_ld_pass (/*temp_file*/false);
>
>         return 0;
>        }
> @@ -1780,7 +1785,7 @@ main (int argc, char **argv)
>  #ifdef COLLECT_EXPORT_LIST
>        maybe_unlink (export_file);
>  #endif
> -      post_ld_pass (false);
> +      post_ld_pass (/*temp_file*/false);
>
>        maybe_unlink (c_file);
>        maybe_unlink (o_file);
> @@ -1874,7 +1879,7 @@ main (int argc, char **argv)
>    else
>      {
>        fork_execute ("ld", ld2_argv, HAVE_GNU_LD && at_file_supplied);
> -      post_ld_pass (false);
> +      post_ld_pass (/*temp_file*/false);
>      }
>
>    /* Let scan_prog_file do any final mods (OSF/rose needs this for
> @@ -2332,38 +2337,52 @@ write_aix_file (FILE *stream, struct id *list)
>
>  /* Check to make sure the file is an LTO object file.  */
>
> +static int
> +has_lto_section (void *data, const char *name ATTRIBUTE_UNUSED,
> +                off_t offset ATTRIBUTE_UNUSED,
> +                off_t length ATTRIBUTE_UNUSED)
> +{
> +  int *found = (int *) data;
> +
> +  if (strncmp (name, LTO_SECTION_NAME_PREFIX,
> +              sizeof (LTO_SECTION_NAME_PREFIX) - 1) != 0)
> +    {
> +      if (strncmp (name, OFFLOAD_SECTION_NAME_PREFIX,
> +                  sizeof (OFFLOAD_SECTION_NAME_PREFIX) - 1) != 0)
> +        return 1;
> +    }
> +
> +  *found = 1;
> +
> +  /* Stop iteration.  */
> +  return 0;
> +}
> +
>  static bool
> -maybe_lto_object_file (const char *prog_name)
> +is_lto_object_file (const char *prog_name)
>  {
> -  FILE *f;
> -  unsigned char buf[4];
> -  int i;
> +  const char *errmsg;
> +  int err;
> +  int found = 0;
> +  off_t inoff = 0;
> +  int infd = open (prog_name, O_RDONLY | O_BINARY);
>
> -  static unsigned char elfmagic[4] = { 0x7f, 'E', 'L', 'F' };
> -  static unsigned char coffmagic[2] = { 0x4c, 0x01 };
> -  static unsigned char coffmagic_x64[2] = { 0x64, 0x86 };
> -  static unsigned char machomagic[4][4] = {
> -    { 0xcf, 0xfa, 0xed, 0xfe },
> -    { 0xce, 0xfa, 0xed, 0xfe },
> -    { 0xfe, 0xed, 0xfa, 0xcf },
> -    { 0xfe, 0xed, 0xfa, 0xce }
> -  };
> +  if (infd == -1)
> +    return false;
>
> -  f = fopen (prog_name, "rb");
> -  if (f == NULL)
> +  simple_object_read *inobj = simple_object_start_read (infd, inoff,
> +                                                       LTO_SEGMENT_NAME,
> +                                                       &errmsg, &err);
> +  if (!inobj)
>      return false;
> -  if (fread (buf, sizeof (buf), 1, f) != 1)
> -    buf[0] = 0;
> -  fclose (f);
>
> -  if (memcmp (buf, elfmagic, sizeof (elfmagic)) == 0
> -      || memcmp (buf, coffmagic, sizeof (coffmagic)) == 0
> -      || memcmp (buf, coffmagic_x64, sizeof (coffmagic_x64)) == 0)
> +  errmsg = simple_object_find_sections (inobj, has_lto_section,
> +                                       (void *) &found, &err);
> +  if (! errmsg && found)
>      return true;
> -  for (i = 0; i < 4; i++)
> -    if (memcmp (buf, machomagic[i], sizeof (machomagic[i])) == 0)
> -      return true;
>
> +  if (errmsg)
> +    fatal_error (0, "%s: %s\n", errmsg, xstrerror (err));
>    return false;
>  }
>
> @@ -2386,7 +2405,6 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>    int err;
>    char *p, buf[1024];
>    FILE *inf;
> -  int found_lto = 0;
>
>    if (which_pass == PASS_SECOND)
>      return;
> @@ -2394,8 +2412,13 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>    /* LTO objects must be in a known format.  This check prevents
>       us from accepting an archive containing LTO objects, which
>       gcc cannot currently handle.  */
> -  if (which_pass == PASS_LTOINFO && !maybe_lto_object_file (prog_name))
> -    return;
> +  if (which_pass == PASS_LTOINFO)
> +    {
> +      if(is_lto_object_file (prog_name)) {
> +       add_lto_object (&lto_objects, prog_name);
> +      }
> +      return;
> +    }
>
>    /* If we do not have an `nm', complain.  */
>    if (nm_file_name == 0)
> @@ -2450,12 +2473,7 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>      fatal_error (input_location, "can't open nm output: %m");
>
>    if (debug)
> -    {
> -      if (which_pass == PASS_LTOINFO)
> -        fprintf (stderr, "\nnm output with LTO info marker symbol.\n");
> -      else
> -        fprintf (stderr, "\nnm output with constructors/destructors.\n");
> -    }
> +    fprintf (stderr, "\nnm output with constructors/destructors.\n");
>
>    /* Read each line of nm output.  */
>    while (fgets (buf, sizeof buf, inf) != (char *) 0)
> @@ -2466,30 +2484,6 @@ scan_prog_file (const char *prog_name, scanpass which_pass,
>        if (debug)
>          fprintf (stderr, "\t%s\n", buf);
>
> -      if (which_pass == PASS_LTOINFO)
> -        {
> -          if (found_lto)
> -            continue;
> -
> -          /* Look for the LTO info marker symbol, and add filename to
> -             the LTO objects list if found.  */
> -          for (p = buf; (ch = *p) != '\0' && ch != '\n'; p++)
> -            if (ch == ' '  && p[1] == '_' && p[2] == '_'
> -               && (strncmp (p + (p[3] == '_' ? 2 : 1), "__gnu_lto_v1", 12) == 0)
> -               && ISSPACE (p[p[3] == '_' ? 14 : 13]))
> -              {
> -                add_lto_object (&lto_objects, prog_name);
> -
> -                /* We need to read all the input, so we can't just
> -                   return here.  But we can avoid useless work.  */
> -                found_lto = 1;
> -
> -                break;
> -              }
> -
> -         continue;
> -        }
> -
>        /* If it contains a constructor or destructor name, add the name
>          to the appropriate list unless this is a kind of symbol we're
>          not supposed to even consider.  */
> --
> 2.17.1
>
>
>

Re: [PATCH, driver specs][3] Put -flto-partition= on the collect2 c/l

[Iain Sandoe]

> On 20 Aug 2018, at 11:01, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>> 
>> Hi
>> 
>> While working on the Darwin LTO issues I noticed that collect2 looks for "-flto-partition=none” in its command line option, but it doesn’t get passed.
>> 
>> So - is the attached patch the right idea, or should collect2 be looking in the COLLECT_GCC_OPTIONS as the lto-wrapper does?
> 
> Looking at COLLECT_GCC_OPTIONS is probably better.

related;  
collect2 is currently “non-obvious” in that it doesn’t respond to -save-temps and to get the tempories saved we have to invoke “-debug” which produces a bunch of other output we probably don’t care about most times.

So .. perhaps we could move to 
 -save-temps (does the “usual thing”)
 -v (does the “usual thing”)
 -debug imples the two above + other informative output

(it would be kinda nice if there was some way to implement -### so that we could see the collect subordinate pass structure)

comments?
thanks
Iain

gcc/
	* collect2.c (main): Parse the output file early so we can make nicer
	temp names.  Respond to “-save-temps” in the GCC OPTIONS.
	(maybe_unlink): Don’t print “[Leaving…”] for files we never created
	and don’t exist.

---
 gcc/collect2.c | 57 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 14 deletions(-)

diff --git a/gcc/collect2.c b/gcc/collect2.c
index 4b0f191ad3..b892e1a2aa 100644
--- a/gcc/collect2.c
+++ b/gcc/collect2.c
@@ -205,8 +205,8 @@ bool helpflag;			/* true if --help */
 static int shared_obj;			/* true if -shared */
 static int static_obj;			/* true if -static */
 
-static const char *c_file;		/* <xxx>.c for constructor/destructor list.  */
-static const char *o_file;		/* <xxx>.o for constructor/destructor list.  */
+static char *c_file;		/* <xxx>.c for constructor/destructor list.  */
+static char *o_file;		/* <xxx>.o for constructor/destructor list.  */
 #ifdef COLLECT_EXPORT_LIST
 static const char *export_file;		/* <xxx>.x for AIX export list.  */
 #endif
@@ -989,6 +989,13 @@ main (int argc, char **argv)
 
   save_temps = false;
   verbose = false;
+
+#ifndef DEFAULT_A_OUT_NAME
+  output_file = "a.out";
+#else
+  output_file = DEFAULT_A_OUT_NAME;
+#endif
+
   /* Parse command line / environment for flags we want early.
      This allows the debug flag to be set before functions like find_a_file()
      are called. */
@@ -1011,7 +1018,17 @@ main (int argc, char **argv)
 	  selected_linker = USE_BFD_LD;
 	else if (strcmp (argv[i], "-fuse-ld=gold") == 0)
 	  selected_linker = USE_GOLD_LD;
-
+	else if (strncmp (argv[i], "-o", 2) == 0)
+	  {
+	    /* Parse the output filename if it's given so that we can make
+	       meaningful temp filenames.  */
+	    if (argv[i][2] == '\0')
+	      output_file = argv[i+1];
+	    else if (argv[i][2] == '=')
+	      output_file = &argv[i][3];
+	    else
+	      output_file = &argv[i][2];
+	  }
 #ifdef COLLECT_EXPORT_LIST
 	/* These flags are position independent, although their order
 	   is important - subsequent flags override earlier ones. */
@@ -1032,12 +1049,6 @@ main (int argc, char **argv)
 #endif
       }
 
-#ifndef DEFAULT_A_OUT_NAME
-  output_file = "a.out";
-#else
-  output_file = DEFAULT_A_OUT_NAME;
-#endif
-
     obstack_begin (&temporary_obstack, 0);
     temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
 
@@ -1058,6 +1069,9 @@ main (int argc, char **argv)
 	  no_partition = true;
 	else if (strncmp (q, "-fno-lto", 8) == 0)
 	  lto_mode = LTO_MODE_NONE;
+	else if (strncmp (q, "-save-temps", 11) == 0)
+	  /* FIXME: Honour =obj.  */
+	  save_temps = true;
     }
     obstack_free (&temporary_obstack, temporary_firstobj);
 
@@ -1217,8 +1231,22 @@ main (int argc, char **argv)
   *ld1++ = *ld2++ = ld_file_name;
 
   /* Make temp file names.  */
-  c_file = make_temp_file (".c");
-  o_file = make_temp_file (".o");
+  if (save_temps)
+    {
+      c_file = (char *) xmalloc (strlen (output_file)
+				  + sizeof (".cdtor.c") + 1);
+      strcpy (c_file, output_file);
+      strcat (c_file, ".cdtor.c");
+      o_file = (char *) xmalloc (strlen (output_file)
+				  + sizeof (".cdtor.o") + 1);
+      strcpy (o_file, output_file);
+      strcat (o_file, ".cdtor.o");
+    }
+  else
+    {
+      c_file = make_temp_file (".cdtor.c");
+      o_file = make_temp_file (".cdtor.o");
+    }
 #ifdef COLLECT_EXPORT_LIST
   export_file = make_temp_file (".x");
 #endif
@@ -1227,6 +1255,7 @@ main (int argc, char **argv)
       ldout = make_temp_file (".ld");
       lderrout = make_temp_file (".le");
     }
+  /* Build the command line to compile the ctor/dtor list.  */
   *c_ptr++ = c_file_name;
   *c_ptr++ = "-x";
   *c_ptr++ = "c";
@@ -1440,6 +1469,8 @@ main (int argc, char **argv)
 	    case 'o':
 	      if (arg[2] == '\0')
 		output_file = *ld1++ = *ld2++ = *++argv;
+	      else if (arg[2] == '=')
+		output_file = &arg[3];
 	      else
 		output_file = &arg[2];
 	      break;
@@ -1684,8 +1715,6 @@ main (int argc, char **argv)
 	else
 	  post_ld_pass (/*temp_file*/false);
 
-	maybe_unlink (c_file);
-	maybe_unlink (o_file);
 	return 0;
       }
   }
@@ -1873,7 +1902,7 @@ main (int argc, char **argv)
 void
 maybe_unlink (const char *file)
 {
-  if (debug)
+  if (save_temps && file_exists (file))
     {
       notice ("[Leaving %s]\n", file);
       return;
-- 
2.17.1

Re: [PATCH, driver specs][3] Put -flto-partition= on the collect2 c/l

[Richard Biener]

On Wed, Aug 22, 2018 at 3:06 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
>
>
> > On 20 Aug 2018, at 11:01, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Sat, Aug 18, 2018 at 9:00 PM Iain Sandoe <iain@sandoe.co.uk> wrote:
> >>
> >> Hi
> >>
> >> While working on the Darwin LTO issues I noticed that collect2 looks for "-flto-partition=none” in its command line option, but it doesn’t get passed.
> >>
> >> So - is the attached patch the right idea, or should collect2 be looking in the COLLECT_GCC_OPTIONS as the lto-wrapper does?
> >
> > Looking at COLLECT_GCC_OPTIONS is probably better.
>
> related;
> collect2 is currently “non-obvious” in that it doesn’t respond to -save-temps and to get the tempories saved we have to invoke “-debug” which produces a bunch of other output we probably don’t care about most times.

Yeah, I noticed this in the past as well (as well as buffering all
output without -debug).  But I
got used to specify -save-temps -Wl,-debug ...

>
> So .. perhaps we could move to
>  -save-temps (does the “usual thing”)
>  -v (does the “usual thing”)
>  -debug imples the two above + other informative output

Makes sense.

>
> (it would be kinda nice if there was some way to implement -### so that we could see the collect subordinate pass structure)
>
> comments?

LGTM, aka OK.

Thanks,
Richard.

> thanks
> Iain
>
> gcc/
>         * collect2.c (main): Parse the output file early so we can make nicer
>         temp names.  Respond to “-save-temps” in the GCC OPTIONS.
>         (maybe_unlink): Don’t print “[Leaving…”] for files we never created
>         and don’t exist.
>
> ---
>  gcc/collect2.c | 57 +++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/gcc/collect2.c b/gcc/collect2.c
> index 4b0f191ad3..b892e1a2aa 100644
> --- a/gcc/collect2.c
> +++ b/gcc/collect2.c
> @@ -205,8 +205,8 @@ bool helpflag;                      /* true if --help */
>  static int shared_obj;                 /* true if -shared */
>  static int static_obj;                 /* true if -static */
>
> -static const char *c_file;             /* <xxx>.c for constructor/destructor list.  */
> -static const char *o_file;             /* <xxx>.o for constructor/destructor list.  */
> +static char *c_file;           /* <xxx>.c for constructor/destructor list.  */
> +static char *o_file;           /* <xxx>.o for constructor/destructor list.  */
>  #ifdef COLLECT_EXPORT_LIST
>  static const char *export_file;                /* <xxx>.x for AIX export list.  */
>  #endif
> @@ -989,6 +989,13 @@ main (int argc, char **argv)
>
>    save_temps = false;
>    verbose = false;
> +
> +#ifndef DEFAULT_A_OUT_NAME
> +  output_file = "a.out";
> +#else
> +  output_file = DEFAULT_A_OUT_NAME;
> +#endif
> +
>    /* Parse command line / environment for flags we want early.
>       This allows the debug flag to be set before functions like find_a_file()
>       are called. */
> @@ -1011,7 +1018,17 @@ main (int argc, char **argv)
>           selected_linker = USE_BFD_LD;
>         else if (strcmp (argv[i], "-fuse-ld=gold") == 0)
>           selected_linker = USE_GOLD_LD;
> -
> +       else if (strncmp (argv[i], "-o", 2) == 0)
> +         {
> +           /* Parse the output filename if it's given so that we can make
> +              meaningful temp filenames.  */
> +           if (argv[i][2] == '\0')
> +             output_file = argv[i+1];
> +           else if (argv[i][2] == '=')
> +             output_file = &argv[i][3];
> +           else
> +             output_file = &argv[i][2];
> +         }
>  #ifdef COLLECT_EXPORT_LIST
>         /* These flags are position independent, although their order
>            is important - subsequent flags override earlier ones. */
> @@ -1032,12 +1049,6 @@ main (int argc, char **argv)
>  #endif
>        }
>
> -#ifndef DEFAULT_A_OUT_NAME
> -  output_file = "a.out";
> -#else
> -  output_file = DEFAULT_A_OUT_NAME;
> -#endif
> -
>      obstack_begin (&temporary_obstack, 0);
>      temporary_firstobj = (char *) obstack_alloc (&temporary_obstack, 0);
>
> @@ -1058,6 +1069,9 @@ main (int argc, char **argv)
>           no_partition = true;
>         else if (strncmp (q, "-fno-lto", 8) == 0)
>           lto_mode = LTO_MODE_NONE;
> +       else if (strncmp (q, "-save-temps", 11) == 0)
> +         /* FIXME: Honour =obj.  */
> +         save_temps = true;
>      }
>      obstack_free (&temporary_obstack, temporary_firstobj);
>
> @@ -1217,8 +1231,22 @@ main (int argc, char **argv)
>    *ld1++ = *ld2++ = ld_file_name;
>
>    /* Make temp file names.  */
> -  c_file = make_temp_file (".c");
> -  o_file = make_temp_file (".o");
> +  if (save_temps)
> +    {
> +      c_file = (char *) xmalloc (strlen (output_file)
> +                                 + sizeof (".cdtor.c") + 1);
> +      strcpy (c_file, output_file);
> +      strcat (c_file, ".cdtor.c");
> +      o_file = (char *) xmalloc (strlen (output_file)
> +                                 + sizeof (".cdtor.o") + 1);
> +      strcpy (o_file, output_file);
> +      strcat (o_file, ".cdtor.o");
> +    }
> +  else
> +    {
> +      c_file = make_temp_file (".cdtor.c");
> +      o_file = make_temp_file (".cdtor.o");
> +    }
>  #ifdef COLLECT_EXPORT_LIST
>    export_file = make_temp_file (".x");
>  #endif
> @@ -1227,6 +1255,7 @@ main (int argc, char **argv)
>        ldout = make_temp_file (".ld");
>        lderrout = make_temp_file (".le");
>      }
> +  /* Build the command line to compile the ctor/dtor list.  */
>    *c_ptr++ = c_file_name;
>    *c_ptr++ = "-x";
>    *c_ptr++ = "c";
> @@ -1440,6 +1469,8 @@ main (int argc, char **argv)
>             case 'o':
>               if (arg[2] == '\0')
>                 output_file = *ld1++ = *ld2++ = *++argv;
> +             else if (arg[2] == '=')
> +               output_file = &arg[3];
>               else
>                 output_file = &arg[2];
>               break;
> @@ -1684,8 +1715,6 @@ main (int argc, char **argv)
>         else
>           post_ld_pass (/*temp_file*/false);
>
> -       maybe_unlink (c_file);
> -       maybe_unlink (o_file);
>         return 0;
>        }
>    }
> @@ -1873,7 +1902,7 @@ main (int argc, char **argv)
>  void
>  maybe_unlink (const char *file)
>  {
> -  if (debug)
> +  if (save_temps && file_exists (file))
>      {
>        notice ("[Leaving %s]\n", file);
>        return;
> --
> 2.17.1
>
>